Simplify persistence design for temporarily offline status

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

@@ -179,11 +179,6 @@
 
     private long connectTime = 0;
 
-    /**
-     * True if Jenkins shouldn't start new builds on this node.
-     */
-    private boolean temporarilyOffline;
-
     /**
      * {@link Node} object may be created and deleted independently
      * from this object.
@@ -360,7 +355,8 @@
      */
     @Exported
     public OfflineCause getOfflineCause() {
-        return offlineCause;
+        var temporaryOfflineCause = getNodeOrDie().getTemporaryOfflineCause();
+        return temporaryOfflineCause == null ? offlineCause : temporaryOfflineCause;
     }
 
     @Override
@@ -549,7 +545,7 @@
     @Deprecated
     public void cliOffline(String cause) throws ExecutionException, InterruptedException {
         checkPermission(DISCONNECT);
-        setTemporarilyOffline(true, new ByCLI(cause));
+        setTemporarilyOfflineCause(new ByCLI(cause));
     }
 
     /**
@@ -558,7 +554,7 @@
     @Deprecated
     public void cliOnline() throws ExecutionException, InterruptedException {
         checkPermission(CONNECT);
-        setTemporarilyOffline(false, null);
+        setTemporarilyOfflineCause(null);
     }
 
     /**
@@ -606,6 +602,15 @@
         return j.getNode(nodeName);
     }
 
+    @NonNull
+    private Node getNodeOrDie() {
+        var node = nodeName == null ? Jenkins.get() : Jenkins.get().getNode(nodeName);
+        if (node == null) {
+            throw new IllegalStateException("Can't set a temporary offline cause if the node has been removed");
+        }
+        return node;
+    }
+
     @Exported
     public LoadStatistics getLoadStatistics() {
         return LabelAtom.get(nodeName != null ? nodeName : Jenkins.get().getSelfLabel().toString()).loadStatistics;
@@ -620,7 +625,7 @@
     @Exported
     @Override
     public boolean isOffline() {
-        return temporarilyOffline || getChannel() == null;
+        return getNodeOrDie().isTemporarilyOffline() || getChannel() == null;
     }
 
     public final boolean isOnline() {
@@ -669,43 +674,46 @@
     @Exported
     @Deprecated
     public boolean isTemporarilyOffline() {
-        return temporarilyOffline;
+        return getNodeOrDie().isTemporarilyOffline();
     }
 
     /**
      * @deprecated as of 1.320.
-     *      Use {@link #setTemporarilyOffline(boolean, OfflineCause)}
+     *      Use {@link #setTemporarilyOfflineCause(OfflineCause)}
      */
     @Deprecated
     public void setTemporarilyOffline(boolean temporarilyOffline) {
-        setTemporarilyOffline(temporarilyOffline, null);
+        setTemporarilyOfflineCause(temporarilyOffline ? new OfflineCause.LegacyOfflineCause() : null);
     }
 
     /**
-     * Marks the computer as temporarily offline. This retains the underlying
-     * {@link Channel} connection, but prevent builds from executing.
-     *
-     * @param cause
-     *      If the first argument is true, specify the reason why the node is being put
-     *      offline.
+     * @deprecated as of TODO.
+     *      Use {@link #setTemporarilyOfflineCause(OfflineCause)} instead.
      */
+    @Deprecated
     public void setTemporarilyOffline(boolean temporarilyOffline, OfflineCause cause) {
-        offlineCause = temporarilyOffline ? cause : null;
-        this.temporarilyOffline = temporarilyOffline;
-        Node node = getNode();
-        if (node != null) {
-            node.setTemporaryOfflineCause(offlineCause);
-        }
-        synchronized (statusChangeLock) {
-            statusChangeLock.notifyAll();
-        }
-        if (temporarilyOffline) {
-            Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOffline(this, cause));
+        if (cause == null) {
+            setTemporarilyOffline(temporarilyOffline);
         } else {
-            Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOnline(this));
+            setTemporarilyOfflineCause(cause);
         }
     }
 
