From 3992a30efb1abf697cae8e02f9e81dd04b92b135 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 25 Sep 2020 22:34:05 +0200 Subject: [PATCH] Add `CloudState` to be passed to `Cloud#provision` and `Cloud#canProvision` methods. (#4922) Co-authored-by: Jesse Glick --- core/src/main/java/hudson/Util.java | 25 +++++ core/src/main/java/hudson/slaves/Cloud.java | 99 ++++++++++++++++++- .../slaves/CloudProvisioningListener.java | 27 ++++- .../java/hudson/slaves/NodeProvisioner.java | 8 +- core/src/test/java/hudson/UtilTest.java | 31 ++++++ 5 files changed, 183 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index caef4d1020..6db16d5b04 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -79,6 +79,7 @@ import java.time.temporal.ChronoUnit; import java.util.*; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; import java.util.logging.Level; import java.util.logging.LogRecord; import java.util.logging.Logger; @@ -1460,6 +1461,30 @@ public class Util { return derivedMethod != null && derivedMethod != baseMethod; } + /** + * Calls the given supplier if the method defined on the base type with the given arguments is overridden in the + * given derived type. + * + * @param supplier The supplier to call if the method is indeed overridden. + * @param base The base type. + * @param derived The derived type. + * @param methodName The name of the method. + * @param types The types of the arguments for the method. + * @return {@code true} when {@code derived} provides the specified method other than as inherited from {@code base}. + * @throws IllegalArgumentException When {@code derived} does not derive from {@code base}, or when {@code base} + * does not contain the specified method. + * @throws AbstractMethodError If the derived class doesn't override the given method. + * @since TODO + */ + public static T ifOverridden(Supplier supplier, @NonNull Class base, @NonNull Class derived, @NonNull String methodName, @NonNull Class... types) { + if (isOverridden(base, derived, methodName, types)) { + return supplier.get(); + } else { + throw new AbstractMethodError("You must override at least one of the " + + base.getSimpleName() + "." + methodName + " methods"); + } + } + private static Method getMethod(@NonNull Class clazz, @Nullable Class base, @NonNull String methodName, @NonNull Class... types) { try { final Method res = clazz.getDeclaredMethod(methodName, types); diff --git a/core/src/main/java/hudson/slaves/Cloud.java b/core/src/main/java/hudson/slaves/Cloud.java index 9eeb4ad1d7..354dbbdac9 100644 --- a/core/src/main/java/hudson/slaves/Cloud.java +++ b/core/src/main/java/hudson/slaves/Cloud.java @@ -23,9 +23,11 @@ */ package hudson.slaves; +import edu.umd.cs.findbugs.annotations.CheckForNull; import hudson.ExtensionPoint; import hudson.Extension; import hudson.DescriptorExtensionList; +import hudson.Util; import hudson.model.Actionable; import hudson.model.Computer; import hudson.model.Slave; @@ -156,13 +158,68 @@ public abstract class Cloud extends Actionable implements ExtensionPoint, Descri * just needs to return {@link PlannedNode}s that each contain an object that implements {@link Future}. * When the {@link Future} has completed its work, {@link Future#get} will be called to obtain the * provisioned {@link Node} object. + * @deprecated Use {@link #provision(CloudState, int)} instead. */ - public abstract Collection provision(Label label, int excessWorkload); + @Deprecated + public Collection provision(Label label, int excessWorkload) { + return Util.ifOverridden(() -> provision(new CloudState(label, 0), excessWorkload), + Cloud.class, + getClass(), + "provision", + CloudState.class, + int.class); + } + + /** + * Provisions new {@link Node}s from this cloud. + * + *

+ * {@link NodeProvisioner} performs a trend analysis on the load, + * and when it determines that it really needs to bring up + * additional nodes, this method is invoked. + * + *

+ * The implementation of this method asynchronously starts + * node provisioning. + * + * @param state the current state. + * @param excessWorkload + * Number of total executors needed to meet the current demand. + * Always ≥ 1. For example, if this is 3, the implementation + * should launch 3 agents with 1 executor each, or 1 agent with + * 3 executors, etc. + * @return + * {@link PlannedNode}s that represent asynchronous {@link Node} + * provisioning operations. Can be empty but must not be null. + * {@link NodeProvisioner} will be responsible for adding the resulting {@link Node}s + * into Hudson via {@link jenkins.model.Jenkins#addNode(Node)}, so a {@link Cloud} implementation + * just needs to return {@link PlannedNode}s that each contain an object that implements {@link Future}. + * When the {@link Future} has completed its work, {@link Future#get} will be called to obtain the + * provisioned {@link Node} object. + */ + public Collection provision(CloudState state, int excessWorkload) { + return provision(state.getLabel(), excessWorkload); + } + + /** + * Returns true if this cloud is capable of provisioning new nodes for the given label. + * @deprecated Use {@link #canProvision(CloudState)} instead. + */ + @Deprecated + public boolean canProvision(Label label) { + return Util.ifOverridden(() -> canProvision(new CloudState(label, 0)), + Cloud.class, + getClass(), + "canProvision", + CloudState.class); + } /** * Returns true if this cloud is capable of provisioning new nodes for the given label. */ - public abstract boolean canProvision(Label label); + public boolean canProvision(CloudState state) { + return canProvision(state.getLabel()); + } public Descriptor getDescriptor() { return Jenkins.get().getDescriptorOrDie(getClass()); @@ -194,4 +251,42 @@ public abstract class Cloud extends Actionable implements ExtensionPoint, Descri public static final Permission PROVISION = new Permission( Computer.PERMISSIONS, "Provision", Messages._Cloud_ProvisionPermission_Description(), Jenkins.ADMINISTER, PERMISSION_SCOPE ); + + /** + * Parameter object for {@link hudson.slaves.Cloud}. + * @since TODO + */ + public static final class CloudState { + /** + * The label under consideration. + */ + @CheckForNull + private final Label label; + /** + * The additional planned capacity for this {@link #label} and provisioned by previous strategies during the + * current updating of the {@link NodeProvisioner}. + */ + private final int additionalPlannedCapacity; + + public CloudState(@CheckForNull Label label, int additionalPlannedCapacity) { + this.label = label; + this.additionalPlannedCapacity = additionalPlannedCapacity; + } + + /** + * The label under consideration. + */ + @CheckForNull + public Label getLabel() { + return label; + } + + /** + * The additional planned capacity for this {@link #getLabel()} and provisioned by previous strategies during + * the current updating of the {@link NodeProvisioner}. + */ + public int getAdditionalPlannedCapacity() { + return additionalPlannedCapacity; + } + } } diff --git a/core/src/main/java/hudson/slaves/CloudProvisioningListener.java b/core/src/main/java/hudson/slaves/CloudProvisioningListener.java index a25433191e..fec52f98e1 100644 --- a/core/src/main/java/hudson/slaves/CloudProvisioningListener.java +++ b/core/src/main/java/hudson/slaves/CloudProvisioningListener.java @@ -2,9 +2,11 @@ package hudson.slaves; import hudson.ExtensionList; import hudson.ExtensionPoint; +import hudson.Util; import hudson.model.Label; import hudson.model.Node; import hudson.model.queue.CauseOfBlockage; +import hudson.slaves.Cloud.CloudState; import jenkins.model.Jenkins; import java.util.Collection; @@ -35,9 +37,32 @@ public abstract class CloudProvisioningListener implements ExtensionPoint { * * @return {@code null} if provisioning can proceed, or a * {@link CauseOfBlockage} reason why it cannot be provisioned. + * + * @deprecated Use {@link #canProvision(Cloud, CloudState, int)} instead. */ + @Deprecated public CauseOfBlockage canProvision(Cloud cloud, Label label, int numExecutors) { - return null; + return Util.ifOverridden(() -> canProvision(cloud, new CloudState(label, 0), numExecutors), + CloudProvisioningListener.class, + getClass(), + "canProvision", + CloudState.class); + } + + /** + * Allows extensions to prevent a cloud from provisioning. + * + * Return null to allow provisioning, or non-null to prevent it. + * + * @param cloud The cloud being provisioned from. + * @param state The current cloud state. + * @param numExecutors The number of executors needed. + * + * @return {@code null} if provisioning can proceed, or a + * {@link CauseOfBlockage} reason why it cannot be provisioned. + */ + public CauseOfBlockage canProvision(Cloud cloud, CloudState state, int numExecutors) { + return canProvision(cloud, state.getLabel(), numExecutors); } /** diff --git a/core/src/main/java/hudson/slaves/NodeProvisioner.java b/core/src/main/java/hudson/slaves/NodeProvisioner.java index 2a03bad8e3..991c09c23c 100644 --- a/core/src/main/java/hudson/slaves/NodeProvisioner.java +++ b/core/src/main/java/hudson/slaves/NodeProvisioner.java @@ -705,9 +705,10 @@ public class NodeProvisioner { if (excessWorkload < 0) { break; // enough agents allocated } + Cloud.CloudState cloudState = new Cloud.CloudState(state.getLabel(), state.getAdditionalPlannedCapacity()); // Make sure this cloud actually can provision for this label. - if (c.canProvision(state.getLabel())) { + if (c.canProvision(cloudState)) { // provisioning a new node should be conservative --- for example if excessWorkload is 1.4, // we don't want to allocate two nodes but just one. // OTOH, because of the exponential decay, even when we need one agent, @@ -719,14 +720,13 @@ public class NodeProvisioner { int workloadToProvision = (int) Math.round(Math.floor(excessWorkload + m)); for (CloudProvisioningListener cl : CloudProvisioningListener.all()) { - if (cl.canProvision(c, state.getLabel(), workloadToProvision) != null) { + if (cl.canProvision(c, cloudState, workloadToProvision) != null) { // consider displaying reasons in a future cloud ux continue CLOUD; } } - Collection additionalCapacities = - c.provision(state.getLabel(), workloadToProvision); + Collection additionalCapacities = c.provision(cloudState, workloadToProvision); fireOnStarted(c, state.getLabel(), additionalCapacities); diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index 1426ecb657..87a454f573 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -30,6 +30,7 @@ import hudson.util.StreamTaskListener; import org.apache.commons.io.FileUtils; import org.hamcrest.BaseMatcher; import org.hamcrest.Description; +import org.junit.Assert; import org.junit.Assume; import org.junit.Rule; import org.junit.Test; @@ -586,4 +587,34 @@ public class UtilTest { assertNull(Util.resolveSymlinkToFile(new File(_a, "aa"))); assertNull(Util.resolveSymlinkToFile(new File(_a, "aa/aa.txt"))); } + + @Test + public void ifOverriddenSuccess() { + assertTrue(Util.ifOverridden(() -> true, BaseClass.class, DerivedClassSuccess.class, "method")); + } + + @Test + public void ifOverriddenFailure() { + AbstractMethodError error = Assert.assertThrows(AbstractMethodError.class, () -> { + Util.ifOverridden(() -> true, BaseClass.class, DerivedClassFailure.class, "method"); + }); + assertEquals("You must override at least one of the BaseClass.method methods", error.getMessage()); + } + + public static class BaseClass { + protected String method() { + return "base"; + } + } + + public static class DerivedClassFailure extends BaseClass { + + } + + public static class DerivedClassSuccess extends BaseClass { + @Override + protected String method() { + return DerivedClassSuccess.class.getName(); + } + } }