[JENKINS-30101][JENKINS-30175] Simplify persistence design for tempor…

…arily offline status (#9855)

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

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

@@ -180,11 +180,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.
@@ -361,6 +356,13 @@ public AnnotatedLargeText<Computer> getLogText() {
      */
     @Exported
     public OfflineCause getOfflineCause() {
+        var node = getNode();
+        if (node != null) {
+            var temporaryOfflineCause = node.getTemporaryOfflineCause();
+            if (temporaryOfflineCause != null) {
+                return temporaryOfflineCause;
+            }
+        }
         return offlineCause;
     }
 
@@ -372,6 +374,7 @@ public boolean hasOfflineCause() {
     @Exported
     @Override
     public String getOfflineCauseReason() {
+        var offlineCause = getOfflineCause();
         if (offlineCause == null) {
             return "";
         }
@@ -550,7 +553,7 @@ public void cliDisconnect(String cause) throws ExecutionException, InterruptedEx
     @Deprecated
     public void cliOffline(String cause) throws ExecutionException, InterruptedException {
         checkPermission(DISCONNECT);
-        setTemporarilyOffline(true, new ByCLI(cause));
+        setTemporaryOfflineCause(new ByCLI(cause));
     }
 
     /**
@@ -559,7 +562,7 @@ public void cliOffline(String cause) throws ExecutionException, InterruptedExcep
     @Deprecated
     public void cliOnline() throws ExecutionException, InterruptedException {
         checkPermission(CONNECT);
-        setTemporarilyOffline(false, null);
+        setTemporaryOfflineCause(null);
     }
 
     /**
@@ -621,7 +624,7 @@ public BuildTimelineWidget getTimeline() {
     @Exported
     @Override
     public boolean isOffline() {
-        return temporarilyOffline || getChannel() == null;
+        return isTemporarilyOffline() || getChannel() == null;
     }
 
     public final boolean isOnline() {
@@ -670,41 +673,64 @@ public boolean isLaunchSupported() {
     @Exported
     @Deprecated
     public boolean isTemporarilyOffline() {
-        return temporarilyOffline;
+        var node = getNode();
+        return node != null && node.isTemporarilyOffline();
     }
 
     /**
      * @deprecated as of 1.320.
-     *      Use {@link #setTemporarilyOffline(boolean, OfflineCause)}
+     *      Use {@link #setTemporaryOfflineCause(OfflineCause)}
      */
     @Deprecated
     public void setTemporarilyOffline(boolean temporarilyOffline) {
-        setTemporarilyOffline(temporarilyOffline, null);
+        setTemporaryOfflineCause(temporarilyOffline ? new OfflineCause.LegacyOfflineCause() : null);
+    }
+
+    /**
+     * @deprecated
+     *      Use {@link #setTemporaryOfflineCause(OfflineCause)} instead.
+     */
+    @Deprecated(since = "TODO")
+    public void setTemporarilyOffline(boolean temporarilyOffline, OfflineCause cause) {
+        if (cause == null) {
+            setTemporarilyOffline(temporarilyOffline);
+        } else {
+            setTemporaryOfflineCause(temporarilyOffline ? cause : 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.
+     * @param temporaryOfflineCause The reason why the node is being put offline.
+     *                                If null, this cancels the status
+     * @since TODO
      */
-    public void setTemporarilyOffline(boolean temporarilyOffline, OfflineCause cause) {
-        offlineCause = temporarilyOffline ? cause : null;
-        this.temporarilyOffline = temporarilyOffline;
-        Node node = getNode();
-        if (node != null) {
-            node.setTemporaryOfflineCause(offlineCause);
+    public void setTemporaryOfflineCause(@CheckForNull OfflineCause temporaryOfflineCause) {
+        var node = getNode();
+        if (node == null) {
+            throw new IllegalStateException("Can't set a temporary offline cause if the node has been removed");
         }
-        synchronized (statusChangeLock) {
-            statusChangeLock.notifyAll();
+        node.setTemporaryOfflineCause(temporaryOfflineCause);
+    }
+
+    /**
+     * @since TODO
+     * @return If the node is temporarily offline, the reason why.
+     */
+    @SuppressWarnings("unused") // used by setOfflineCause.jelly
+    public String getTemporaryOfflineCauseReason() {
+        var node = getNode();
+        if (node == null) {
+            // Node was deleted; computer still exists
+            return null;
         }
-        if (temporarilyOffline) {
-            Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOffline(this, cause));
-        } else {
-            Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOnline(this));
+        var cause = node.getTemporaryOfflineCause();
+        if (cause instanceof OfflineCause.UserCause userCause) {
+            return userCause.getMessage();
         }
+        return cause != null ? cause.toString() : "";
     }
 
     @Exported
@@ -786,16 +812,6 @@ protected void setNode(Node node) {
             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);
-        }
     }
 
     /**
@@ -1397,24 +1413,23 @@ public void doRssLatest(StaplerRequest2 req, StaplerResponse2 rsp) throws IOExce
 
     @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 {
+        var node = getNode();
+        if (node == null) {
+            return HttpResponses.notFound();
+        }
+        if (node.isTemporarilyOffline()) {
             checkPermission(CONNECT);
-            setTemporarilyOffline(!temporarilyOffline, null);
+            setTemporaryOfflineCause(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));
+        setTemporaryOfflineCause(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;
@@ -265,24 +265,13 @@ public void onLoad(Nodes parent, String name) {
     }
 
     /**
-     * Let Nodes be aware of the lifecycle of their own {@link Computer}.
+     * @return true if this node has a temporary offline cause set.
      */
-    @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 +283,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,7 +233,7 @@ public boolean isIgnored() {
      */
     protected boolean markOnline(Computer c) {
         if (isIgnored() || c.isOnline()) return false; // noop
-        c.setTemporarilyOffline(false, null);
+        c.setTemporaryOfflineCause(null);
         return true;
     }
 
@@ -247,7 +247,7 @@ protected boolean markOnline(Computer c) {
     protected boolean markOffline(Computer c, OfflineCause oc) {
         if (isIgnored() || c.isTemporarilyOffline()) return false; // noop
 
-        c.setTemporarilyOffline(true, oc);
+        c.setTemporaryOfflineCause(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.
@@ -136,15 +151,15 @@ public static class UserCause extends SimpleOfflineCause {
         // null when unknown
         private /*final*/ @CheckForNull String userId;
 
+        private final String message;
+
         public UserCause(@CheckForNull User user, @CheckForNull String message) {
-            this(
-                    user != null ? user.getId() : null,
-                    message != null ? " : " + message : ""
-            );
+            this(user != null ? user.getId() : null, message);
         }
 
         private UserCause(String userId, String message) {
-            super(hudson.slaves.Messages._SlaveComputer_DisconnectedBy(userId != null ? userId : Jenkins.ANONYMOUS2.getName(), message));
+            super(hudson.slaves.Messages._SlaveComputer_DisconnectedBy(userId != null ? userId : Jenkins.ANONYMOUS2.getName(), message != null ? " : " + message : ""));
+            this.message = message;
             this.userId = userId;
         }
 
@@ -155,6 +170,13 @@ public User getUser() {
             ;
         }
 
+        /**
+         * @return the message that was provided when the computer was taken offline
+         */
+        public String getMessage() {
+            return message;
+        }
+
         // Storing the User in a filed was a mistake, switch to userId
         private Object readResolve() throws ObjectStreamException {
             if (user != null) {

core/src/main/resources/hudson/model/Computer/setOfflineCause.jelly

@@ -34,7 +34,7 @@ THE SOFTWARE.
           ${%blurb}
         </p>
         <form method="post" action="changeOfflineCause">
-          <f:textarea name="offlineMessage" value="${it.getOfflineCauseReason()}"/>
+          <f:textarea name="offlineMessage" value="${it.temporaryOfflineCauseReason}"/>
           <p>
             <f:submit value="${%submit}"  />
           </p>

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

@@ -27,6 +27,8 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.endsWith;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -102,7 +104,10 @@ 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());
+        // Exists in some plugins
+        node.toComputer().setTemporarilyOffline(false, new OfflineCause.ByCLI("A third message"));
+        assertThat(node.getTemporaryOfflineCause(), nullValue());
     }
 
     @Test
@@ -115,13 +120,16 @@ public void testOfflineCause() throws Exception {
         try (ACLContext ignored = ACL.as2(someone.impersonate2())) {
             computer.doToggleOffline("original message");
             cause = (OfflineCause.UserCause) computer.getOfflineCause();
+            assertThat(computer.getOfflineCauseReason(), is("original message"));
+            assertThat(computer.getTemporaryOfflineCauseReason(), is("original message"));
             assertTrue(cause.toString(), cause.toString().matches("^.*?Disconnected by someone@somewhere.com : original message"));
             assertEquals(someone, cause.getUser());
         }
         final User root = User.getOrCreateByIdOrFullName("root@localhost");
         try (ACLContext ignored = ACL.as2(root.impersonate2())) {
             computer.doChangeOfflineCause("new message");
             cause = (OfflineCause.UserCause) computer.getOfflineCause();
+            assertThat(computer.getTemporaryOfflineCauseReason(), is("new message"));
             assertTrue(cause.toString(), cause.toString().matches("^.*?Disconnected by root@localhost : new message"));
             assertEquals(root, cause.getUser());