From 8db0ae132ab832a036f6171d8b91f92e693c48d0 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 1 Dec 2021 15:43:32 -0500 Subject: [PATCH 1/6] Add ignored test where a PlaceholderTask is in the queue but the associated build is complete --- .../support/steps/ExecutorStepTest.java | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java index 3eae0921..6bd5173c 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java @@ -35,6 +35,7 @@ import hudson.model.FreeStyleProject; import hudson.model.Item; import hudson.model.Job; +import hudson.model.Label; import hudson.model.Node; import hudson.model.Queue; import hudson.model.Result; @@ -79,7 +80,9 @@ import hudson.util.VersionNumber; import java.nio.charset.StandardCharsets; +import java.nio.file.StandardCopyOption; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import jenkins.model.Jenkins; import jenkins.security.MasterToSlaveCallable; import jenkins.security.QueueItemAuthenticator; @@ -126,6 +129,7 @@ import static org.junit.Assert.fail; import org.junit.Assume; import org.junit.ClassRule; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -1264,6 +1268,44 @@ public void accessPermittedOnlyFromCurrentBuild() throws Throwable { r.buildAndAssertSuccess(main); }); } + + @Ignore + @Test public void placeholderTaskInQueueButAssociatedBuildComplete() throws Throwable { + AtomicReference rootDir = new AtomicReference<>(); + Path tempQueueFile = Files.createTempFile("queue", ".xml"); + sessions.then(r -> { + rootDir.set(r.jenkins.getRootDir()); + System.out.println(rootDir); + WorkflowJob p = r.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition("node('custom-label') { }", true)); + WorkflowRun b = p.scheduleBuild2(0).waitForStart(); + // Get into a state where a PlaceholderTask is in the queue. + while (true) { + Queue.Item[] items = Queue.getInstance().getItems(); + if (items.length == 1 && items[0].task instanceof ExecutorStepExecution.PlaceholderTask) { + break; + } + Thread.sleep(500L); + } + // Copy queue.xml to a temp file while the PlaceholderTask is in the queue. + r.jenkins.getQueue().save(); + Files.copy(rootDir.get().toPath().resolve("queue.xml"), tempQueueFile, StandardCopyOption.REPLACE_EXISTING); + // Create a node with the correct label and let the build complete. + DumbSlave node = r.createOnlineSlave(Label.get("custom-label")); + r.assertBuildStatusSuccess(r.waitForCompletion(b)); + }); + // Copy the temp queue.xml over the real one. The associated build has already completed, so the queue now + // has a bogus PlaceholderTask. + Files.copy(tempQueueFile, rootDir.get().toPath().resolve("queue.xml"), StandardCopyOption.REPLACE_EXISTING); + sessions.then(r -> { + WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class); + WorkflowRun b = p.getBuildByNumber(1); + assertFalse(b.isLogUpdated()); + r.assertBuildStatusSuccess(b); + assertThat(Queue.getInstance().getItems(), emptyArray()); // This assertion fails. + }); + } + public static final class WriteBackStep extends Step { static File controllerFile; static boolean legal = true; From d0632afb859ab9a67caf5b0864c08a46bc2b320f Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Wed, 1 Dec 2021 16:21:58 -0500 Subject: [PATCH 2/6] Update annotation to explain why test is ignored Co-authored-by: Jesse Glick --- .../plugins/workflow/support/steps/ExecutorStepTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java index 6bd5173c..202520a7 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java @@ -1269,7 +1269,7 @@ public void accessPermittedOnlyFromCurrentBuild() throws Throwable { }); } - @Ignore + @Ignore("TODO safe fix still TBD") @Test public void placeholderTaskInQueueButAssociatedBuildComplete() throws Throwable { AtomicReference rootDir = new AtomicReference<>(); Path tempQueueFile = Files.createTempFile("queue", ".xml"); From 3abef268b5050e6a95776c6293d8445a25c0e932 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Thu, 2 Dec 2021 14:49:20 -0500 Subject: [PATCH 3/6] Simplify test using JenkinsSessionRule.getHome and TemporaryFolder --- pom.xml | 1 + .../workflow/support/steps/ExecutorStepTest.java | 10 +++------- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/pom.xml b/pom.xml index ceec031c..7a8dbd6f 100644 --- a/pom.xml +++ b/pom.xml @@ -71,6 +71,7 @@ true jenkinsci/${project.artifactId}-plugin 2.40 + 1664.ve9ed23f5e0f2 diff --git a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java index 202520a7..3c607501 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java @@ -82,7 +82,6 @@ import java.nio.charset.StandardCharsets; import java.nio.file.StandardCopyOption; import java.util.Set; -import java.util.concurrent.atomic.AtomicReference; import jenkins.model.Jenkins; import jenkins.security.MasterToSlaveCallable; import jenkins.security.QueueItemAuthenticator; @@ -1271,11 +1270,8 @@ public void accessPermittedOnlyFromCurrentBuild() throws Throwable { @Ignore("TODO safe fix still TBD") @Test public void placeholderTaskInQueueButAssociatedBuildComplete() throws Throwable { - AtomicReference rootDir = new AtomicReference<>(); - Path tempQueueFile = Files.createTempFile("queue", ".xml"); + Path tempQueueFile = tmp.newFile().toPath(); sessions.then(r -> { - rootDir.set(r.jenkins.getRootDir()); - System.out.println(rootDir); WorkflowJob p = r.createProject(WorkflowJob.class, "p"); p.setDefinition(new CpsFlowDefinition("node('custom-label') { }", true)); WorkflowRun b = p.scheduleBuild2(0).waitForStart(); @@ -1289,14 +1285,14 @@ public void accessPermittedOnlyFromCurrentBuild() throws Throwable { } // Copy queue.xml to a temp file while the PlaceholderTask is in the queue. r.jenkins.getQueue().save(); - Files.copy(rootDir.get().toPath().resolve("queue.xml"), tempQueueFile, StandardCopyOption.REPLACE_EXISTING); + Files.copy(sessions.getHome().toPath().resolve("queue.xml"), tempQueueFile, StandardCopyOption.REPLACE_EXISTING); // Create a node with the correct label and let the build complete. DumbSlave node = r.createOnlineSlave(Label.get("custom-label")); r.assertBuildStatusSuccess(r.waitForCompletion(b)); }); // Copy the temp queue.xml over the real one. The associated build has already completed, so the queue now // has a bogus PlaceholderTask. - Files.copy(tempQueueFile, rootDir.get().toPath().resolve("queue.xml"), StandardCopyOption.REPLACE_EXISTING); + Files.copy(tempQueueFile, sessions.getHome().toPath().resolve("queue.xml"), StandardCopyOption.REPLACE_EXISTING); sessions.then(r -> { WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class); WorkflowRun b = p.getBuildByNumber(1); From 00cdd69026b52465b66afa687414573d7c10fafe Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Fri, 3 Dec 2021 15:50:19 -0500 Subject: [PATCH 4/6] De-flake test and update to released version of jenkins-test-harness --- pom.xml | 2 +- .../plugins/workflow/support/steps/ExecutorStepTest.java | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 7a8dbd6f..a8a8ba92 100644 --- a/pom.xml +++ b/pom.xml @@ -71,7 +71,7 @@ true jenkinsci/${project.artifactId}-plugin 2.40 - 1664.ve9ed23f5e0f2 + 1666.vd1360abbfe9e diff --git a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java index 3c607501..25f22244 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java @@ -1289,6 +1289,8 @@ public void accessPermittedOnlyFromCurrentBuild() throws Throwable { // Create a node with the correct label and let the build complete. DumbSlave node = r.createOnlineSlave(Label.get("custom-label")); r.assertBuildStatusSuccess(r.waitForCompletion(b)); + // Remove node so that tasks requiring custom-label are stuck in the queue. + Jenkins.get().removeNode(node); }); // Copy the temp queue.xml over the real one. The associated build has already completed, so the queue now // has a bogus PlaceholderTask. From 9d8eeca06ba0eab24dea6ff8990dcc7ef0770c3a Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Fri, 3 Dec 2021 15:51:54 -0500 Subject: [PATCH 5/6] Check if build is complete in PlaceholderTask.getCauseOfBlockage and if so, block execution and cancel task --- .../support/steps/ExecutorStepExecution.java | 16 ++++++++++++++++ .../workflow/support/steps/ExecutorStepTest.java | 8 +++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java index c81bf0c1..d3d818fe 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java @@ -422,6 +422,22 @@ public String getCookie() { } @Override public CauseOfBlockage getCauseOfBlockage() { + Run run = runForDisplay(); + if (!stopping && run != null && !run.isLogUpdated()) { + stopping = true; + LOGGER.warning(() -> "Refusing to build " + this + " and cancelling it because associated build is complete"); + Timer.get().execute(() -> { + Queue.getInstance().cancel(this); + }); + } + if (stopping) { + return new CauseOfBlockage() { + @Override + public String getShortDescription() { + return "Stopping " + getDisplayName(); + } + }; + } return null; } diff --git a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java index 25f22244..e7269c84 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java @@ -128,7 +128,6 @@ import static org.junit.Assert.fail; import org.junit.Assume; import org.junit.ClassRule; -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -1268,8 +1267,8 @@ public void accessPermittedOnlyFromCurrentBuild() throws Throwable { }); } - @Ignore("TODO safe fix still TBD") @Test public void placeholderTaskInQueueButAssociatedBuildComplete() throws Throwable { + logging.record(ExecutorStepExecution.class, Level.FINE).capture(50); Path tempQueueFile = tmp.newFile().toPath(); sessions.then(r -> { WorkflowJob p = r.createProject(WorkflowJob.class, "p"); @@ -1300,7 +1299,10 @@ public void accessPermittedOnlyFromCurrentBuild() throws Throwable { WorkflowRun b = p.getBuildByNumber(1); assertFalse(b.isLogUpdated()); r.assertBuildStatusSuccess(b); - assertThat(Queue.getInstance().getItems(), emptyArray()); // This assertion fails. + while (Queue.getInstance().getItems().length > 0) { + Thread.sleep(100L); + } + assertThat(logging.getMessages(), hasItem(startsWith("Refusing to build ExecutorStepExecution.PlaceholderTask{runId=p#"))); }); } From e0b5abf0ca7ff6fb676849b88a9af4b4b933f702 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Fri, 3 Dec 2021 16:58:24 -0500 Subject: [PATCH 6/6] Simplify lambda in PlaceholderTask.getCauseOfBlockage Co-authored-by: Jesse Glick --- .../plugins/workflow/support/steps/ExecutorStepExecution.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java index d3d818fe..77fb8e6f 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java @@ -426,9 +426,7 @@ public String getCookie() { if (!stopping && run != null && !run.isLogUpdated()) { stopping = true; LOGGER.warning(() -> "Refusing to build " + this + " and cancelling it because associated build is complete"); - Timer.get().execute(() -> { - Queue.getInstance().cancel(this); - }); + Timer.get().execute(() -> Queue.getInstance().cancel(this)); } if (stopping) { return new CauseOfBlockage() {