From 232e9abba5c2bb6a863c515bdd05bab0d7b06a5c Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Sat, 25 Nov 2023 02:52:09 +0100 Subject: [PATCH 1/4] [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. --- .../main/java/hudson/model/ComputerSet.java | 24 ++++++++++++++++--- .../AbstractNodeMonitorDescriptor.java | 15 ++++++++++++ .../node_monitors/ArchitectureMonitor.java | 6 ++--- .../hudson/node_monitors/ClockMonitor.java | 12 ++++------ .../hudson/node_monitors/NodeMonitor.java | 2 ++ .../node_monitors/ResponseTimeMonitor.java | 7 ------ .../node_monitors/SwapSpaceMonitor.java | 12 ++++------ .../hudson/model/ComputerSet/configure.jelly | 3 ++- .../ArchitectureMonitor/help.html | 2 +- .../node_monitors/ClockMonitor/help.html | 3 ++- .../node_monitors/NodeMonitor/configure.jelly | 24 +++++++++++++++++++ .../NodeMonitor/help-ignored.html | 4 ++++ .../ResponseTimeMonitor/help.html | 3 +-- .../node_monitors/SwapSpaceMonitor/help.html | 10 ++++++++ 14 files changed, 94 insertions(+), 33 deletions(-) create mode 100644 core/src/main/resources/hudson/node_monitors/NodeMonitor/configure.jelly create mode 100644 core/src/main/resources/hudson/node_monitors/NodeMonitor/help-ignored.html create mode 100644 core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html diff --git a/core/src/main/java/hudson/model/ComputerSet.java b/core/src/main/java/hudson/model/ComputerSet.java index cad9089b776f..5cb798978dea 100644 --- 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 d : NodeMonitor.all()) - if (monitors.get(d) == null) { + for (Descriptor 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 index 4a2386c2a1b1..d8a594cd12c1 100644 --- a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java +++ b/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java @@ -65,6 +65,21 @@ 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. + * @return true if this monitor might take agents offline + */ + 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 index d2c60a2544d0..e9c63ab74433 100644 --- 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 index b958df61d776..b9c0f77b5c99 100644 --- 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 createCallable(Computer c) { Node n = c.getNode(); @@ -84,10 +87,5 @@ protected Callable 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 index 0fb737a7677a..61ac5d67b7d4 100644 --- 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 index 5a7223039696..a2bf84ea817f 100644 --- 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 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 { diff --git a/core/src/main/java/hudson/node_monitors/SwapSpaceMonitor.java b/core/src/main/java/hudson/node_monitors/SwapSpaceMonitor.java index 09064ccb4a82..fa7f53eab70b 100644 --- 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 index 4bbd52b4c4dc..9e58aeda59e6 100644 --- 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. + instances="${it.monitors}" + forceRowSet="true"/> diff --git a/core/src/main/resources/hudson/node_monitors/ArchitectureMonitor/help.html b/core/src/main/resources/hudson/node_monitors/ArchitectureMonitor/help.html index 23efe3bdeff0..a67b11024f35 100644 --- 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 @@
This monitor just shows the architecture of the agent for your information. It - never marks the agent offline. + never marks an agent offline.
diff --git a/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html b/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html index f9a1a868b1ab..dd8604316d61 100644 --- a/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html +++ b/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html @@ -3,7 +3,8 @@ Jenkins itself is generally capable of tolerating clock differences between 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. + different clocks.
+ It never marks an agent offline.

To keep clocks in sync, refer to NTP.

diff --git a/core/src/main/resources/hudson/node_monitors/NodeMonitor/configure.jelly b/core/src/main/resources/hudson/node_monitors/NodeMonitor/configure.jelly new file mode 100644 index 000000000000..a2016d12307f --- /dev/null +++ b/core/src/main/resources/hudson/node_monitors/NodeMonitor/configure.jelly @@ -0,0 +1,24 @@ + + + +
+ ${descriptor.displayName} + +
+ +
+
+ + + + + + + + + + + +
+
+ \ No newline at end of file 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 new file mode 100644 index 000000000000..ac6f3ff2a3e7 --- /dev/null +++ b/core/src/main/resources/hudson/node_monitors/NodeMonitor/help-ignored.html @@ -0,0 +1,4 @@ +
+ When checked the agent will not 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. +
diff --git a/core/src/main/resources/hudson/node_monitors/ResponseTimeMonitor/help.html b/core/src/main/resources/hudson/node_monitors/ResponseTimeMonitor/help.html index ed324b07d50c..978d0a28dfb9 100644 --- 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 @@
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.

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 new file mode 100644 index 000000000000..543f709c4a67 --- /dev/null +++ b/core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html @@ -0,0 +1,10 @@ +

+ This monitors the available virtual memory (also known as swap space). +

The exact definition of swap space is platform dependent.

+
    +
  • 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.
    • +
  • On Linux this value is retrieved from /proc/meminfo.
    • +
  • For other Unix systems, the value is obtained from the top command.
    • +
+
From 8d83d49591b714451730ead884f6be718d40f658 Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Sat, 25 Nov 2023 03:33:48 +0100 Subject: [PATCH 2/4] improve description --- .../resources/hudson/model/ComputerSet/configure.properties | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/resources/hudson/model/ComputerSet/configure.properties b/core/src/main/resources/hudson/model/ComputerSet/configure.properties index e2e74cba5ece..1df5cc610747 100644 --- 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. From 3580d8947e331c8ee27bdbc79b8ff7a8c6eb82f6 Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Sat, 25 Nov 2023 14:07:15 +0100 Subject: [PATCH 3/4] fix formatting --- .../node_monitors/ClockMonitor/help.html | 3 ++- .../NodeMonitor/help-ignored.html | 7 ++++-- .../node_monitors/SwapSpaceMonitor/help.html | 22 ++++++++++++++----- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html b/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html index dd8604316d61..ee5e795f2ce7 100644 --- a/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html +++ b/core/src/main/resources/hudson/node_monitors/ClockMonitor/help.html @@ -3,7 +3,8 @@ Jenkins itself is generally capable of tolerating clock differences between 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.
+ different clocks. +
It never marks an agent offline.

To keep clocks in sync, refer to NTP.

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 index ac6f3ff2a3e7..7dfe9ac4071a 100644 --- a/core/src/main/resources/hudson/node_monitors/NodeMonitor/help-ignored.html +++ b/core/src/main/resources/hudson/node_monitors/NodeMonitor/help-ignored.html @@ -1,4 +1,7 @@
- When checked the agent will not 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. + When checked the agent will + not + 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.
diff --git a/core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html b/core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html index 543f709c4a67..f1827bdccbbe 100644 --- a/core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html +++ b/core/src/main/resources/hudson/node_monitors/SwapSpaceMonitor/help.html @@ -1,10 +1,22 @@
- This monitors the available virtual memory (also known as swap space). + This monitors the available virtual memory (also known as + swap space + ).

The exact definition of swap space is platform dependent.

    -
  • 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.
    • -
  • On Linux this value is retrieved from /proc/meminfo.
    • -
  • For other Unix systems, the value is obtained from the top command.
    • +
  • + 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. +
    • +
  • + On Linux this value is retrieved from + /proc/meminfo + . +
    • +
  • + For other Unix systems, the value is obtained from the + top + command. +
From 883db263013bcddb369f5d1baceea71da874b95a Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Sat, 25 Nov 2023 17:48:57 +0100 Subject: [PATCH 4/4] add @since --- .../hudson/node_monitors/AbstractNodeMonitorDescriptor.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java b/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java index d8a594cd12c1..927e826ece67 100644 --- a/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java +++ b/core/src/main/java/hudson/node_monitors/AbstractNodeMonitorDescriptor.java @@ -69,7 +69,11 @@ protected AbstractNodeMonitorDescriptor() { * 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;