[JENKINS-72371] rework node monitor configuration (#8719)

* [JENKINS-72371] rework node monitor configuration This PR reworks the way node monitors are configured. It ensures that also monitors which are set to ignored, can be configured. Previously, there was a checkbox before each monitor, where the behaviour was not totally clear. It meant that the monitor is ignored but still collecting data and not disabled as one might think. But when the monitor was disabled any configuration was lost and default values were used. * improve description * fix formatting * add since
jenkinsci · Dec 18, 2023 · 7258cad · 7258cad
1 parent 7a2e389
commit 7258cad
Show file tree

Hide file tree

Showing 15 changed files with 115 additions and 33 deletions.
diff --git a/core/src/main/java/hudson/model/ComputerSet.java b/core/src/main/java/hudson/model/ComputerSet.java
@@ -352,15 +352,33 @@ public synchronized HttpResponse doConfigSubmit(StaplerRequest req) throws IOExc
         BulkChange bc = new BulkChange(MONITORS_OWNER);
         try {
             Jenkins.get().checkPermission(Jenkins.MANAGE);
-            monitors.rebuild(req, req.getSubmittedForm(), getNodeMonitorDescriptors());
+            JSONObject json = req.getSubmittedForm();
+            monitors.rebuild(req, json, getNodeMonitorDescriptors());
 
             // add in the rest of instances are ignored instances
-            for (Descriptor<NodeMonitor> d : NodeMonitor.all())
-                if (monitors.get(d) == null) {
+            for (Descriptor<NodeMonitor> d : NodeMonitor.all()) {
+                NodeMonitor monitor = monitors.get(d);
+                if (monitor == null) {
                     NodeMonitor i = createDefaultInstance(d, true);
                     if (i != null)
                         monitors.add(i);
+                } else {
+                    /*
+                     * Some monitors in plugins do not have a DataBoundConstructor
+                     * but a Descriptor that overrides newInstance. For those the ignored
+                     * field is not set, so we have to explicitly set it.
+                     */
+                    String name = d.getJsonSafeClassName();
+                    JSONObject o = json.optJSONObject(name);
+                    boolean ignored = true;
+                    if (o != null) {
+                        if (o.containsKey("ignored")) {
+                            ignored = o.getBoolean("ignored");
+                        }
+                    }
+                    monitor.setIgnored(ignored);
                 }
+            }
 
             // recompute the data
             for (NodeMonitor nm : monitors) {

diff --git a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java b/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java
@@ -65,6 +65,25 @@ protected AbstractNodeMonitorDescriptor() {
         this(PERIOD);
     }
 
+    /**
+     * Indicates if this monitor is capable to take agents offline in case it detects a problem.
+     * If true, this will enable the configuration option to ignore the monitor.
+     * Defaults to {@code true} so plugins that do not override this method behave as before.
+     * Plugins that do implement a monitor that will not take agents offline should override this
+     * method and return false.
+     *
+     * @return true if this monitor might take agents offline
+     * @since TODO
+     */
+    public boolean canTakeOffline() {
+        return true;
+    }
+
+    @Override
+    public String getConfigPage() {
+        return getViewPage(clazz, "configure.jelly");
+    }
+
     /**
      * @deprecated as of 1.522
      *      Extend from {@link AbstractAsyncNodeMonitorDescriptor}

diff --git a/core/src/main/java/hudson/node_monitors/ArchitectureMonitor.java b/core/src/main/java/hudson/node_monitors/ArchitectureMonitor.java
@@ -30,10 +30,8 @@
 import hudson.remoting.Callable;
 import java.io.IOException;
 import jenkins.security.MasterToSlaveCallable;
-import net.sf.json.JSONObject;
 import org.jenkinsci.Symbol;
 import org.kohsuke.stapler.DataBoundConstructor;
-import org.kohsuke.stapler.StaplerRequest;
 
 /**
  * Discovers the architecture of the system to display in the agent list page.
@@ -60,8 +58,8 @@ public String getDisplayName() {
         }
 
         @Override
-        public NodeMonitor newInstance(StaplerRequest req, JSONObject formData) throws FormException {
-            return new ArchitectureMonitor();
+        public boolean canTakeOffline() {
+            return false;
         }
     }
 

diff --git a/core/src/main/java/hudson/node_monitors/ClockMonitor.java b/core/src/main/java/hudson/node_monitors/ClockMonitor.java
@@ -32,12 +32,10 @@
 import hudson.remoting.Callable;
 import hudson.util.ClockDifference;
 import java.io.IOException;
-import net.sf.json.JSONObject;
 import org.jenkinsci.Symbol;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.DataBoundConstructor;
-import org.kohsuke.stapler.StaplerRequest;
 
 /**
  * {@link NodeMonitor} that checks clock of {@link Node} to
@@ -72,6 +70,11 @@ public DescriptorImpl() {
             DESCRIPTOR = this;
         }
 
+        @Override
+        public boolean canTakeOffline() {
+            return false;
+        }
+
         @Override
         protected Callable<ClockDifference, IOException> createCallable(Computer c) {
             Node n = c.getNode();
@@ -84,10 +87,5 @@ protected Callable<ClockDifference, IOException> createCallable(Computer c) {
         public String getDisplayName() {
             return Messages.ClockMonitor_DisplayName();
         }
-
-        @Override
-        public NodeMonitor newInstance(StaplerRequest req, JSONObject formData) throws FormException {
-            return new ClockMonitor();
-        }
     }
 }
diff --git a/core/src/main/java/hudson/node_monitors/NodeMonitor.java b/core/src/main/java/hudson/node_monitors/NodeMonitor.java
@@ -37,6 +37,7 @@
 import hudson.util.DescriptorList;
 import java.util.List;
 import jenkins.model.Jenkins;
+import org.kohsuke.stapler.DataBoundSetter;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
 
@@ -133,6 +134,7 @@ public boolean isIgnored() {
         return ignored;
     }
 
+    @DataBoundSetter
     public void setIgnored(boolean ignored) {
         this.ignored = ignored;
     }

diff --git a/core/src/main/java/hudson/node_monitors/ResponseTimeMonitor.java b/core/src/main/java/hudson/node_monitors/ResponseTimeMonitor.java
@@ -34,10 +34,8 @@
 import java.util.Map;
 import java.util.logging.Logger;
 import jenkins.security.MasterToSlaveCallable;
-import net.sf.json.JSONObject;
 import org.jenkinsci.Symbol;
 import org.kohsuke.stapler.DataBoundConstructor;
-import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
 
@@ -103,11 +101,6 @@ protected Map<Computer, Data> monitor() throws InterruptedException {
         public String getDisplayName() {
             return Messages.ResponseTimeMonitor_DisplayName();
         }
-
-        @Override
-        public NodeMonitor newInstance(StaplerRequest req, JSONObject formData) throws FormException {
-            return new ResponseTimeMonitor();
-        }
     }
 
     private static final class Step1 extends MasterToSlaveCallable<Data, IOException> {

diff --git a/core/src/main/java/hudson/node_monitors/SwapSpaceMonitor.java b/core/src/main/java/hudson/node_monitors/SwapSpaceMonitor.java
@@ -33,12 +33,10 @@
 import java.io.IOException;
 import jenkins.model.Jenkins;
 import jenkins.security.MasterToSlaveCallable;
-import net.sf.json.JSONObject;
 import org.jenkinsci.Symbol;
 import org.jvnet.hudson.MemoryMonitor;
 import org.jvnet.hudson.MemoryUsage;
 import org.kohsuke.stapler.DataBoundConstructor;
-import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.export.Exported;
 import org.kohsuke.stapler.export.ExportedBean;
 
@@ -103,6 +101,11 @@ public DescriptorImpl() {
             DESCRIPTOR = this;
         }
 
+        @Override
+        public boolean canTakeOffline() {
+            return false;
+        }
+
         @Override
         protected MonitorTask createCallable(Computer c) {
             return new MonitorTask();
@@ -113,11 +116,6 @@ protected MonitorTask createCallable(Computer c) {
         public String getDisplayName() {
             return Messages.SwapSpaceMonitor_DisplayName();
         }
-
-        @Override
-        public NodeMonitor newInstance(StaplerRequest req, JSONObject formData) throws FormException {
-            return new SwapSpaceMonitor();
-        }
     }
 
     /**

diff --git a/core/src/main/resources/hudson/model/ComputerSet/configure.jelly b/core/src/main/resources/hudson/model/ComputerSet/configure.jelly
@@ -42,7 +42,8 @@ THE SOFTWARE.
       <f:form method="post" action="configSubmit" name="config">
         <f:descriptorList title="${%Preventive Node Monitoring}"
                           descriptors="${it.nodeMonitorDescriptors}"
-                          instances="${it.nonIgnoredMonitors}" />
+                          instances="${it.monitors}"
+                          forceRowSet="true"/>
 
         <l:hasAdministerOrManage>
           <f:bottomButtonBar>

diff --git a/core/src/main/resources/hudson/model/ComputerSet/configure.properties b/core/src/main/resources/hudson/model/ComputerSet/configure.properties
@@ -1 +1,2 @@
-description=Jenkins monitors each attached node for disk space, free temp space, free swap, clock time/sync, and response time. Nodes will be taken offline if any of these values go outside of the configured threshold.
+description=Jenkins monitors each attached node for various metrics like free disk space, free temp space, free swap, clock time/sync, response time and others provided by plugins. \
+  Some of these monitors are able to mark nodes temporarily offline, when the values go outside the configured thresholds.
diff --git a/core/src/main/resources/hudson/node_monitors/ArchitectureMonitor/help.html b/core/src/main/resources/hudson/node_monitors/ArchitectureMonitor/help.html
@@ -1,4 +1,4 @@
 <div>
   This monitor just shows the architecture of the agent for your information. It
-  never marks the agent offline.
+  never marks an agent offline.
 </div>
diff --git a/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html b/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html
@@ -4,6 +4,8 @@
   systems, version control activities and distributed file access (such as NFS,
   Windows file shares) tend to have strange problems when systems involved have
   different clocks.
+  <br />
+  It never marks an agent offline.
 
   <p>To keep clocks in sync, refer to NTP.</p>
 </div>
diff --git a/core/src/main/resources/hudson/node_monitors/NodeMonitor/configure.jelly b/core/src/main/resources/hudson/node_monitors/NodeMonitor/configure.jelly
@@ -0,0 +1,24 @@
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:f="/lib/form">
+  <j:set var="help" value="${descriptor.helpFile}"/>
+  <div class="jenkins-form-label help-sibling tr">
+    ${descriptor.displayName}
+    <f:helpLink url="${help}" featureName="${descriptor.displayName}"/>
+  </div>
+  <f:helpArea />
+  <div class="optionalBlock-container jenkins-form-item">
+    <div class="form-container">
+      <j:if test="${descriptor.canTakeOffline()}">
+        <f:entry title="${%Don't mark agents temporarily offline}" field="ignored">
+          <f:checkbox checked="${instance.ignored}" />
+        </f:entry>
+      </j:if>
+      <j:if test="${!descriptor.canTakeOffline()}">
+        <f:invisibleEntry>
+          <f:checkbox name="ignored" checked="false" />
+        </f:invisibleEntry>
+      </j:if>
+      <st:include page="config.jelly" class="${descriptor.clazz}" optional="true"/>
+    </div>
+  </div>
+</j:jelly>
diff --git a/core/src/main/resources/hudson/node_monitors/NodeMonitor/help-ignored.html b/core/src/main/resources/hudson/node_monitors/NodeMonitor/help-ignored.html
@@ -0,0 +1,7 @@
+<div>
+  When checked the agent will
+  <i>not</i>
+  be marked temporarily offline in case the monitor detects a problem. But the
+  monitor will still run to collect data and the corresponding field will be
+  highlighted in case of a problem.
+</div>
diff --git a/core/src/main/resources/hudson/node_monitors/ResponseTimeMonitor/help.html b/core/src/main/resources/hudson/node_monitors/ResponseTimeMonitor/help.html
@@ -1,7 +1,6 @@
 <div>
   This monitors the round trip network response time from the controller to the
-  agent, and if it goes above a threshold repeatedly, it marks the agent
-  offline.
+  agent, and if it goes above a threshold repeatedly, it disconnects the agent.
 
   <p>
     This is useful for detecting unresponsive agents, or other network problems

diff --git a/core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html b/core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html
@@ -0,0 +1,22 @@
+<div>
+  This monitors the available virtual memory (also known as
+  <code>swap space</code>
+  ).
+  <p>The exact definition of swap space is platform dependent.</p>
+  <ul>
+    <li>
+      On Windows this is the available space of the page file. As Windows can
+      automatically increase the page file size, this value doesn't mean much.
+    </li>
+    <li>
+      On Linux this value is retrieved from
+      <code>/proc/meminfo</code>
+      .
+    </li>
+    <li>
+      For other Unix systems, the value is obtained from the
+      <code>top</code>
+      command.
+    </li>
+  </ul>
+</div>