From a4dfd7715856469a899219383ac3dc0af2f2df81 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 21 Feb 2018 14:11:55 +0100 Subject: [PATCH 1/2] Refactoring on PlannedNode and KubernetesSlave to improve extensibility * New extension point PlannedNodeBuilderFactory to allow extensions to provide alternative PlannedNode implementations. * Builder pattern for KubernetesSlave to stop the explosion of constructors --- .../plugins/kubernetes/KubernetesCloud.java | 14 +- .../plugins/kubernetes/KubernetesSlave.java | 136 +++++++++++++++++- .../kubernetes/PlannedNodeBuilder.java | 81 +++++++++++ .../kubernetes/PlannedNodeBuilderFactory.java | 37 +++++ .../kubernetes/ProvisioningCallback.java | 31 ++-- .../StandardPlannedNodeBuilder.java | 16 +++ 6 files changed, 283 insertions(+), 32 deletions(-) create mode 100644 src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilder.java create mode 100644 src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilderFactory.java create mode 100644 src/main/java/org/csanchez/jenkins/plugins/kubernetes/StandardPlannedNodeBuilder.java diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java index 136a1ed2bf..bfb0734af1 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java @@ -51,7 +51,6 @@ import hudson.Util; import hudson.init.InitMilestone; import hudson.init.Initializer; -import hudson.model.Computer; import hudson.model.Descriptor; import hudson.model.Label; import hudson.model.Node; @@ -405,9 +404,7 @@ public synchronized Collection provision(@CheckForN if (!addProvisionedSlave(t, label)) { break; } - - r.add(new NodeProvisioner.PlannedNode(t.getDisplayName(), Computer.threadPoolForRemoting - .submit(new ProvisioningCallback(this, t, label)), 1)); + r.add(PlannedNodeBuilderFactory.createInstance().cloud(this).template(t).label(label).build()); } if (r.size() > 0) { // Already found a matching template @@ -487,6 +484,15 @@ public PodTemplate getTemplate(@CheckForNull Label label) { return PodTemplateUtils.getTemplateByLabel(label, getAllTemplates()); } + /** + * Unwraps the given pod template. + * @param podTemplate the pod template to unwrap. + * @return the unwrapped pod template + */ + public PodTemplate getUnwrappedTemplate(PodTemplate podTemplate) { + return PodTemplateUtils.unwrap(podTemplate, getDefaultsProviderTemplate(), getAllTemplates()); + } + /** * Gets all PodTemplates that have the matching {@link Label}. * @param label label to look for in templates diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java index 4cc2650035..23fe3a5ead 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java @@ -14,6 +14,7 @@ import org.apache.commons.lang.RandomStringUtils; import org.apache.commons.lang.StringUtils; +import org.apache.commons.lang.Validate; import org.jenkinsci.plugins.durabletask.executors.Messages; import org.jenkinsci.plugins.durabletask.executors.OnceRetentionStrategy; import org.jvnet.localizer.Localizable; @@ -29,6 +30,8 @@ import hudson.model.TaskListener; import hudson.slaves.AbstractCloudSlave; import hudson.slaves.Cloud; +import hudson.slaves.CloudRetentionStrategy; +import hudson.slaves.ComputerLauncher; import hudson.slaves.OfflineCause; import hudson.slaves.RetentionStrategy; import io.fabric8.kubernetes.client.KubernetesClient; @@ -62,7 +65,7 @@ public PodTemplate getTemplate() { } /** - * @deprecated Use {@link #KubernetesSlave(PodTemplate, String, String, String, RetentionStrategy)} instead. + * @deprecated Use {@link Builder} instead. */ @Deprecated public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesCloud cloud, String labelStr) @@ -72,7 +75,7 @@ public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesC } /** - * @deprecated Use {@link #KubernetesSlave(PodTemplate, String, String, String, RetentionStrategy)} instead. + * @deprecated Use {@link Builder} instead. */ @Deprecated public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesCloud cloud, Label label) @@ -81,7 +84,7 @@ public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesC } /** - * @deprecated Use {@link #KubernetesSlave(PodTemplate, String, String, String, RetentionStrategy)} instead. + * @deprecated Use {@link Builder} instead. */ @Deprecated public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesCloud cloud, String labelStr, @@ -90,18 +93,27 @@ public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesC this(template, nodeDescription, cloud.name, labelStr, rs); } - @DataBoundConstructor + /** + * @deprecated Use {@link Builder} instead. + */ + @Deprecated + @DataBoundConstructor // make stapler happy. Not actually used. public KubernetesSlave(PodTemplate template, String nodeDescription, String cloudName, String labelStr, RetentionStrategy rs) throws Descriptor.FormException, IOException { + this(getSlaveName(template), template, nodeDescription, cloudName, labelStr, new KubernetesLauncher(), rs); + } - super(getSlaveName(template), + protected KubernetesSlave(String name, PodTemplate template, String nodeDescription, String cloudName, String labelStr, + ComputerLauncher computerLauncher, RetentionStrategy rs) + throws Descriptor.FormException, IOException { + super(name, nodeDescription, template.getRemoteFs(), 1, template.getNodeUsageMode() != null ? template.getNodeUsageMode() : Node.Mode.NORMAL, labelStr == null ? null : labelStr, - new KubernetesLauncher(), + computerLauncher, rs, template.getNodeProperties()); @@ -257,6 +269,118 @@ public int hashCode() { return result; } + /** + * Returns a new {@link Builder} instance. + * @return a new {@link Builder} instance. + */ + public static Builder builder() { + return new Builder(); + } + + /** + * Builds a {@link KubernetesSlave} instance. + */ + public static class Builder { + private String name; + private String nodeDescription; + private PodTemplate podTemplate; + private KubernetesCloud cloud; + private String label; + private ComputerLauncher computerLauncher; + private RetentionStrategy retentionStrategy; + + /** + * @param name The name of the future {@link KubernetesSlave} + * @return the current instance for method chaining + */ + public Builder name(String name) { + this.name = name; + return this; + } + + /** + * @param nodeDescription The node description of the future {@link KubernetesSlave} + * @return the current instance for method chaining + */ + public Builder nodeDescription(String nodeDescription) { + this.nodeDescription = nodeDescription; + return this; + } + + /** + * @param podTemplate The pod template the future {@link KubernetesSlave} has been created from + * @return the current instance for method chaining + */ + public Builder podTemplate(PodTemplate podTemplate) { + this.podTemplate = podTemplate; + return this; + } + + /** + * @param cloud The cloud that is provisioning the {@link KubernetesSlave} instance. + * @return the current instance for method chaining + */ + public Builder cloud(KubernetesCloud cloud) { + this.cloud = cloud; + return this; + } + + /** + * @param label The label the {@link KubernetesSlave} has. + * @return the current instance for method chaining + */ + public Builder label(String label) { + this.label = label; + return this; + } + + /** + * @param computerLauncher The computer launcher to use to launch the {@link KubernetesSlave} instance. + * @return the current instance for method chaining + */ + public Builder computerLauncher(ComputerLauncher computerLauncher) { + this.computerLauncher = computerLauncher; + return this; + } + + /** + * @param retentionStrategy The retention strategy to use for the {@link KubernetesSlave} instance. + * @return the current instance for method chaining + */ + public Builder retentionStrategy(RetentionStrategy retentionStrategy) { + this.retentionStrategy = retentionStrategy; + return this; + } + + private RetentionStrategy determineRetentionStrategy() { + if (podTemplate.getIdleMinutes() == 0) { + return new OnceRetentionStrategy(cloud.getRetentionTimeout()); + } else { + return new CloudRetentionStrategy(podTemplate.getIdleMinutes()); + } + } + + /** + * Builds the resulting {@link KubernetesSlave} instance. + * @return an initialized {@link KubernetesSlave} instance. + * @throws IOException + * @throws Descriptor.FormException + */ + public KubernetesSlave build() throws IOException, Descriptor.FormException { + Validate.notNull(podTemplate); + Validate.notNull(cloud); + return new KubernetesSlave( + name == null ? getSlaveName(podTemplate) : name, + podTemplate, + nodeDescription == null ? podTemplate.getName() : nodeDescription, + cloud.name, + label == null ? podTemplate.getLabel() : label, + computerLauncher == null ? new KubernetesLauncher() : computerLauncher, + retentionStrategy == null ? determineRetentionStrategy() : retentionStrategy); + } + } + + @Extension public static final class DescriptorImpl extends SlaveDescriptor { diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilder.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilder.java new file mode 100644 index 0000000000..8782805773 --- /dev/null +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilder.java @@ -0,0 +1,81 @@ +package org.csanchez.jenkins.plugins.kubernetes; + +import hudson.model.Label; +import hudson.slaves.NodeProvisioner; + +/** + * A builder of {@link hudson.slaves.NodeProvisioner.PlannedNode} implementations for Kubernetes. + * Can be subclassed to provide alternative implementations of {@link hudson.slaves.NodeProvisioner.PlannedNode}. + */ +public abstract class PlannedNodeBuilder { + private KubernetesCloud cloud; + private PodTemplate template; + private Label label; + private int numExecutors = 1; + + /** + * Returns the {@link KubernetesCloud}. + * @return the {@link KubernetesCloud}. + */ + public KubernetesCloud getCloud() { + return cloud; + } + + /** + * Returns the {@link PodTemplate}. + * @return + */ + public PodTemplate getTemplate() { + return template; + } + + public Label getLabel() { + return label; + } + + public int getNumExecutors() { + return numExecutors; + } + + /** + * @param cloud the {@link KubernetesCloud} instance to use. + * @return the current builder. + */ + public PlannedNodeBuilder cloud(KubernetesCloud cloud) { + this.cloud = cloud; + return this; + } + + /** + * @param template the {@link PodTemplate} instance to use. + * @return the current builder. + */ + public PlannedNodeBuilder template(PodTemplate template) { + this.template = template; + return this; + } + + /** + * @param label the {@link Label} to use. + * @return the current builder. + */ + public PlannedNodeBuilder label(Label label) { + this.label = label; + return this; + } + + /** + * @param numExecutors the number of executors. + * @return the current builder. + */ + public PlannedNodeBuilder numExecutors(int numExecutors) { + this.numExecutors = numExecutors; + return this; + } + + /** + * Builds the {@link hudson.slaves.NodeProvisioner.PlannedNode} instance based on the given inputs. + * @return a {@link hudson.slaves.NodeProvisioner.PlannedNode} configured from this builder. + */ + public abstract NodeProvisioner.PlannedNode build(); +} diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilderFactory.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilderFactory.java new file mode 100644 index 0000000000..fb6787aefa --- /dev/null +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PlannedNodeBuilderFactory.java @@ -0,0 +1,37 @@ +package org.csanchez.jenkins.plugins.kubernetes; + +import hudson.ExtensionList; +import hudson.ExtensionPoint; + +/** + * A factory of {@link PlannedNodeBuilder} instances. + */ +public abstract class PlannedNodeBuilderFactory implements ExtensionPoint { + /** + * Returns all registered implementations of {@link PlannedNodeBuilderFactory}. + * @return all registered implementations of {@link PlannedNodeBuilderFactory}. + */ + public static ExtensionList all() { + return ExtensionList.lookup(PlannedNodeBuilderFactory.class); + } + + /** + * Returns a new instance of {@link PlannedNodeBuilder}. + * @return a new instance of {@link PlannedNodeBuilder}. + */ + public static PlannedNodeBuilder createInstance() { + for (PlannedNodeBuilderFactory factory: all()) { + PlannedNodeBuilder plannedNodeBuilder = factory.newInstance(); + if (plannedNodeBuilder != null) { + return plannedNodeBuilder; + } + } + return new StandardPlannedNodeBuilder(); + } + + /** + * Creates a new instance of {@link PlannedNodeBuilder}. + * @return a new instance of {@link PlannedNodeBuilder}. + */ + public abstract PlannedNodeBuilder newInstance(); +} diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java index a5de2d79a2..75dcdb04cf 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java @@ -24,15 +24,11 @@ package org.csanchez.jenkins.plugins.kubernetes; -import hudson.model.Label; -import hudson.model.Node; -import hudson.slaves.CloudRetentionStrategy; -import hudson.slaves.RetentionStrategy; -import org.jenkinsci.plugins.durabletask.executors.OnceRetentionStrategy; +import java.util.concurrent.Callable; -import javax.annotation.CheckForNull; import javax.annotation.Nonnull; -import java.util.concurrent.Callable; + +import hudson.model.Node; /** * Callback for Kubernetes cloud provision @@ -45,27 +41,18 @@ class ProvisioningCallback implements Callable { private final KubernetesCloud cloud; @Nonnull private final PodTemplate t; - @CheckForNull - private final Label label; - public ProvisioningCallback(@Nonnull KubernetesCloud cloud, @Nonnull PodTemplate t, @CheckForNull Label label) { + public ProvisioningCallback(@Nonnull KubernetesCloud cloud, @Nonnull PodTemplate t) { this.cloud = cloud; this.t = t; - this.label = label; } public Node call() throws Exception { - RetentionStrategy retentionStrategy; - if (t.getIdleMinutes() == 0) { - retentionStrategy = new OnceRetentionStrategy(cloud.getRetentionTimeout()); - } else { - retentionStrategy = new CloudRetentionStrategy(t.getIdleMinutes()); - } - - final PodTemplate unwrappedTemplate = PodTemplateUtils.unwrap(cloud.getTemplate(label), - cloud.getDefaultsProviderTemplate(), cloud.getTemplates()); - return new KubernetesSlave(unwrappedTemplate, unwrappedTemplate.getName(), cloud.name, - unwrappedTemplate.getLabel(), retentionStrategy); + return KubernetesSlave + .builder() + .podTemplate(cloud.getUnwrappedTemplate(t)) + .cloud(cloud) + .build(); } } diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/StandardPlannedNodeBuilder.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/StandardPlannedNodeBuilder.java new file mode 100644 index 0000000000..d376d16059 --- /dev/null +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/StandardPlannedNodeBuilder.java @@ -0,0 +1,16 @@ +package org.csanchez.jenkins.plugins.kubernetes; + +import hudson.model.Computer; +import hudson.slaves.NodeProvisioner; + +/** + * The default {@link PlannedNodeBuilder} implementation, in case there is other registered. + */ +public class StandardPlannedNodeBuilder extends PlannedNodeBuilder { + @Override + public NodeProvisioner.PlannedNode build() { + return new NodeProvisioner.PlannedNode(getTemplate().getDisplayName(), + Computer.threadPoolForRemoting.submit(new ProvisioningCallback(getCloud(), getTemplate())), + getNumExecutors()); + } +} From 28d9597c83226a5d2a02bec698fe2df90b7b85d8 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 21 Mar 2018 13:00:55 +0100 Subject: [PATCH 2/2] Fix api breakage --- .../plugins/kubernetes/ProvisioningCallback.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java index 75dcdb04cf..a3863aba26 100644 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/ProvisioningCallback.java @@ -26,8 +26,10 @@ import java.util.concurrent.Callable; +import javax.annotation.CheckForNull; import javax.annotation.Nonnull; +import hudson.model.Label; import hudson.model.Node; /** @@ -42,6 +44,14 @@ class ProvisioningCallback implements Callable { @Nonnull private final PodTemplate t; + /** + * @deprecated Use {@link ProvisioningCallback#ProvisioningCallback(KubernetesCloud, PodTemplate)} instead. + */ + @Deprecated + public ProvisioningCallback(@Nonnull KubernetesCloud cloud, @Nonnull PodTemplate t, @CheckForNull Label label) { + this(cloud, t); + } + public ProvisioningCallback(@Nonnull KubernetesCloud cloud, @Nonnull PodTemplate t) { this.cloud = cloud; this.t = t;