Fix Jenkins#updateNode to call NodeListener#onUpdated (#10397)

Author: krisstern

core/src/main/java/hudson/model/Node.java

@@ -138,6 +138,7 @@ public boolean isHoldOffLaunchUntilSave() {
     }
 
     /**
+     * In most cases, you should not call this method directly, but {@link Jenkins#updateNode(Node)} instead.
      * @since 1.635.
      */
     @Override
@@ -281,7 +282,7 @@ void setTemporaryOfflineCause(OfflineCause cause) {
         try {
             if (temporaryOfflineCause != cause) {
                 temporaryOfflineCause = cause;
-                save();
+                Jenkins.get().updateNode(this);
             }
             if (temporaryOfflineCause != null) {
                 Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOffline(toComputer(), temporaryOfflineCause));

core/src/main/java/hudson/model/Slave.java

@@ -248,6 +248,15 @@ public ComputerLauncher getLauncher() {
         return launcher == null ? new JNLPLauncher() : launcher;
     }
 
+    /**
+     * @deprecated In most cases, you should not call this method directly, but {@link Jenkins#updateNode(Node)} instead.
+     */
+    @Override
+    @Deprecated
+    public void save() throws IOException {
+        super.save();
+    }
+
     public void setLauncher(ComputerLauncher launcher) {
         this.launcher = launcher;
     }

core/src/main/java/jenkins/model/Nodes.java

@@ -47,7 +47,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
-import java.util.concurrent.Callable;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ConcurrentSkipListMap;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -203,17 +202,18 @@ public XmlFile getConfigFile(String nodeName) {
      * @since 1.634
      */
     public boolean updateNode(final @NonNull Node node) throws IOException {
+        return updateNode(node, true);
+    }
+
+    private boolean updateNode(final @NonNull Node node, boolean fireListener) throws IOException {
         boolean exists;
         try {
-            exists = Queue.withLock(new Callable<>() {
-                @Override
-                public Boolean call() throws Exception {
-                    if (node == nodes.get(node.getNodeName())) {
-                        jenkins.trimLabels(node);
-                        return true;
-                    }
-                    return false;
+            exists = Queue.withLock(() -> {
+                if (node == nodes.get(node.getNodeName())) {
+                    jenkins.trimLabels(node);
+                    return true;
                 }
+                return false;
             });
         } catch (RuntimeException e) {
             // should never happen, but if it does let's do the right thing
@@ -225,7 +225,9 @@ public Boolean call() throws Exception {
         if (exists) {
             // TODO there is a theoretical race whereby the node instance is updated/removed after lock release
             node.save();
-            // TODO should this fireOnUpdated?
+            if (fireListener) {
+                NodeListener.fireOnUpdated(node, node);
+            }
             return true;
         }
         return false;
@@ -252,7 +254,7 @@ public void run() {
                     newOne.onLoad(Nodes.this, newOne.getNodeName());
                 }
             });
-            updateNode(newOne);
+            updateNode(newOne, false);
             if (!newOne.getNodeName().equals(oldOne.getNodeName())) {
                 LOGGER.fine(() -> "deleting " + new File(getRootDir(), oldOne.getNodeName()));
                 Util.deleteRecursive(new File(getRootDir(), oldOne.getNodeName()));

test/src/test/java/hudson/model/NodeTest.java

@@ -39,6 +39,7 @@
 
 import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.EnvVars;
+import hudson.ExtensionList;
 import hudson.FilePath;
 import hudson.model.Node.Mode;
 import hudson.model.Queue.WaitingItem;
@@ -61,6 +62,7 @@
 import java.util.GregorianCalendar;
 import java.util.List;
 import jenkins.model.Jenkins;
+import jenkins.model.NodeListener;
 import jenkins.security.QueueItemAuthenticatorConfiguration;
 import org.htmlunit.HttpMethod;
 import org.htmlunit.Page;
@@ -91,23 +93,40 @@ public void before() {
         j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
     }
 
+    @TestExtension("testSetTemporaryOfflineCause")
+    public static class NodeListenerImpl extends NodeListener {
+        private int count;
+
+        public static int getCount() {
+            return ExtensionList.lookupSingleton(NodeListenerImpl.class).count;
+        }
+
+        @Override
+        protected void onUpdated(@NonNull Node oldOne, @NonNull Node newOne) {
+            count++;
+        }
+    }
+
     @Test
     public void testSetTemporaryOfflineCause() throws Exception {
         Node node = j.createOnlineSlave();
         FreeStyleProject project = j.createFreeStyleProject();
         project.setAssignedLabel(j.jenkins.getLabel(node.getDisplayName()));
         OfflineCause cause = new OfflineCause.ByCLI("message");
         node.setTemporaryOfflineCause(cause);
+        assertThat(NodeListenerImpl.getCount(), is(1));
         for (ComputerListener l : ComputerListener.all()) {
             l.onOnline(node.toComputer(), TaskListener.NULL);
         }
         assertEquals("Node should have offline cause which was set.", cause, node.toComputer().getOfflineCause());
         OfflineCause cause2 = new OfflineCause.ByCLI("another message");
         node.setTemporaryOfflineCause(cause2);
+        assertThat(NodeListenerImpl.getCount(), is(2));
         assertEquals("Node should have the new offline cause.", cause2, node.toComputer().getOfflineCause());
         // Exists in some plugins
         node.toComputer().setTemporarilyOffline(false, new OfflineCause.ByCLI("A third message"));
         assertThat(node.getTemporaryOfflineCause(), nullValue());
+        assertThat(NodeListenerImpl.getCount(), is(3));
     }
 
     @Test

test/src/test/java/jenkins/model/NodeListenerTest.java

@@ -49,6 +49,14 @@ public void crud() throws Exception {
         verifyNoMoreInteractions(mock);
     }
 
+    @Test
+    public void updateNode() throws Exception {
+        Node agent = j.createSlave();
+        agent.setLabelString("some label");
+        Jenkins.get().updateNode(agent);
+        verify(mock, times(1)).onUpdated(any(Node.class), any(Node.class));
+    }
+
     private CLICommandInvoker cli(CLICommand cmd) {
         return new CLICommandInvoker(j, cmd);
     }