diff --git a/core/src/main/java/hudson/model/Computer.java b/core/src/main/java/hudson/model/Computer.java index 9e3037edf83c..8c3d814c91fe 100644 --- a/core/src/main/java/hudson/model/Computer.java +++ b/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 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(); } diff --git a/core/src/main/java/hudson/model/Node.java b/core/src/main/java/hudson/model/Node.java index 55cacd269133..d918b0f1db34 100644 --- a/core/src/main/java/hudson/model/Node.java +++ b/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()); } diff --git a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java b/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java index 7cd1c75abc8d..d49924508bee 100644 --- a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java +++ b/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); diff --git a/core/src/main/java/hudson/slaves/OfflineCause.java b/core/src/main/java/hudson/slaves/OfflineCause.java index 556c0ebb0c53..2a267ef03a4a 100644 --- a/core/src/main/java/hudson/slaves/OfflineCause.java +++ b/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) { diff --git a/core/src/main/resources/hudson/model/Computer/setOfflineCause.jelly b/core/src/main/resources/hudson/model/Computer/setOfflineCause.jelly index 587057a464e1..8bac3c7e27c7 100644 --- a/core/src/main/resources/hudson/model/Computer/setOfflineCause.jelly +++ b/core/src/main/resources/hudson/model/Computer/setOfflineCause.jelly @@ -34,7 +34,7 @@ THE SOFTWARE. ${%blurb}

- +

diff --git a/test/src/test/java/hudson/cli/OfflineNodeCommandTest.java b/test/src/test/java/hudson/cli/OfflineNodeCommandTest.java index 564c1b098baf..878001e1b145 100644 --- a/test/src/test/java/hudson/cli/OfflineNodeCommandTest.java +++ b/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) diff --git a/test/src/test/java/hudson/model/NodeTest.java b/test/src/test/java/hudson/model/NodeTest.java index 39ffe25f7643..cae1c2120982 100644 --- a/test/src/test/java/hudson/model/NodeTest.java +++ b/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,6 +120,8 @@ 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()); } @@ -122,6 +129,7 @@ public void testOfflineCause() throws Exception { 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());