+    /**
+     * Marks the computer as temporarily offline. This retains the underlying
+     * {@link Channel} connection, but prevent builds from executing.
+     *
+     * @param temporarilyOfflineCause The reason why the node is being put offline.
+     *                                If null, this cancels the status
+     */
+    public void setTemporarilyOfflineCause(@CheckForNull OfflineCause temporarilyOfflineCause) {
+        getNodeOrDie().setTemporaryOfflineCause(temporarilyOfflineCause);
+    }
+
+    public OfflineCause getTemporarilyOfflineCause() {
+        return getNodeOrDie().getTemporaryOfflineCause();
+    }
+
     @Exported
     @Override
     public String getIcon() {
@@ -785,16 +793,6 @@
             this.nodeName = null;
 
         setNumExecutors(node.getNumExecutors());
-        if (this.temporarilyOffline) {
-            // When we get a new node, push our current temp offline
-            // status to it (as the status is not carried across
-            // configuration changes that recreate the node).
-            // Since this is also called the very first time this
-            // Computer is created, avoid pushing an empty status
-            // as that could overwrite any status that the Node
-            // brought along from its persisted config data.
-            node.setTemporaryOfflineCause(this.offlineCause);
-        }
     }
 
     /**
@@ -1396,24 +1394,19 @@
 
     @RequirePOST
     public HttpResponse doToggleOffline(@QueryParameter String offlineMessage) throws IOException, ServletException {
-        if (!temporarilyOffline) {
-            checkPermission(DISCONNECT);
-            offlineMessage = Util.fixEmptyAndTrim(offlineMessage);
-            setTemporarilyOffline(!temporarilyOffline,
-                    new OfflineCause.UserCause(User.current(), offlineMessage));
-        } else {
+        if (getNodeOrDie().isTemporarilyOffline()) {
             checkPermission(CONNECT);
-            setTemporarilyOffline(!temporarilyOffline, null);
+            setTemporarilyOfflineCause(null);
+            return HttpResponses.redirectToDot();
+        } else {
+            return doChangeOfflineCause(offlineMessage);
         }
-        return HttpResponses.redirectToDot();
     }
 
     @RequirePOST
     public HttpResponse doChangeOfflineCause(@QueryParameter String offlineMessage) throws IOException, ServletException {
         checkPermission(DISCONNECT);
-        offlineMessage = Util.fixEmptyAndTrim(offlineMessage);
-        setTemporarilyOffline(true,
-                new OfflineCause.UserCause(User.current(), offlineMessage));
+        setTemporarilyOfflineCause(new OfflineCause.UserCause(User.current(), Util.fixEmptyAndTrim(offlineMessage)));
         return HttpResponses.redirectToDot();
     }
 

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

@@ -30,7 +30,6 @@
 import edu.umd.cs.findbugs.annotations.NonNull;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.BulkChange;
-import hudson.Extension;
 import hudson.ExtensionPoint;
 import hudson.FilePath;
 import hudson.FileSystemProvisioner;
@@ -69,6 +68,7 @@
 import java.util.logging.Logger;
 import jenkins.model.Jenkins;
 import jenkins.model.Nodes;
+import jenkins.util.Listeners;
 import jenkins.util.SystemProperties;
 import jenkins.util.io.OnMaster;
 import net.sf.json.JSONObject;
@@ -264,25 +264,11 @@ public void onLoad(Nodes parent, String name) {
         setNodeName(name);
     }
 
-    /**
-     * Let Nodes be aware of the lifecycle of their own {@link Computer}.
-     */
-    @Extension
-    public static class InternalComputerListener extends ComputerListener {
-        @Override
-        public void onOnline(Computer c, TaskListener listener) {
-            Node node = c.getNode();
-
-            // At startup, we need to restore any previously in-effect temp offline cause.
-            // We wait until the computer is started rather than getting the data to it sooner
-            // so that the normal computer start up processing works as expected.
-            if (node != null && node.temporaryOfflineCause != null && node.temporaryOfflineCause != c.getOfflineCause()) {
-                c.setTemporarilyOffline(true, node.temporaryOfflineCause);
-            }
-        }
+    boolean isTemporarilyOffline() {
+        return temporaryOfflineCause != null;
     }
 
-    private OfflineCause temporaryOfflineCause;
+    private volatile OfflineCause temporaryOfflineCause;
 
     /**
      * Enable a {@link Computer} to inform its node when it is taken
@@ -294,6 +280,11 @@ void setTemporaryOfflineCause(OfflineCause cause) {
                 temporaryOfflineCause = cause;
                 save();
             }
+            if (temporaryOfflineCause != null) {
+                Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOffline(toComputer(), temporaryOfflineCause));
+            } else {
+                Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOnline(toComputer()));
+            }
         } catch (java.io.IOException e) {
             LOGGER.warning("Unable to complete save, temporary offline status will not be persisted: " + e.getMessage());
         }

core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java

@@ -233,21 +233,21 @@
      */
     protected boolean markOnline(Computer c) {
         if (isIgnored() || c.isOnline()) return false; // noop
-        c.setTemporarilyOffline(false, null);
+        c.setTemporarilyOfflineCause(null);
         return true;
     }
 
     /**
      * Utility method to mark the computer offline for derived classes.
      *
      * @return true
      *      if the node was actually taken offline by this act (as opposed to us deciding not to do it,
      *      or the computer already marked offline.)
      */
     protected boolean markOffline(Computer c, OfflineCause oc) {
         if (isIgnored() || c.isTemporarilyOffline()) return false; // noop
 
-        c.setTemporarilyOffline(true, oc);
+        c.setTemporarilyOfflineCause(oc);
 
         // notify the admin
         MonitorMarkedNodeOffline no = AdministrativeMonitor.all().get(MonitorMarkedNodeOffline.class);

core/src/main/java/hudson/slaves/OfflineCause.java

@@ -33,6 +33,8 @@
 import java.util.Date;
 import jenkins.model.Jenkins;
 import org.jvnet.localizer.Localizable;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
 
@@ -71,6 +73,19 @@ public long getTimestamp() {
         return new Date(timestamp);
     }
 
+    /**
+     * @deprecated Only exists for backward compatibility.
+     * @see Computer#setTemporarilyOffline(boolean)
+     */
+    @Deprecated
+    @Restricted(NoExternalUse.class)
+    public static class LegacyOfflineCause extends OfflineCause {
+        @Exported(name = "description") @Override
+        public String toString() {
+            return "";
+        }
+    }
+
     /**
      * {@link OfflineCause} that renders a static text,
      * but without any further UI.

test/src/test/java/hudson/cli/OfflineNodeCommandTest.java

@@ -118,7 +118,7 @@ public void offlineNodeShouldSucceedOnOfflineNode() throws Exception {
         slave.toComputer().setTemporarilyOffline(true, null);
         assertThat(slave.toComputer().isOffline(), equalTo(true));
         assertThat(slave.toComputer().isTemporarilyOffline(), equalTo(true));
-        assertThat(slave.toComputer().getOfflineCause(), equalTo(null));
+        assertThat(slave.toComputer().getOfflineCause(), instanceOf(OfflineCause.LegacyOfflineCause.class));
 
         final CLICommandInvoker.Result result = command
                 .authorizedTo(Computer.DISCONNECT, Jenkins.READ)
@@ -177,7 +177,7 @@ public void offlineNodeShouldSucceedOnOfflineNodeWithCause() throws Exception {
         slave.toComputer().setTemporarilyOffline(true, null);
         assertThat(slave.toComputer().isOffline(), equalTo(true));
         assertThat(slave.toComputer().isTemporarilyOffline(), equalTo(true));
-        assertThat(slave.toComputer().getOfflineCause(), equalTo(null));
+        assertThat(slave.toComputer().getOfflineCause(), instanceOf(OfflineCause.LegacyOfflineCause.class));
 
         final CLICommandInvoker.Result result = command
                 .authorizedTo(Computer.DISCONNECT, Jenkins.READ)

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

@@ -102,7 +102,7 @@ public void testSetTemporaryOfflineCause() throws Exception {
         assertEquals("Node should have offline cause which was set.", cause, node.toComputer().getOfflineCause());
         OfflineCause cause2 = new OfflineCause.ByCLI("another message");
         node.setTemporaryOfflineCause(cause2);
-        assertEquals("Node should have original offline cause after setting another.", cause, node.toComputer().getOfflineCause());
+        assertEquals("Node should have the new offline cause.", cause2, node.toComputer().getOfflineCause());
     }
 
     @Test