From 357f20c65ebf1e08460e7e3ddcdfa6cb66be27d0 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 2 May 2022 17:46:56 -0400 Subject: [PATCH 1/9] Interpreting `ErrorCondition` --- pom.xml | 2 + .../plugins/workflow/steps/RetryStep.java | 14 ++++- .../workflow/steps/RetryStepExecution.java | 36 +++++++++---- ...onousResumeNotSupportedErrorCondition.java | 53 +++++++++++++++++++ .../workflow/steps/RetryStep/config.jelly | 3 ++ .../steps/RetryStep/help-errorConditions.html | 6 +++ .../plugins/workflow/steps/PushdStepTest.java | 3 +- 7 files changed, 106 insertions(+), 11 deletions(-) create mode 100644 src/main/java/org/jenkinsci/plugins/workflow/steps/SynchronousResumeNotSupportedErrorCondition.java create mode 100644 src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-errorConditions.html diff --git a/pom.xml b/pom.xml index 3ed8ba0b..228881b0 100644 --- a/pom.xml +++ b/pom.xml @@ -96,6 +96,7 @@ org.jenkins-ci.plugins.workflow workflow-api + 1150.vea_8e33e127c8 org.jenkins-ci.plugins @@ -124,6 +125,7 @@ org.jenkins-ci.plugins.workflow workflow-durable-task-step + org.jenkins-ci.plugins.workflow diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java index 25033550..3d8e581e 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java @@ -28,8 +28,11 @@ import hudson.Extension; import hudson.model.TaskListener; import java.util.Collections; +import java.util.List; import java.util.Set; +import org.jenkinsci.plugins.workflow.flow.ErrorCondition; import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.DataBoundSetter; /** * Executes the body up to N times. @@ -39,6 +42,7 @@ public class RetryStep extends Step { private final int count; + private List errorConditions; @DataBoundConstructor public RetryStep(int count) { @@ -49,6 +53,14 @@ public int getCount() { return count; } + public List getErrorConditions() { + return errorConditions; + } + + @DataBoundSetter public void setErrorConditions(List errorConditions) { + this.errorConditions = errorConditions; + } + @Override public DescriptorImpl getDescriptor() { return (DescriptorImpl)super.getDescriptor(); @@ -56,7 +68,7 @@ public DescriptorImpl getDescriptor() { @Override public StepExecution start(StepContext context) throws Exception { - return new RetryStepExecution(count, context); + return new RetryStepExecution(count, context, errorConditions); } @Extension diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java index cc58ac46..f4172b5e 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java @@ -1,10 +1,14 @@ package org.jenkinsci.plugins.workflow.steps; +import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.AbortException; import hudson.Functions; import hudson.model.Run; import hudson.model.TaskListener; +import java.io.IOException; +import java.util.List; +import org.jenkinsci.plugins.workflow.flow.ErrorCondition; /** * @author Kohsuke Kawaguchi @@ -14,16 +18,20 @@ public class RetryStepExecution extends AbstractStepExecutionImpl { @SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Only used when starting.") private transient final int count; - RetryStepExecution(int count, StepContext context) { + @SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Only used when starting.") + private transient final @CheckForNull List errorConditions; + + RetryStepExecution(int count, StepContext context, List errorConditions) { super(context); this.count = count; + this.errorConditions = errorConditions; } @Override public boolean start() throws Exception { StepContext context = getContext(); context.newBodyInvoker() - .withCallback(new Callback(count)) + .withCallback(new Callback(count, errorConditions)) .start(); return false; // execution is asynchronous } @@ -33,9 +41,11 @@ public boolean start() throws Exception { private static class Callback extends BodyExecutionCallback { private int left; + private final @CheckForNull List errorConditions; - Callback(int count) { + Callback(int count, List errorConditions) { left = count; + this.errorConditions = errorConditions; } /* Could be added, but seems unnecessary, given the message already printed in onFailure: @@ -56,13 +66,9 @@ public void onSuccess(StepContext context, Object result) { @Override public void onFailure(StepContext context, Throwable t) { try { - if (t instanceof FlowInterruptedException && ((FlowInterruptedException)t).isActualInterruption()) { - context.onFailure(t); - return; - } left--; - if (left>0) { - TaskListener l = context.get(TaskListener.class); + TaskListener l = context.get(TaskListener.class); + if (left > 0 && matchesErrorConditions(t, context)) { if (t instanceof AbortException) { l.error(t.getMessage()); } else if (t instanceof FlowInterruptedException) { @@ -82,6 +88,18 @@ public void onFailure(StepContext context, Throwable t) { } } + private boolean matchesErrorConditions(Throwable t, StepContext context) throws IOException, InterruptedException { + if (errorConditions == null || errorConditions.isEmpty()) { + return !(t instanceof FlowInterruptedException) || !((FlowInterruptedException) t).isActualInterruption(); + } + for (ErrorCondition ec : errorConditions) { + if (ec.test(t, context)) { + return true; + } + } + return false; + } + private static final long serialVersionUID = 1L; } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/SynchronousResumeNotSupportedErrorCondition.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/SynchronousResumeNotSupportedErrorCondition.java new file mode 100644 index 00000000..e3ac6a70 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/SynchronousResumeNotSupportedErrorCondition.java @@ -0,0 +1,53 @@ +/* + * The MIT License + * + * Copyright 2022 CloudBees, 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.jenkinsci.plugins.workflow.steps; + +import hudson.Extension; +import java.io.IOException; +import org.jenkinsci.Symbol; +import org.jenkinsci.plugins.workflow.flow.ErrorCondition; +import org.kohsuke.stapler.DataBoundConstructor; + +/** + * Checks for {@link SynchronousResumeNotSupportedException}. + */ +public final class SynchronousResumeNotSupportedErrorCondition extends ErrorCondition { + + @DataBoundConstructor public SynchronousResumeNotSupportedErrorCondition() {} + + @Override public boolean test(Throwable error, StepContext context) throws IOException, InterruptedException { + return error instanceof SynchronousResumeNotSupportedException; + } + + @Symbol("nonresumable") + @Extension public static final class DescriptorImpl extends ErrorConditionDescriptor { + + @Override public String getDisplayName() { + return "Non-resumable steps"; + } + + } + +} diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly b/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly index a734bff4..4fc01b78 100644 --- a/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly @@ -28,4 +28,7 @@ THE SOFTWARE. + + + diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-errorConditions.html b/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-errorConditions.html new file mode 100644 index 00000000..b5a023e8 --- /dev/null +++ b/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-errorConditions.html @@ -0,0 +1,6 @@ +
+ Conditions under which the block should be retried. + If none match, the block will fail. + If there are no specified conditions, + the block will always be retried except in case of user aborts. +
diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/PushdStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/PushdStepTest.java index c39e923b..7f24aa01 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/PushdStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/PushdStepTest.java @@ -44,7 +44,8 @@ public class PushdStepTest { @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher(); @Rule public JenkinsSessionRule sessions = new JenkinsSessionRule(); - @Rule public LoggerRule logging = new LoggerRule().record(FilePathDynamicContext.class, Level.FINE); + @Rule public LoggerRule logging = new LoggerRule().recordPackage(FilePathDynamicContext.class, Level.FINE); + // logging.recordPackage(FilePathPickle.class, Level.FINE); @Test public void basics() throws Throwable { sessions.then(j -> { From 91a17d72f63a8d33d9faecf10507dd7faf7f4114 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 3 May 2022 17:18:39 -0400 Subject: [PATCH 2/9] Extracted `RetryExecutorStepTest` from `ExecutorStepTest`, rewrote to `retry` step --- pom.xml | 5 +- .../workflow/steps/RetryExecutorStepTest.java | 241 ++++++++++++++++++ 2 files changed, 244 insertions(+), 2 deletions(-) create mode 100644 src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java diff --git a/pom.xml b/pom.xml index 228881b0..529b3a4a 100644 --- a/pom.xml +++ b/pom.xml @@ -64,6 +64,7 @@ 999999-SNAPSHOT 2.332.1 + 1746.v2b_3c5d1983b_e true jenkinsci/${project.artifactId}-plugin @@ -72,7 +73,7 @@ io.jenkins.tools.bom bom-2.332.x - 1289.v5c4b_1c43511b_ + 1342.v729ca_3818e88 import pom @@ -125,7 +126,7 @@ org.jenkins-ci.plugins.workflow workflow-durable-task-step - + 1189.v39b_0b_d397ccf org.jenkins-ci.plugins.workflow diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java new file mode 100644 index 00000000..4b59fbcd --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java @@ -0,0 +1,241 @@ +/* + * The MIT License + * + * Copyright 2022 CloudBees, 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.jenkinsci.plugins.workflow.steps; + +import hudson.Functions; +import hudson.model.Slave; +import hudson.model.TaskListener; +import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; +import java.util.logging.Level; +import org.jenkinsci.Symbol; +import org.jenkinsci.plugins.durabletask.FileMonitoringTask; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; +import org.jenkinsci.plugins.workflow.flow.ErrorCondition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep; +import org.jenkinsci.plugins.workflow.support.steps.AgentErrorCondition; +import org.jenkinsci.plugins.workflow.support.steps.ExecutorStepExecution; +import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep; +import org.junit.Assume; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.BuildWatcher; +import org.jvnet.hudson.test.InboundAgentRule; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsSessionRule; +import org.jvnet.hudson.test.LoggerRule; +import org.jvnet.hudson.test.TestExtension; +import org.kohsuke.stapler.DataBoundConstructor; + +/** + * Tests of retrying {@code node} blocks which should probably be moved to {@code workflow-durable-task-step} when feasible. + */ +public class RetryExecutorStepTest { + + @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher(); + @Rule public JenkinsSessionRule sessions = new JenkinsSessionRule(); + @Rule public InboundAgentRule inboundAgents = new InboundAgentRule(); + @Rule public LoggerRule logging = new LoggerRule(); + + @Issue("JENKINS-49707") + @Test public void retryNodeBlock() throws Throwable { + Assume.assumeFalse("TODO corresponding batch script TBD", Functions.isWindows()); + sessions.then(r -> { + logging.record(DurableTaskStep.class, Level.FINE).record(FileMonitoringTask.class, Level.FINE).record(ExecutorStepExecution.class, Level.FINE); + Slave s = inboundAgents.createAgent(r, "dumbo1"); + s.setLabelString("dumb"); + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition( + "retry(count: 2, errorConditions: [custom()]) {\n" + + " node('dumb') {\n" + + " sh 'sleep 10'\n" + + " }\n" + + "}", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + r.waitForMessage("+ sleep", b); + inboundAgents.stop("dumbo1"); + r.jenkins.removeNode(s); + r.waitForMessage(RetryThis.MESSAGE, b); + s = inboundAgents.createAgent(r, "dumbo2"); + s.setLabelString("dumb"); + r.jenkins.updateNode(s); // to force setLabelString to be honored + r.waitForMessage("Running on dumbo2 in ", b); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + }); + } + + @Issue("JENKINS-49707") + @Test public void retryNodeBlockSynch() throws Throwable { + Assume.assumeFalse("TODO corresponding Windows process TBD", Functions.isWindows()); + sessions.then(r -> { + logging.record(ExecutorStepExecution.class, Level.FINE); + Slave s = inboundAgents.createAgent(r, "dumbo1"); + s.setLabelString("dumb"); + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition( + "retry(count: 2, errorConditions: [custom()]) {\n" + + " node('dumb') {\n" + + " hang()\n" + + " }\n" + + "}", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + r.waitForMessage("$ sleep", b); + // Immediate kill causes RequestAbortedException from RemoteLauncher.launch, which passes test; + // but more realistic to see IOException: Backing channel 'JNLP4-connect connection from …' is disconnected. + // from RemoteLauncher$ProcImpl.isAlive via RemoteInvocationHandler.channelOrFail. + // Either way the top-level exception wraps ClosedChannelException: + Thread.sleep(1000); + inboundAgents.stop("dumbo1"); + r.jenkins.removeNode(s); + r.waitForMessage(RetryThis.MESSAGE, b); + s = inboundAgents.createAgent(r, "dumbo2"); + s.setLabelString("dumb"); + r.jenkins.updateNode(s); + r.waitForMessage("Running on dumbo2 in ", b); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + }); + } + public static final class HangStep extends Step { + @DataBoundConstructor public HangStep() {} + @Override public StepExecution start(StepContext context) throws Exception { + return StepExecutions.synchronousNonBlocking(context, c -> { + c.get(hudson.Launcher.class).launch().cmds("sleep", "10").stdout(c.get(TaskListener.class)).start().join(); + return null; + }); + } + @TestExtension("retryNodeBlockSynch") public static final class DescriptorImpl extends StepDescriptor { + @Override public String getFunctionName() { + return "hang"; + } + @Override public Set> getRequiredContext() { + return new HashSet<>(Arrays.asList(hudson.Launcher.class, TaskListener.class)); + } + } + } + + @Issue("JENKINS-49707") + @Test public void retryNewStepAcrossRestarts() throws Throwable { + logging.record(DurableTaskStep.class, Level.FINE).record(FileMonitoringTask.class, Level.FINE).record(ExecutorStepExecution.class, Level.FINE); + sessions.then(r -> { + Slave s = inboundAgents.createAgent(r, "dumbo1"); + s.setLabelString("dumb"); + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition( + "retry(count: 2, errorConditions: [custom()]) {\n" + + " node('dumb') {\n" + + " semaphore 'wait'\n" + + " isUnix()\n" + + " }\n" + + "}", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + SemaphoreStep.waitForStart("wait/1", b); + }); + sessions.then(r -> { + inboundAgents.stop("dumbo1"); + r.jenkins.removeNode(r.jenkins.getNode("dumbo1")); + SemaphoreStep.success("wait/1", null); + SemaphoreStep.success("wait/2", null); + WorkflowRun b = r.jenkins.getItemByFullName("p", WorkflowJob.class).getBuildByNumber(1); + r.waitForMessage(RetryThis.MESSAGE, b); + Slave s = inboundAgents.createAgent(r, "dumbo2"); + s.setLabelString("dumb"); + r.jenkins.updateNode(s); + r.waitForMessage("Running on dumbo2 in ", b); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + }); + } + + @Issue({"JENKINS-49707", "JENKINS-30383"}) + @Test public void retryNodeBlockSynchAcrossRestarts() throws Throwable { + logging.record(ExecutorStepExecution.class, Level.FINE); + sessions.then(r -> { + Slave s = inboundAgents.createAgent(r, "dumbo1"); + s.setLabelString("dumb"); + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition( + "retry(count: 2, errorConditions: [custom()]) {\n" + + " node('dumb') {\n" + + " waitWithoutAgent()\n" + + " }\n" + + "}", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + r.waitForMessage("Sleeping without agent", b); + }); + sessions.then(r -> { + inboundAgents.stop("dumbo1"); + r.jenkins.removeNode(r.jenkins.getNode("dumbo1")); + WorkflowRun b = r.jenkins.getItemByFullName("p", WorkflowJob.class).getBuildByNumber(1); + r.waitForMessage(RetryThis.MESSAGE, b); + Slave s = inboundAgents.createAgent(r, "dumbo2"); + s.setLabelString("dumb"); + r.jenkins.updateNode(s); + r.waitForMessage("Running on dumbo2 in ", b); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + }); + } + public static final class WaitWithoutAgentStep extends Step { + @DataBoundConstructor public WaitWithoutAgentStep() {} + @Override public StepExecution start(StepContext context) throws Exception { + return StepExecutions.synchronousNonBlocking(context, c -> { + c.get(TaskListener.class).getLogger().println("Sleeping without agent"); + Thread.sleep(10_000); + return null; + }); + } + @TestExtension("retryNodeBlockSynchAcrossRestarts") public static final class DescriptorImpl extends StepDescriptor { + @Override public String getFunctionName() { + return "waitWithoutAgent"; + } + @Override public Set> getRequiredContext() { + return Collections.singleton(TaskListener.class); + } + } + } + + public static final class RetryThis extends ErrorCondition { + private static final String MESSAGE = "Satisfactory retry condition"; + @DataBoundConstructor public RetryThis() {} + @Override public boolean test(Throwable t, StepContext context) throws IOException, InterruptedException { + TaskListener listener = context.get(TaskListener.class); + Functions.printStackTrace(t, listener.getLogger()); + if (new AgentErrorCondition().test(t, context) || new SynchronousResumeNotSupportedErrorCondition().test(t, context)) { + listener.getLogger().println(MESSAGE); + return true; + } else { + listener.getLogger().println("Ignoring " + t); + return false; + } + } + @Symbol("custom") + @TestExtension public static final class DescriptorImpl extends ErrorConditionDescriptor {} + } + +} From 846c7bf8052a498f7050e63a3f8203cd165e8821 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 4 May 2022 10:43:58 -0400 Subject: [PATCH 3/9] More gracefully handle `StepContext` errors from `CoreWrapperStep.Callback` --- .../workflow/steps/CoreWrapperStep.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java index 1dcee6c5..e4721039 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStep.java @@ -41,6 +41,8 @@ import java.util.Map; import java.util.Set; import edu.umd.cs.findbugs.annotations.NonNull; +import java.util.logging.Level; +import java.util.logging.Logger; import jenkins.model.Jenkins; import jenkins.tasks.SimpleBuildWrapper; import org.kohsuke.stapler.DataBoundConstructor; @@ -50,6 +52,8 @@ */ public class CoreWrapperStep extends Step { + private static final Logger LOGGER = Logger.getLogger(CoreWrapperStep.class.getName()); + private final SimpleBuildWrapper delegate; @DataBoundConstructor public CoreWrapperStep(SimpleBuildWrapper delegate) { @@ -177,15 +181,26 @@ private static final class Callback extends BodyExecutionCallback.TailCall { assert run != null; final TaskListener listener = context.get(TaskListener.class); assert listener != null; - final FilePath workspace = context.get(FilePath.class); - final Launcher launcher = context.get(Launcher.class); + FilePath workspace; + Launcher launcher; if (disposer.requiresWorkspace()) { + workspace = context.get(FilePath.class); if (workspace == null) { throw new MissingContextVariableException(FilePath.class); } + launcher = context.get(Launcher.class); if (launcher == null) { throw new MissingContextVariableException(Launcher.class); } + } else { + try { + workspace = context.get(FilePath.class); + launcher = context.get(Launcher.class); + } catch (IOException | InterruptedException x) { + LOGGER.log(Level.FINE, null, x); + workspace = null; + launcher = null; + } } // always pass the workspace context when available, even when it is not strictly required if (workspace != null && launcher != null) { From b88832385fdbc9dfeeaab1f427d9b204dcfbb261 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 4 May 2022 11:21:32 -0400 Subject: [PATCH 4/9] Analogue of https://github.com/jenkinsci/jenkins-test-harness/pull/428 for `createSpecialEnvSlave` --- .../jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java index 1240d6ad..1e7a234c 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java @@ -257,8 +257,7 @@ private static class UpcaseFilter extends ConsoleLogFilter implements Serializab * @see explanation in core PR 1553 */ public static Slave createSpecialEnvSlave(JenkinsRule rule, String nodeName, @CheckForNull String labels, Map env) throws Exception { - @SuppressWarnings("deprecation") // keep consistency with original signature rather than force the caller to pass in a TemporaryFolder rule - File remoteFS = rule.createTmpDir(); + File remoteFS = new File(rule.jenkins.getRootDir(), "agent-work-dirs/" + nodeName); SpecialEnvSlave slave = new SpecialEnvSlave(remoteFS, rule.createComputerLauncher(/* yes null */null), nodeName, labels != null ? labels : "", env); rule.jenkins.addNode(slave); return slave; From 0004499239c3f0c22544ef37db92558cff1fba16 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 4 May 2022 15:03:19 -0400 Subject: [PATCH 5/9] Work around test failure for now --- .../plugins/workflow/steps/CoreWrapperStepTest.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java index 1e7a234c..2782ed27 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java @@ -55,6 +55,7 @@ import java.util.Map; import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; +import java.util.logging.Level; import jenkins.tasks.SimpleBuildWrapper; import org.hamcrest.Matchers; import org.jenkinsci.Symbol; @@ -79,15 +80,19 @@ import org.kohsuke.stapler.DataBoundConstructor; import static org.hamcrest.MatcherAssert.assertThat; +import org.jenkinsci.plugins.workflow.support.steps.ExecutorStepDynamicContext; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import org.jvnet.hudson.test.LoggerRule; public class CoreWrapperStepTest { @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher(); @Rule public JenkinsSessionRule sessions = new JenkinsSessionRule(); + @Rule public LoggerRule logging = new LoggerRule(); @Test public void useWrapper() throws Throwable { + logging.record(ExecutorStepDynamicContext.class, Level.FINE); sessions.then(j -> { new SnippetizerTester(j).assertRoundTrip(new CoreWrapperStep(new MockWrapper()), "mock {\n // some block\n}"); Map slaveEnv = new HashMap<>(); @@ -106,6 +111,7 @@ public class CoreWrapperStepTest { SemaphoreStep.waitForStart("restarting/1", b); }); sessions.then(j -> { + j.waitOnline((Slave) j.jenkins.getNode("slave")); // TODO otherwise sh will fail until the agent reconnects SemaphoreStep.success("restarting/1", null); WorkflowJob p = j.jenkins.getItemByFullName("p", WorkflowJob.class); WorkflowRun b = p.getLastBuild(); From a43a123f79f34f3d410ad6baa253b6d665db8a0a Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 12 May 2022 07:55:08 -0400 Subject: [PATCH 6/9] Picking up `ExecutorStepExecution.onResume` rewrite --- pom.xml | 9 ++++++--- .../plugins/workflow/steps/CoreWrapperStepTest.java | 1 - 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 529b3a4a..9e3cb179 100644 --- a/pom.xml +++ b/pom.xml @@ -73,7 +73,7 @@ io.jenkins.tools.bom bom-2.332.x - 1342.v729ca_3818e88 + 1370.vfa_e23fe119c3 import pom @@ -97,7 +97,7 @@ org.jenkins-ci.plugins.workflow workflow-api - 1150.vea_8e33e127c8 + 1159.v27cb_4545c3ff org.jenkins-ci.plugins @@ -110,23 +110,26 @@ org.jenkins-ci.plugins.workflow workflow-cps + 2691.va_688a_c3d8fd0 test org.jenkins-ci.plugins.workflow workflow-cps + 2691.va_688a_c3d8fd0 tests test org.jenkins-ci.plugins.workflow workflow-job + 1181.vcea_0362753c3 test org.jenkins-ci.plugins.workflow workflow-durable-task-step - 1189.v39b_0b_d397ccf + 1200.v7231de192754 org.jenkins-ci.plugins.workflow diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java index 2782ed27..c2a32f2c 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/CoreWrapperStepTest.java @@ -111,7 +111,6 @@ public class CoreWrapperStepTest { SemaphoreStep.waitForStart("restarting/1", b); }); sessions.then(j -> { - j.waitOnline((Slave) j.jenkins.getNode("slave")); // TODO otherwise sh will fail until the agent reconnects SemaphoreStep.success("restarting/1", null); WorkflowJob p = j.jenkins.getItemByFullName("p", WorkflowJob.class); WorkflowRun b = p.getLastBuild(); From 145f716def482946f5f42a1d2f6149fdfb05c6f7 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 19 May 2022 17:47:27 -0400 Subject: [PATCH 7/9] =?UTF-8?q?`errorConditions`=20=E2=86=92=20`conditions?= =?UTF-8?q?`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../plugins/workflow/steps/RetryStep.java | 12 +++++----- .../workflow/steps/RetryStepExecution.java | 22 +++++++++---------- .../workflow/steps/RetryStep/config.jelly | 2 +- ...orConditions.html => help-conditions.html} | 0 .../workflow/steps/RetryExecutorStepTest.java | 8 +++---- 5 files changed, 22 insertions(+), 22 deletions(-) rename src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/{help-errorConditions.html => help-conditions.html} (100%) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java index 3d8e581e..1fa1ede4 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStep.java @@ -42,7 +42,7 @@ public class RetryStep extends Step { private final int count; - private List errorConditions; + private List conditions; @DataBoundConstructor public RetryStep(int count) { @@ -53,12 +53,12 @@ public int getCount() { return count; } - public List getErrorConditions() { - return errorConditions; + public List getConditions() { + return conditions; } - @DataBoundSetter public void setErrorConditions(List errorConditions) { - this.errorConditions = errorConditions; + @DataBoundSetter public void setConditions(List conditions) { + this.conditions = conditions; } @Override @@ -68,7 +68,7 @@ public DescriptorImpl getDescriptor() { @Override public StepExecution start(StepContext context) throws Exception { - return new RetryStepExecution(count, context, errorConditions); + return new RetryStepExecution(count, context, conditions); } @Extension diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java index f4172b5e..57924d69 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java @@ -19,19 +19,19 @@ public class RetryStepExecution extends AbstractStepExecutionImpl { private transient final int count; @SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Only used when starting.") - private transient final @CheckForNull List errorConditions; + private transient final @CheckForNull List conditions; - RetryStepExecution(int count, StepContext context, List errorConditions) { + RetryStepExecution(int count, StepContext context, List conditions) { super(context); this.count = count; - this.errorConditions = errorConditions; + this.conditions = conditions; } @Override public boolean start() throws Exception { StepContext context = getContext(); context.newBodyInvoker() - .withCallback(new Callback(count, errorConditions)) + .withCallback(new Callback(count, conditions)) .start(); return false; // execution is asynchronous } @@ -41,11 +41,11 @@ public boolean start() throws Exception { private static class Callback extends BodyExecutionCallback { private int left; - private final @CheckForNull List errorConditions; + private final @CheckForNull List conditions; - Callback(int count, List errorConditions) { + Callback(int count, List conditions) { left = count; - this.errorConditions = errorConditions; + this.conditions = conditions; } /* Could be added, but seems unnecessary, given the message already printed in onFailure: @@ -68,7 +68,7 @@ public void onFailure(StepContext context, Throwable t) { try { left--; TaskListener l = context.get(TaskListener.class); - if (left > 0 && matchesErrorConditions(t, context)) { + if (left > 0 && matchesConditions(t, context)) { if (t instanceof AbortException) { l.error(t.getMessage()); } else if (t instanceof FlowInterruptedException) { @@ -88,11 +88,11 @@ public void onFailure(StepContext context, Throwable t) { } } - private boolean matchesErrorConditions(Throwable t, StepContext context) throws IOException, InterruptedException { - if (errorConditions == null || errorConditions.isEmpty()) { + private boolean matchesConditions(Throwable t, StepContext context) throws IOException, InterruptedException { + if (conditions == null || conditions.isEmpty()) { return !(t instanceof FlowInterruptedException) || !((FlowInterruptedException) t).isActualInterruption(); } - for (ErrorCondition ec : errorConditions) { + for (ErrorCondition ec : conditions) { if (ec.test(t, context)) { return true; } diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly b/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly index 4fc01b78..b6ed0d9d 100644 --- a/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly +++ b/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/config.jelly @@ -28,7 +28,7 @@ THE SOFTWARE. - + diff --git a/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-errorConditions.html b/src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-conditions.html similarity index 100% rename from src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-errorConditions.html rename to src/main/resources/org/jenkinsci/plugins/workflow/steps/RetryStep/help-conditions.html diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java index 4b59fbcd..f28f89e5 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java @@ -74,7 +74,7 @@ public class RetryExecutorStepTest { s.setLabelString("dumb"); WorkflowJob p = r.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition( - "retry(count: 2, errorConditions: [custom()]) {\n" + + "retry(count: 2, conditions: [custom()]) {\n" + " node('dumb') {\n" + " sh 'sleep 10'\n" + " }\n" + @@ -101,7 +101,7 @@ public class RetryExecutorStepTest { s.setLabelString("dumb"); WorkflowJob p = r.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition( - "retry(count: 2, errorConditions: [custom()]) {\n" + + "retry(count: 2, conditions: [custom()]) {\n" + " node('dumb') {\n" + " hang()\n" + " }\n" + @@ -149,7 +149,7 @@ public static final class HangStep extends Step { s.setLabelString("dumb"); WorkflowJob p = r.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition( - "retry(count: 2, errorConditions: [custom()]) {\n" + + "retry(count: 2, conditions: [custom()]) {\n" + " node('dumb') {\n" + " semaphore 'wait'\n" + " isUnix()\n" + @@ -181,7 +181,7 @@ public static final class HangStep extends Step { s.setLabelString("dumb"); WorkflowJob p = r.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition( - "retry(count: 2, errorConditions: [custom()]) {\n" + + "retry(count: 2, conditions: [custom()]) {\n" + " node('dumb') {\n" + " waitWithoutAgent()\n" + " }\n" + From 19c2fdeab67597016b9cb960cda999839421d112 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 20 May 2022 11:51:54 -0400 Subject: [PATCH 8/9] Updating deps; PCT flagged a regression in `RetryExecutorStepTest.retryNodeBlockSynchAcrossRestarts` --- pom.xml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 9e3cb179..fdf9a093 100644 --- a/pom.xml +++ b/pom.xml @@ -97,7 +97,7 @@ org.jenkins-ci.plugins.workflow workflow-api - 1159.v27cb_4545c3ff + 1166.v459a_d69b_3271 org.jenkins-ci.plugins @@ -110,13 +110,13 @@ org.jenkins-ci.plugins.workflow workflow-cps - 2691.va_688a_c3d8fd0 + 2713.v8b_f3c8cb_97a_0 test org.jenkins-ci.plugins.workflow workflow-cps - 2691.va_688a_c3d8fd0 + 2713.v8b_f3c8cb_97a_0 tests test @@ -129,7 +129,7 @@ org.jenkins-ci.plugins.workflow workflow-durable-task-step - 1200.v7231de192754 + 1204.v2a_40b_1127a_a_8 org.jenkins-ci.plugins.workflow @@ -189,6 +189,7 @@ org.jenkins-ci.plugins script-security + 1172.v35f6a_0b_8207e test From 241f452744710b56c79a42b5d2a5f9588a1677be Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 20 May 2022 13:18:28 -0400 Subject: [PATCH 9/9] Make `WaitWithoutAgentStep` more accurately simulate a new JVM --- .../plugins/workflow/steps/RetryExecutorStepTest.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java index f28f89e5..0803b414 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/steps/RetryExecutorStepTest.java @@ -33,10 +33,12 @@ import java.util.HashSet; import java.util.Set; import java.util.logging.Level; +import jenkins.model.Jenkins; import org.jenkinsci.Symbol; import org.jenkinsci.plugins.durabletask.FileMonitoringTask; import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; import org.jenkinsci.plugins.workflow.flow.ErrorCondition; +import org.jenkinsci.plugins.workflow.flow.FlowExecutionList; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; import org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep; @@ -175,7 +177,7 @@ public static final class HangStep extends Step { @Issue({"JENKINS-49707", "JENKINS-30383"}) @Test public void retryNodeBlockSynchAcrossRestarts() throws Throwable { - logging.record(ExecutorStepExecution.class, Level.FINE); + logging.record(ExecutorStepExecution.class, Level.FINE).record(FlowExecutionList.class, Level.FINE); sessions.then(r -> { Slave s = inboundAgents.createAgent(r, "dumbo1"); s.setLabelString("dumb"); @@ -206,7 +208,14 @@ public static final class WaitWithoutAgentStep extends Step { @Override public StepExecution start(StepContext context) throws Exception { return StepExecutions.synchronousNonBlocking(context, c -> { c.get(TaskListener.class).getLogger().println("Sleeping without agent"); + Jenkins j = Jenkins.get(); Thread.sleep(10_000); + if (Jenkins.get() != j) { + // Finished sleeping in another session, which outside of JenkinsSessionRule is impossible. + // Avoid marking the step as completed since the Java objects here are stale. + // See https://github.com/jenkinsci/workflow-cps-plugin/pull/540. + Thread.sleep(Long.MAX_VALUE); + } return null; }); }