When calling Nodes#setNodes, NodeListener methods should be called as…

… required For example, when loading a new casc configuration, it calls Nodes#setNodes, and the NodeListener methods were not called, causing listener implementations not to be notified according to the new state.
jenkinsci · Oct 23, 2024 · 36524d0 · 36524d0
1 parent d9fbb65
commit 36524d0
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 16 deletions.
diff --git a/core/src/main/java/jenkins/model/Nodes.java b/core/src/main/java/jenkins/model/Nodes.java
@@ -54,6 +54,7 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 import jenkins.util.SystemProperties;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -114,26 +115,32 @@ public List<Node> getNodes() {
      * @throws IOException if the new list of nodes could not be persisted.
      */
     public void setNodes(final @NonNull Collection<? extends Node> nodes) throws IOException {
-        Set<String> toRemove = new HashSet<>();
-        Queue.withLock(new Runnable() {
-            @Override
-            public void run() {
-                toRemove.addAll(Nodes.this.nodes.keySet());
-                for (Node n : nodes) {
-                    final String name = n.getNodeName();
-                    toRemove.remove(name);
-                    Nodes.this.nodes.put(name, n);
-                    n.onLoad(Nodes.this, name);
+        Set<Node> toRemove = new HashSet<>();
+        Queue.withLock(() -> {
+            toRemove.addAll(Nodes.this.nodes.values());
+            for (var node : nodes) {
+                final var name = node.getNodeName();
+                var existingNode = toRemove.stream().filter(n -> n.getNodeName().equals(name)).findFirst();
+                if (existingNode.isPresent()) {
+                    var oldNode = existingNode.get();
+                    NodeListener.fireOnUpdated(oldNode, node);
+                    toRemove.remove(oldNode);
+                } else {
+                    NodeListener.fireOnCreated(node);
                 }
-                Nodes.this.nodes.keySet().removeAll(toRemove);
-                jenkins.updateComputerList();
-                jenkins.trimLabels();
+                Nodes.this.nodes.put(name, node);
+                node.onLoad(Nodes.this, name);
             }
+            Nodes.this.nodes.keySet().removeAll(toRemove.stream().map(Node::getNodeName).collect(Collectors.toSet()));
+            jenkins.updateComputerList();
+            jenkins.trimLabels();
         });
         save();
-        for (String name : toRemove) {
-            LOGGER.fine(() -> "deleting " + new File(getRootDir(), name));
-            Util.deleteRecursive(new File(getRootDir(), name));
+        for (var deletedNode : toRemove) {
+            NodeListener.fireOnDeleted(deletedNode);
+            var nodeName = deletedNode.getNodeName();
+            LOGGER.fine(() -> "deleting " + new File(getRootDir(), nodeName));
+            Util.deleteRecursive(new File(getRootDir(), nodeName));
         }
     }
 

diff --git a/test/src/test/java/jenkins/model/NodesTest.java b/test/src/test/java/jenkins/model/NodesTest.java
@@ -25,6 +25,8 @@
 package jenkins.model;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.nullValue;
@@ -47,6 +49,9 @@
 import hudson.slaves.ComputerLauncher;
 import hudson.slaves.DumbSlave;
 import java.io.IOException;
+import java.net.URISyntaxException;
+import java.util.ArrayList;
+import java.util.List;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -239,4 +244,44 @@ protected boolean allowLoad(@NonNull Node node) {
         }
     }
 
+    @Test
+    public void listenersCalledOnSetNodes() throws URISyntaxException, IOException, Descriptor.FormException {
+        var agentA = new DumbSlave("nodeA", "temp", r.createComputerLauncher(null));
+        var agentB = new DumbSlave("nodeB", "temp", r.createComputerLauncher(null));
+        var agentA2 = new DumbSlave("nodeA", "temp2", r.createComputerLauncher(null));
+        Jenkins.get().setNodes(List.of(agentA, agentB));
+        assertThat(CheckSetNodes.created, containsInAnyOrder(agentA.toString(), agentB.toString()));
+        Jenkins.get().setNodes(List.of(agentA2));
+        assertThat(CheckSetNodes.updated, contains(new NodePair(agentA.toString(), agentA2.toString())));
+        assertThat(CheckSetNodes.deleted, contains(agentB.toString()));
+        Jenkins.get().setNodes(List.of());
+        assertThat(CheckSetNodes.deleted, containsInAnyOrder(agentA2.toString(), agentB.toString()));
+    }
+
+    private record NodePair(String oldNode, String newNode) {
+
+    }
+
+    @TestExtension("listenersCalledOnSetNodes")
+    public static class CheckSetNodes extends NodeListener {
+        private static final List<String> created = new ArrayList<>();
+        private static final List<NodePair> updated = new ArrayList<>();
+        private static final List<String> deleted = new ArrayList<>();
+
+        @Override
+        protected void onCreated(@NonNull Node node) {
+            created.add(node.toString());
+        }
+
+        @Override
+        protected void onUpdated(@NonNull Node oldOne, @NonNull Node newOne) {
+            updated.add(new NodePair(oldOne.toString(), newOne.toString()));
+        }
+
+        @Override
+        protected void onDeleted(@NonNull Node node) {
+            deleted.add(node.toString());
+        }
+    }
+
 }