From 3df6ccaec8ba60b144d60a5302233e149507712b Mon Sep 17 00:00:00 2001
From: Anish Dangi <Anish.Dangi@wdc.com>
Date: Thu, 7 Mar 2019 15:52:36 -0800
Subject: [PATCH] [JENKINS-56403]: Renaming agents doesn't remove old agent
 configuration from disk

[JENKINS-33780] introduced a regression in persisting the update to disk.
In prior code, the update was performed through a full config save after
updating the nodes, but the functionality was not carried over.

* Added a file delete call to remove the old agent from disk iff
  the node name was changed.
* Add tests for this scenario and when node name was not updated.
---
 core/src/main/java/jenkins/model/Nodes.java   |  4 ++++
 .../test/java/jenkins/model/NodesTest.java    | 24 +++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/core/src/main/java/jenkins/model/Nodes.java b/core/src/main/java/jenkins/model/Nodes.java
index 24f9ac7431c0..bf10f50a49c1 100644
--- a/core/src/main/java/jenkins/model/Nodes.java
+++ b/core/src/main/java/jenkins/model/Nodes.java
@@ -233,7 +233,11 @@ public void run() {
                 }
             });
             updateNode(newOne);
+            if (!newOne.getNodeName().equals(oldOne.getNodeName())) {
+                Util.deleteRecursive(new File(getNodesDir(), oldOne.getNodeName()));
+            }
             NodeListener.fireOnUpdated(oldOne, newOne);
+
             return true;
         } else {
             return false;
diff --git a/test/src/test/java/jenkins/model/NodesTest.java b/test/src/test/java/jenkins/model/NodesTest.java
index 8e81a98a433b..966fe8331361 100644
--- a/test/src/test/java/jenkins/model/NodesTest.java
+++ b/test/src/test/java/jenkins/model/NodesTest.java
@@ -38,6 +38,8 @@
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.fail;
 
 public class NodesTest {
@@ -88,6 +90,28 @@ public void addNodeShouldReplaceExistingNode() throws Exception {
         assertThat(r.jenkins.getNode("foo"), sameInstance(newNode));
     }
 
+    @Test
+    @Issue("JENKINS-56403")
+    public void replaceNodeShouldRemoveOldNode() throws Exception {
+        Node oldNode = r.createSlave("foo", "", null);
+        Node newNode = r.createSlave("foo-new", "", null);
+        r.jenkins.addNode(oldNode);
+        r.jenkins.getNodesObject().replaceNode(oldNode, newNode);
+        r.jenkins.getNodesObject().load();
+        assertNull(r.jenkins.getNode(oldNode.getNodeName()));
+    }
+
+    @Test
+    @Issue("JENKINS-56403")
+    public void replaceNodeShouldNotRemoveIdenticalOldNode() throws Exception {
+        Node oldNode = r.createSlave("foo", "", null);
+        Node newNode = r.createSlave("foo", "", null);
+        r.jenkins.addNode(oldNode);
+        r.jenkins.getNodesObject().replaceNode(oldNode, newNode);
+        r.jenkins.getNodesObject().load();
+        assertNotNull(r.jenkins.getNode(oldNode.getNodeName()));
+    }
+
     private static class InvalidNode extends Slave {
         // JEP-200 whitelist changes prevent this field (and thus instances of this class) from being serialized.
         private ClassLoader cl = InvalidNode.class.getClassLoader();