From ff4ce527ca01cdc11421f22a8407fef9a67d08e8 Mon Sep 17 00:00:00 2001 From: Fred Cooke Date: Fri, 7 May 2021 16:54:35 +1200 Subject: [PATCH 1/7] Add optional ID to the pod template step class such that it can be supplied by dynamic groovy and then applied if present in the step execution, thus leaving functionality unchanged for existing use cases such as manually configuring pod templates in a Jenkins UI. --- .../plugins/kubernetes/pipeline/PodTemplateStep.java | 11 +++++++++++ .../kubernetes/pipeline/PodTemplateStepExecution.java | 7 ++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStep.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStep.java index 86364212f9..a533f993b7 100755 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStep.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStep.java @@ -54,6 +54,8 @@ public class PodTemplateStep extends Step implements Serializable { @CheckForNull private String name; + private String id; + @CheckForNull private String namespace; @@ -118,11 +120,20 @@ public void setLabel(@CheckForNull String label) { this.label = Util.fixEmpty(label); } + @DataBoundSetter + public void setId(String id) { + this.id = Util.fixEmpty(id); + } + @CheckForNull public String getName() { return name; } + public String getId() { + return id; + } + @DataBoundSetter public void setName(@CheckForNull String name) { this.name = Util.fixEmpty(name); diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java index 6c4cf2f768..633ab5138f 100755 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java @@ -84,7 +84,12 @@ public boolean start() throws Exception { String name = String.format(NAME_FORMAT, stepName, randString); String namespace = checkNamespace(cloud, podTemplateContext); - newTemplate = new PodTemplate(); + if (step.getId() != null) { + newTemplate = new PodTemplate(step.getId()); + } else { + newTemplate = new PodTemplate(); + } + newTemplate.setName(name); newTemplate.setNamespace(namespace); From b8ba0e07543dd0379f1594b97a43257b0a869c89 Mon Sep 17 00:00:00 2001 From: Fred Cooke Date: Fri, 7 May 2021 16:57:52 +1200 Subject: [PATCH 2/7] Add missing spaces to an if condition to aid readability. --- .../plugins/kubernetes/pipeline/PodTemplateStepExecution.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java index 633ab5138f..3ffc112be1 100755 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java @@ -120,7 +120,7 @@ public boolean start() throws Exception { newTemplate.setAnnotations(step.getAnnotations()); newTemplate.setListener(getContext().get(TaskListener.class)); newTemplate.setYamlMergeStrategy(step.getYamlMergeStrategy()); - if(run!=null) { + if(run != null) { String url = cloud.getJenkinsUrlOrNull(); if(url != null) { newTemplate.getAnnotations().add(new PodAnnotation("buildUrl", url + run.getUrl())); From 3f3178e3c00f836961ee90935338f7fe466fe612 Mon Sep 17 00:00:00 2001 From: Fred Cooke Date: Fri, 14 May 2021 09:54:32 +1200 Subject: [PATCH 3/7] No point checking for null, the constructor handles the null the same as the default constructor which just passes null directly into this constructor anyway. So if there's a value we get what we want, and if there's not, we still get what we want. Simpler is better. --- .../kubernetes/pipeline/PodTemplateStepExecution.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java index 3ffc112be1..fcc0dc0ec0 100755 --- a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java +++ b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java @@ -84,12 +84,7 @@ public boolean start() throws Exception { String name = String.format(NAME_FORMAT, stepName, randString); String namespace = checkNamespace(cloud, podTemplateContext); - if (step.getId() != null) { - newTemplate = new PodTemplate(step.getId()); - } else { - newTemplate = new PodTemplate(); - } - + newTemplate = new PodTemplate(step.getId()); newTemplate.setName(name); newTemplate.setNamespace(namespace); From 6dacdf5b2c09997b2da14c5a082480b1f3620b7d Mon Sep 17 00:00:00 2001 From: Fred Cooke Date: Tue, 1 Jun 2021 17:38:43 +1200 Subject: [PATCH 4/7] Expand static imports out so the api usage footprint is visible in diffs and locally used methods can always be found explicitly by name. --- .../kubernetes/pipeline/RestartPipelineTest.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelineTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelineTest.java index 2136deb675..5425baffd7 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelineTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelineTest.java @@ -24,8 +24,15 @@ package org.csanchez.jenkins.plugins.kubernetes.pipeline; -import static java.util.Arrays.*; -import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.*; +import static java.util.Arrays.asList; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.setupHost; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.setupCloud; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.createSecret; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.createPipelineJobThenScheduleRun; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.assumeWindows; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.assumeKubernetes; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.getLabels; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.deletePods; import static org.junit.Assert.assertTrue; import java.io.IOException; From 0003b1756256cb52e4a338dc6c254b47159e5663 Mon Sep 17 00:00:00 2001 From: Fred Cooke Date: Tue, 1 Jun 2021 18:30:06 +1200 Subject: [PATCH 5/7] NUKE this commit before final merge, obviously. Hoping this will allow a faster CI build on the PR as the full suite is massive. --- test-in-k8s.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test-in-k8s.sh b/test-in-k8s.sh index 8c55fab33b..ae11775049 100644 --- a/test-in-k8s.sh +++ b/test-in-k8s.sh @@ -13,6 +13,8 @@ kubectl exec jenkins -- mkdir /checkout kubectl cp pom.xml jenkins:/checkout/pom.xml kubectl cp .mvn jenkins:/checkout/.mvn kubectl cp src jenkins:/checkout/src + +TEST='RestartPipelinePodTemplateIdConsistencyTest,RestartPipelineTest' if [ -v TEST ] then args="-Dtest=$TEST test" From a7abe45ce1036a711a9a169a738029d7b6c6320a Mon Sep 17 00:00:00 2001 From: Fred Cooke Date: Tue, 1 Jun 2021 19:07:26 +1200 Subject: [PATCH 6/7] First cut of a test that I hope fails - for the right reason. Not 100 percent confident in that, but can't seem to run it locally, seems like a precondition is for a legit kube env to be available for this particular test. That's both good and bad as it's more realistic but I don't currently have that available myself. --- ...tPipelinePodTemplateIdConsistencyTest.java | 173 ++++++++++++++++++ ...namicPodTemplateStepSupportsRestart.groovy | 57 ++++++ 2 files changed, 230 insertions(+) create mode 100644 src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java create mode 100644 src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/dynamicPodTemplateStepSupportsRestart.groovy diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java new file mode 100644 index 0000000000..850a827b41 --- /dev/null +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java @@ -0,0 +1,173 @@ +/* + * The MIT License + * + * Copyright (c) 2017, Red Hat, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.csanchez.jenkins.plugins.kubernetes.pipeline; + +import static java.util.Arrays.asList; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.setupHost; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.setupCloud; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.createSecret; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.createPipelineJobThenScheduleRun; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.assumeWindows; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.assumeKubernetes; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.getLabels; +import static org.csanchez.jenkins.plugins.kubernetes.KubernetesTestUtil.deletePods; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.util.Collections; +import java.util.Optional; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Level; +import java.util.logging.Logger; + +import org.apache.commons.compress.utils.IOUtils; +import org.csanchez.jenkins.plugins.kubernetes.ContainerEnvVar; +import org.csanchez.jenkins.plugins.kubernetes.ContainerTemplate; +import org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud; +import org.csanchez.jenkins.plugins.kubernetes.KubernetesSlave; +import org.csanchez.jenkins.plugins.kubernetes.PodTemplate; +import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar; +import org.csanchez.jenkins.plugins.kubernetes.model.SecretEnvVar; +import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.jenkinsci.plugins.workflow.support.steps.ExecutorStepExecution; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.rules.TestName; +import org.jvnet.hudson.test.BuildWatcher; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.LoggerRule; +import org.jvnet.hudson.test.RestartableJenkinsNonLocalhostRule; + +import hudson.model.Node; +import hudson.model.Result; +import hudson.slaves.DumbSlave; +import hudson.slaves.JNLPLauncher; +import hudson.slaves.NodeProperty; +import hudson.slaves.RetentionStrategy; + +public class RestartPipelinePodTemplateIdConsistencyTest { + protected static final String CONTAINER_ENV_VAR_VALUE = "container-env-var-value"; + protected static final String POD_ENV_VAR_VALUE = "pod-env-var-value"; + protected static final String SECRET_KEY = "password"; + protected static final String CONTAINER_ENV_VAR_FROM_SECRET_VALUE = "container-pa55w0rd"; + protected static final String POD_ENV_VAR_FROM_SECRET_VALUE = "pod-pa55w0rd"; + protected KubernetesCloud cloud; + + @Rule + public RestartableJenkinsNonLocalhostRule story = new RestartableJenkinsNonLocalhostRule(44000); + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + @ClassRule + public static BuildWatcher buildWatcher = new BuildWatcher(); + + @Rule + public LoggerRule logs = new LoggerRule().record(Logger.getLogger(KubernetesCloud.class.getPackage().getName()), + Level.ALL); + //.record("org.jenkinsci.plugins.durabletask", Level.ALL).record("org.jenkinsci.plugins.workflow.support.concurrent", Level.ALL).record("org.csanchez.jenkins.plugins.kubernetes.pipeline", Level.ALL); + + @Rule + public TestName name = new TestName(); + + @BeforeClass + public static void isKubernetesConfigured() throws Exception { + assumeKubernetes(); + } + + private static void setEnvVariables(PodTemplate podTemplate) { + TemplateEnvVar podSecretEnvVar = new SecretEnvVar("POD_ENV_VAR_FROM_SECRET", "pod-secret", SECRET_KEY, false); + TemplateEnvVar podSimpleEnvVar = new KeyValueEnvVar("POD_ENV_VAR", POD_ENV_VAR_VALUE); + podTemplate.setEnvVars(asList(podSecretEnvVar, podSimpleEnvVar)); + TemplateEnvVar containerEnvVariable = new KeyValueEnvVar("CONTAINER_ENV_VAR", CONTAINER_ENV_VAR_VALUE); + TemplateEnvVar containerEnvVariableLegacy = new ContainerEnvVar("CONTAINER_ENV_VAR_LEGACY", + CONTAINER_ENV_VAR_VALUE); + TemplateEnvVar containerSecretEnvVariable = new SecretEnvVar("CONTAINER_ENV_VAR_FROM_SECRET", + "container-secret", SECRET_KEY, false); + podTemplate.getContainers().get(0) + .setEnvVars(asList(containerEnvVariable, containerEnvVariableLegacy, containerSecretEnvVariable)); + } + + public void configureCloud() throws Exception { + cloud = setupCloud(this, name); + createSecret(cloud.connect(), cloud.getNamespace()); + cloud.getTemplates().clear(); + + setupHost(); + + story.j.jenkins.clouds.add(cloud); + } + + public void configureAgentListener() throws IOException { + //Take random port and fix it, to be the same after Jenkins restart + int fixedPort = story.j.jenkins.getTcpSlaveAgentListener().getAdvertisedPort(); + story.j.jenkins.setSlaveAgentPort(fixedPort); + } + + protected String loadPipelineScript(String name) { + try { + return new String(IOUtils.toByteArray(getClass().getResourceAsStream(name))); + } catch (Throwable t) { + throw new RuntimeException("Could not read resource:[" + name + "]."); + } + } + + @Test + public void dynamicPodTemplateStepSupportsRestart() { + story.then(r -> { + // Do this only once: + configureAgentListener(); + configureCloud(); + r.jenkins.setNumExecutors(0); + // Do this stuff here and then again in a new story.then() call to simulate restart + WorkflowRun b = getPipelineJobThenScheduleRun(r); + r.waitForMessage("dynamic pod template step success", b); + r.assertBuildStatus(Result.SUCCESS, r.waitForCompletion(b)); + }); + story.then(r -> { + // Do what we do in the first one again, except the configureCloud and before bits + WorkflowRun b = getPipelineJobThenScheduleRun(r); + r.waitForMessage("dynamic pod template step success", b); + r.assertBuildStatus(Result.SUCCESS, r.waitForCompletion(b)); + }); + } + + private PodTemplate waitForTemplate(KubernetesSlave node) throws InterruptedException { + while (node.getTemplateOrNull() == null) { + Thread.sleep(100L); + } + return node.getTemplate(); + } + + private WorkflowRun getPipelineJobThenScheduleRun(JenkinsRule r) throws InterruptedException, ExecutionException, IOException { + return createPipelineJobThenScheduleRun(r, getClass(), name.getMethodName()); + } +} diff --git a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/dynamicPodTemplateStepSupportsRestart.groovy b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/dynamicPodTemplateStepSupportsRestart.groovy new file mode 100644 index 0000000000..bfb9e90b9f --- /dev/null +++ b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/dynamicPodTemplateStepSupportsRestart.groovy @@ -0,0 +1,57 @@ +def call(body) { + def config = [:] + body.resolveStrategy = Closure.DELEGATE_FIRST + body.delegate = config + milestone() + def yamlPod = ''' +apiVersion: v1 +kind: Pod +metadata: + labels: + jenkins: agent +spec: + containers: + - name: maven-image + command: + - cat + image: busybox + imagePullPolicy: IfNotPresent + resources: + limits: + cpu: 100m + memory: 8Mi + requests: + cpu: 100m + memory: 8Mi + tty: true + dnsPolicy: ClusterFirst + restartPolicy: Never + securityContext: {} + terminationGracePeriodSeconds: 30 +''' + def yamlPodHash = '#{project.artifactId}-#{project.version}-maven-image' + + // one build per job to ensure tag consistency + try { + lock(resource: env.JOB_NAME.split('/')[1], inversePrecedence: true) { + milestone() + podTemplate(label: yamlPodHash, instanceCap: 6, idleMinutes: 60, cloud: 'dynamicPodTemplateStepSupportsRestart', yaml: yamlPod) { + node(yamlPodHash) { + container('maven-image') { + timeout(20) { + println "dynamic pod template step success" + } + } + } + } + } + + println "${JOB_NAME} ${currentBuild.displayName} see ${JOB_URL}" + + } + catch (e) { + println e + println "${JOB_NAME} ${currentBuild.displayName} see ${JOB_URL}" + throw e + } +} From f9e55068655ebabac22f4be69183a035d5ee2b2e Mon Sep 17 00:00:00 2001 From: Fred Cooke Date: Tue, 1 Jun 2021 19:27:25 +1200 Subject: [PATCH 7/7] Replace the expected message with one that is actually output to get it into the next part of the same test. The message wasn't important and I've clearly misconfigured it somehow. --- .../pipeline/RestartPipelinePodTemplateIdConsistencyTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java index 850a827b41..cac4182e6d 100644 --- a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java +++ b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/RestartPipelinePodTemplateIdConsistencyTest.java @@ -149,13 +149,13 @@ public void dynamicPodTemplateStepSupportsRestart() { r.jenkins.setNumExecutors(0); // Do this stuff here and then again in a new story.then() call to simulate restart WorkflowRun b = getPipelineJobThenScheduleRun(r); - r.waitForMessage("dynamic pod template step success", b); + r.waitForMessage("End of Pipeline", b); r.assertBuildStatus(Result.SUCCESS, r.waitForCompletion(b)); }); story.then(r -> { // Do what we do in the first one again, except the configureCloud and before bits WorkflowRun b = getPipelineJobThenScheduleRun(r); - r.waitForMessage("dynamic pod template step success", b); + r.waitForMessage("End of Pipeline", b); r.assertBuildStatus(Result.SUCCESS, r.waitForCompletion(b)); }); }