[JENKINS-49707] (part): Fail the build if an agent has been removed

[jglick]

Improved behavior for one subcase of JENKINS-49707, that a disconnected agent is actually removed from Jenkins configuration for whatever reason. We assume that it is not going to be readded (under the same name), so there is no point in waiting for it to reconnect. The one exception that I know of is that a Swarm agent will remove itself upon disconnect but might readd itself later, so this might break cross-disconnect durability of Pipeline builds for Swarm, but that seems like a far less likely case than typical Clouds.

#47 and #48 do something similar across Jenkins restarts. I think the TIMEOUT_WAITING_FOR_NODE_MILLIS grace period there is irrelevant here, since that would be done before the program even resumes.

• decide about FAILURE vs. ABORTED status
• test on EC2 with spot instances did not manage to set up the ec2 plugin successfully
• test against kubernetes plugin with a killed pod
• consider implementing instead in ExecutorStepExecution (would work not just for DurableTaskStep)

@schottsfired

[basil]

this might break cross-disconnect durability of Pipeline builds for Swarm, but that seems like a far less likely case than typical Clouds

I rely on cross-disconnect durability of Pipeline builds for Swarm across my entire Jenkins fleet, so this would be a serious regression from my point of view. Would you be willing to run PipelineJobTest#buildShellScriptAfterRestart from jenkinsci/swarm-plugin#81 with these changes to ensure this workflow keeps working?

[jglick]

Would you be willing to run PipelineJobTest#buildShellScriptAfterRestart from jenkinsci/swarm-plugin#81 with these changes

Sure, though I doubt that would reproduce any problem anyway, as this patch is about the case that a client is disconnected and removed while the Jenkins master is still running. To test this patch you would need to kill the client and then have it restart, in a single “story”. Reconnection of a Swarm client across restarts is what #47/#48 should have been affecting; in that case there was a grace period, needed since SwarmSlave is an EphemeralNode and so we actually have to wait for it to be reattached.

---

@basil this code looks wrong to me. It could all be replaced by simply

jenkins.addNode(slave);

since that method already replaces any existing node of the same name, without firing the deleted event. (Currently it always fires the created event, even when it ought to be firing updated; if I get a moment I will try to patch that.)

[jglick]

Currently it always fires the created event, even when it ought to be firing updated

jenkinsci/jenkins#4004

[basil]

Sure, though I doubt that would reproduce any problem anyway, as this patch is about the case that a client is disconnected and removed while the Jenkins master is still running. To test this patch you would need to kill the client and then have it restart, in a single “story”.

My latest update to jenkinsci/swarm-plugin#81 adds a new test for this case in PipelineJobTest#buildShellScriptAfterDisconnect.

[basil]

@basil this code looks wrong to me. It could all be replaced by simply

jenkins.addNode(slave);

since that method already replaces any existing node of the same name, without firing the deleted event.

That change makes sense to me and my new integration tests still pass with it, so I opened jenkinsci/swarm-plugin#82.

[jglick]

Even with jenkinsci/swarm-plugin#82, buildShellScriptAfterDisconnect will fail due to the removeNode call from SwarmSlave.SELF_CLEANUP_LAUNCHER.afterDisconnect. I will check if a grace period helps.

[jglick]

hudson.plugins.swarm.PipelineJobTest succeeds after

diff --git a/plugin/pom.xml b/plugin/pom.xml
index bfda46a..1a913a9 100644
--- a/plugin/pom.xml
+++ b/plugin/pom.xml
@@ -77,7 +77,7 @@
       </plugins>
     </build>
     <properties>
-        <workflow-support-plugin.version>2.13</workflow-support-plugin.version>
+        <workflow-support-plugin.version>3.3</workflow-support-plugin.version>
     </properties>
     <dependencyManagement>
         <dependencies>
@@ -90,19 +90,19 @@
             <dependency>
                 <groupId>org.jenkins-ci.plugins</groupId>
                 <artifactId>scm-api</artifactId>
-                <version>1.3</version>
+                <version>2.2.6</version>
                 <scope>test</scope>
             </dependency>
             <dependency>
                 <groupId>org.jenkins-ci.plugins</groupId>
                 <artifactId>structs</artifactId>
