From e1f3ca89be5885b0c876db0350f17c0886542210 Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Thu, 4 Nov 2021 17:55:31 +0100
Subject: [PATCH 1/7] Make trim labels more selective when we're operating on
 selected nodes

---
 core/src/main/java/jenkins/model/Jenkins.java | 21 +++++++++++++++++++
 core/src/main/java/jenkins/model/Nodes.java   | 13 +++++++-----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index b6303b377a9f..a22f7efb3303 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2251,6 +2251,27 @@ public DescribableList<NodeProperty<?>, NodePropertyDescriptor> getGlobalNodePro
         }
     }
 
+    /**
+     * Reset labels and remove invalid ones for the given nodes.
+     * @param nodes the nodes taken as reference to update labels
+     */
+    void trimLabels(Node... nodes) {
+        Set<LabelAtom> includedLabels = new HashSet<>();
+        for (Node n : nodes) {
+            includedLabels.addAll(n.getAssignedLabels());
+        }
+        Set<Label> nodeLabels = new HashSet<>(this.getAssignedLabels());
+        this.getNodes().forEach(n -> nodeLabels.addAll(n.getAssignedLabels()));
+        for (Iterator<Label> itr = this.labels.values().stream().filter(l -> includedLabels.contains(l)).iterator(); itr.hasNext();) {
+            Label l = itr.next();
+            if (nodeLabels.contains(l) || this.clouds.stream().anyMatch(c -> c.canProvision(l))) {
+                resetLabel(l);
+            } else {
+                itr.remove();
+            }
+        }
+    }
+
     /**
      * Binds {@link AdministrativeMonitor}s to URL.
      * @param id Monitor ID
diff --git a/core/src/main/java/jenkins/model/Nodes.java b/core/src/main/java/jenkins/model/Nodes.java
index 7db2bc88417e..d7f4f0f10817 100644
--- a/core/src/main/java/jenkins/model/Nodes.java
+++ b/core/src/main/java/jenkins/model/Nodes.java
@@ -32,6 +32,7 @@
 import hudson.model.Node;
 import hudson.model.Queue;
 import hudson.model.Saveable;
+import hudson.model.labels.LabelAtom;
 import hudson.model.listeners.SaveableListener;
 import hudson.slaves.EphemeralNode;
 import hudson.slaves.OfflineCause;
@@ -112,10 +113,12 @@ public void setNodes(final @NonNull Collection<? extends Node> nodes) throws IOE
             @Override
             public void run() {
                 Set<String> toRemove = new HashSet<>(Nodes.this.nodes.keySet());
+                Set<LabelAtom> labels = new HashSet<>();
                 for (Node n : nodes) {
                     final String name = n.getNodeName();
                     toRemove.remove(name);
                     Nodes.this.nodes.put(name, n);
+                    labels.addAll(n.getAssignedLabels());
                 }
                 Nodes.this.nodes.keySet().removeAll(toRemove); // directory clean up will be handled by save
                 jenkins.updateComputerList();
@@ -141,7 +144,7 @@ public void addNode(final @NonNull Node node) throws IOException {
             AtomicReference<Node> old = new AtomicReference<>();
             old.set(nodes.put(node.getNodeName(), node));
             jenkins.updateNewComputer(node);
-            jenkins.trimLabels();
+            jenkins.trimLabels(node, oldNode);
             // TODO there is a theoretical race whereby the node instance is updated/removed after lock release
             try {
                 persistNode(node);
@@ -153,7 +156,7 @@ public void addNode(final @NonNull Node node) throws IOException {
                     public void run() {
                         nodes.compute(node.getNodeName(), (ignoredNodeName, ignoredNode) -> oldNode);
                         jenkins.updateComputerList();
-                        jenkins.trimLabels();
+                        jenkins.trimLabels(node, oldNode);
                     }
                 });
                 throw e;
@@ -201,7 +204,7 @@ public boolean updateNode(final @NonNull Node node) throws IOException {
                 @Override
                 public Boolean call() throws Exception {
                     if (node == nodes.get(node.getNodeName())) {
-                        jenkins.trimLabels();
+                        jenkins.trimLabels(node);
                         return true;
                     }
                     return false;
@@ -242,7 +245,7 @@ public void run() {
                     Nodes.this.nodes.remove(oldOne.getNodeName());
                     Nodes.this.nodes.put(newOne.getNodeName(), newOne);
                     jenkins.updateComputerList();
-                    jenkins.trimLabels();
+                    jenkins.trimLabels(oldOne, newOne);
                 }
             });
             updateNode(newOne);
@@ -276,7 +279,7 @@ public void run() {
                     }
                     if (node == nodes.remove(node.getNodeName())) {
                         jenkins.updateComputerList();
-                        jenkins.trimLabels();
+                        jenkins.trimLabels(node);
                     }
                 }
             });

From 82abd6d6a9ef839540dae5305e47a49587a2406d Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Fri, 5 Nov 2021 15:02:12 +0100
Subject: [PATCH 2/7] Remove unused variable

---
 core/src/main/java/jenkins/model/Nodes.java | 2 --
 1 file changed, 2 deletions(-)

diff --git a/core/src/main/java/jenkins/model/Nodes.java b/core/src/main/java/jenkins/model/Nodes.java
index d7f4f0f10817..8eaaca810c58 100644
--- a/core/src/main/java/jenkins/model/Nodes.java
+++ b/core/src/main/java/jenkins/model/Nodes.java
@@ -113,12 +113,10 @@ public void setNodes(final @NonNull Collection<? extends Node> nodes) throws IOE
             @Override
             public void run() {
                 Set<String> toRemove = new HashSet<>(Nodes.this.nodes.keySet());
-                Set<LabelAtom> labels = new HashSet<>();
                 for (Node n : nodes) {
                     final String name = n.getNodeName();
                     toRemove.remove(name);
                     Nodes.this.nodes.put(name, n);
-                    labels.addAll(n.getAssignedLabels());
                 }
                 Nodes.this.nodes.keySet().removeAll(toRemove); // directory clean up will be handled by save
                 jenkins.updateComputerList();

From 82713a7f59285b8a2aac45b26ab56383c3aff82d Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Tue, 9 Nov 2021 10:42:00 +0100
Subject: [PATCH 3/7] n can be null

---
 core/src/main/java/jenkins/model/Jenkins.java | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index a22f7efb3303..185d1d431b0f 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2258,7 +2258,9 @@ public DescribableList<NodeProperty<?>, NodePropertyDescriptor> getGlobalNodePro
     void trimLabels(Node... nodes) {
         Set<LabelAtom> includedLabels = new HashSet<>();
         for (Node n : nodes) {
-            includedLabels.addAll(n.getAssignedLabels());
+            if (n != null) {
+                includedLabels.addAll(n.getAssignedLabels());
+            }
         }
         Set<Label> nodeLabels = new HashSet<>(this.getAssignedLabels());
         this.getNodes().forEach(n -> nodeLabels.addAll(n.getAssignedLabels()));

From dc8f944d41cf87baee4afff792147f6003c066f8 Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Tue, 9 Nov 2021 12:44:45 +0100
Subject: [PATCH 4/7] Don't filter in stream in order to get a writable
 iterator

---
 core/src/main/java/jenkins/model/Jenkins.java | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index 185d1d431b0f..8d75110ab1ac 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2264,12 +2264,14 @@ void trimLabels(Node... nodes) {
         }
         Set<Label> nodeLabels = new HashSet<>(this.getAssignedLabels());
         this.getNodes().forEach(n -> nodeLabels.addAll(n.getAssignedLabels()));
-        for (Iterator<Label> itr = this.labels.values().stream().filter(l -> includedLabels.contains(l)).iterator(); itr.hasNext();) {
+        for (Iterator<Label> itr = this.labels.values().iterator(); itr.hasNext();) {
             Label l = itr.next();
-            if (nodeLabels.contains(l) || this.clouds.stream().anyMatch(c -> c.canProvision(l))) {
-                resetLabel(l);
-            } else {
-                itr.remove();
+            if (includedLabels.contains(l)) {
+                if (nodeLabels.contains(l) || this.clouds.stream().anyMatch(c -> c.canProvision(l))) {
+                    resetLabel(l);
+                } else {
+                    itr.remove();
+                }
             }
         }
     }

From ff971a550eecbb681cc532306776a86ee15b5bfb Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Tue, 9 Nov 2021 15:24:29 +0100
Subject: [PATCH 5/7] Refactoring

---
 core/src/main/java/jenkins/model/Jenkins.java | 29 ++++++++-----------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index 8d75110ab1ac..85ba24ab79c1 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2239,16 +2239,7 @@ public DescribableList<NodeProperty<?>, NodePropertyDescriptor> getGlobalNodePro
      * but we also call this periodically to self-heal any data out-of-sync issue.
      */
     /*package*/ void trimLabels() {
-        Set<Label> nodeLabels = new HashSet<>(this.getAssignedLabels());
-        this.getNodes().forEach(n -> nodeLabels.addAll(n.getAssignedLabels()));
-        for (Iterator<Label> itr = labels.values().iterator(); itr.hasNext();) {
-            Label l = itr.next();
-            if (nodeLabels.contains(l) || this.clouds.stream().anyMatch(c -> c.canProvision(l))) {
-                resetLabel(l);
-            } else {
-                itr.remove();
-            }
-        }
+        trimLabels((Set) null);
     }
 
     /**
@@ -2257,16 +2248,20 @@ public DescribableList<NodeProperty<?>, NodePropertyDescriptor> getGlobalNodePro
      */
     void trimLabels(Node... nodes) {
         Set<LabelAtom> includedLabels = new HashSet<>();
-        for (Node n : nodes) {
-            if (n != null) {
-                includedLabels.addAll(n.getAssignedLabels());
-            }
-        }
+        Arrays.asList(nodes).stream().filter(Objects::nonNull).forEach(n -> includedLabels.addAll(n.getAssignedLabels()));
+        trimLabels(includedLabels);
+    }
+
+    /**
+     * Reset labels and remove invalid ones for the given nodes.
+     * @param includedLabels the labels taken as reference to update labels. If null, all labels are considered.
+     */
+    private void trimLabels(@CheckForNull Set<LabelAtom> includedLabels) {
         Set<Label> nodeLabels = new HashSet<>(this.getAssignedLabels());
         this.getNodes().forEach(n -> nodeLabels.addAll(n.getAssignedLabels()));
-        for (Iterator<Label> itr = this.labels.values().iterator(); itr.hasNext();) {
+        for (Iterator<Label> itr = labels.values().iterator(); itr.hasNext();) {
             Label l = itr.next();
-            if (includedLabels.contains(l)) {
+            if (includedLabels == null || includedLabels.contains(l)) {
                 if (nodeLabels.contains(l) || this.clouds.stream().anyMatch(c -> c.canProvision(l))) {
                     resetLabel(l);
                 } else {

From 857e17a7aaa61576887fc7825e9593609a7265a4 Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Wed, 10 Nov 2021 09:53:50 +0100
Subject: [PATCH 6/7] Apply @teilo's suggestion

---
 core/src/main/java/jenkins/model/Jenkins.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index 85ba24ab79c1..af796b1f16ae 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2262,7 +2262,7 @@ private void trimLabels(@CheckForNull Set<LabelAtom> includedLabels) {
         for (Iterator<Label> itr = labels.values().iterator(); itr.hasNext();) {
             Label l = itr.next();
             if (includedLabels == null || includedLabels.contains(l)) {
-                if (nodeLabels.contains(l) || this.clouds.stream().anyMatch(c -> c.canProvision(l))) {
+                if (nodeLabels.contains(l) || !l.getClouds().isEmpty()) {
                     resetLabel(l);
                 } else {
                     itr.remove();

From 1d17bb5235c4bfff84adc519ebbe7f1234149544 Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Wed, 10 Nov 2021 14:29:58 +0100
Subject: [PATCH 7/7] Apply suggestions from @jtnord code review

Co-authored-by: James Nord <jtnord@users.noreply.github.com>
---
 core/src/main/java/jenkins/model/Jenkins.java | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
index af796b1f16ae..03c43b8bdfc4 100644
--- a/core/src/main/java/jenkins/model/Jenkins.java
+++ b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2254,7 +2254,7 @@ void trimLabels(Node... nodes) {
 
     /**
      * Reset labels and remove invalid ones for the given nodes.
-     * @param includedLabels the labels taken as reference to update labels. If null, all labels are considered.
+     * @param includedLabels the labels taken as reference to update labels. If {@code null}, all labels are considered.
      */
     private void trimLabels(@CheckForNull Set<LabelAtom> includedLabels) {
         Set<Label> nodeLabels = new HashSet<>(this.getAssignedLabels());
@@ -2263,6 +2263,10 @@ private void trimLabels(@CheckForNull Set<LabelAtom> includedLabels) {
             Label l = itr.next();
             if (includedLabels == null || includedLabels.contains(l)) {
                 if (nodeLabels.contains(l) || !l.getClouds().isEmpty()) {
+                    // there is at least one static agent or one cloud that currently claims it can handle the label.
+                    // if the cloud has been removed, or its labels updated such that it can not handle this, this is handle in later calls
+                    // resetLabel will remove the agents, and clouds from the label, and they will be repopulated later.
+                    // not checking `cloud.canProvision()` here prevents a potential call that will only be repeated later
                     resetLabel(l);
                 } else {
                     itr.remove();