-                <version>1.5</version>
+                <version>1.18</version>
                 <scope>test</scope>
             </dependency>
             <dependency>
                 <groupId>org.jenkins-ci.plugins</groupId>
                 <artifactId>script-security</artifactId>
-                <version>1.24</version>
+                <version>1.58</version>
                 <scope>test</scope>
             </dependency>
         </dependencies>
@@ -111,7 +111,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-api</artifactId>
-            <version>2.12</version>
+            <version>2.33</version>
             <scope>test</scope>
         </dependency>
         <dependency>
@@ -123,13 +123,13 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-cps</artifactId>
-            <version>2.28</version>
+            <version>2.70</version>
             <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-durable-task-step</artifactId>
-            <version>2.4</version>
+            <version>2.32-rc917.b70f8edaf896</version> <!-- https://github.com/jenkinsci/workflow-durable-task-step-plugin/pull/104 -->
             <scope>test</scope>
         </dependency>
         <dependency>
@@ -141,7 +141,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-step-api</artifactId>
-            <version>2.7</version>
+            <version>2.20</version>
             <scope>test</scope>
         </dependency>
         <dependency>
diff --git a/pom.xml b/pom.xml
index d5fbe91..4ce3480 100644
--- a/pom.xml
+++ b/pom.xml
@@ -24,7 +24,7 @@
     <version>3.18-SNAPSHOT</version>
 
     <properties>
-        <jenkins.version>2.60.3</jenkins.version>
+        <jenkins.version>2.150.1</jenkins.version>
         <java.level>8</java.level>
     </properties>
 

[dwnusbaum]

Looks good in general, added some comments/questions.

[dwnusbaum]

Is ABORTED really the result we want in this case? My initial thought was that FAILURE would be more appropriate. Thoughts?

Looks like at least one reason for ABORTED is that over in RemovedNodeListener you call BodyExecution.cancel, and CpsBodyExecution always throws a FlowInterruptedException with result ABORTED in that method, so it makes sense to be consistent.

[jglick]

Generally FAILURE is used for cases where something about the build itself broke. The typical scenario here is that the build started running, something like a spot instance got killed, and so the build could not complete due to external causes. Prior to this patch (plus downstream where appropriate) the build would just hang until manually ABORTED; now the abort is automatic.

[dwnusbaum]

Is that really the case here? It looks like we only check the Node, not the Computer.

[jglick]

FilePathUtils.find above already checked nullness of Jenkins.getComputer.

[dwnusbaum]

So IIUC, if you restart Jenkins while a Pipeline is executing inside of a node block, and then remove the Node from Jenkins, that's how you get to the "Agent foo was deleted, but do not have a node body to cancel" branch in RemovedNodeListener? Or session here refers to something else?

[jglick]

Your understanding is correct. The patch to DurableTaskStep should still suffice to abort the build if it was inside sh when the node was removed.

If you remove the node during the restart, a different bit of code in ExecutorPickle (existing long before this patch) kicks in and aborts the build before it even resumes the CPS VM thread.

[slaughter550]

@jglick Quick question, we came across this and I was wondering how to set org.jenkinsci.plugins.workflow.support.pickles.ExecutorPickle.timeOutForNodeMillis correctly? We tried setting it on jenkins master with an arg on Jenkins startup with -D but it doesn't appear to be inheriting correctly. I'm new to Jenkins so apologies if it's a stupid question. Thanks!

[jglick]

-D is used for this sort of thing. Maybe try the users’ mailing list?

[slaughter550]

For anyone who finds this later, its org.jenkinsci.plugins.workflow.support.pickles.ExecutorPickle.timeoutForNodeMillis where timeout is all lower case. The changelog here is incorrect

[jglick]

@slaughter550 you can file a PR for https://github.com/jenkinsci/workflow-durable-task-step-plugin/blob/master/CHANGELOG.md#232 as needed though I see no mention of the property to be misspelled.

[slaughter550]

@jglick PR submitted, it's not misspelled, its incorrectly capitalized. #142