[JENKINS-68155] Fix state of previous labels when updating a node label (#10501)

Author: krisstern

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

@@ -341,6 +341,15 @@ public Set<LabelAtom> getAssignedLabels() {
         return Collections.unmodifiableSet(r);
     }
 
+    /**
+     * @return the labels to be trimmed for this node.
+     */
+    @NonNull
+    @Restricted(NoExternalUse.class)
+    public Set<LabelAtom> drainLabelsToTrim() {
+        return new HashSet<>(getAssignedLabels());
+    }
+
     /**
      * Return all the labels assigned dynamically to this node.
      * This calls all the LabelFinder implementations with the node converts

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

@@ -63,13 +63,15 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 import java.util.jar.JarFile;
 import java.util.jar.Manifest;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 import jenkins.model.Jenkins;
 import jenkins.security.MasterToSlaveCallable;
 import jenkins.slaves.WorkspaceLocator;
@@ -340,6 +342,23 @@ public String getLabelString() {
         return Util.fixNull(label).trim();
     }
 
+    private transient Set<LabelAtom> previouslyAssignedLabels = new HashSet<>();
+
+    /**
+     * @return the labels to be trimmed for this node. This includes current and previous labels that were applied before the last call to this method.
+     */
+    @Override
+    @NonNull
+    @Restricted(NoExternalUse.class)
+    public Set<LabelAtom> drainLabelsToTrim() {
+        var result = new HashSet<>(super.drainLabelsToTrim());
+        synchronized (previouslyAssignedLabels) {
+            result.addAll(previouslyAssignedLabels);
+            previouslyAssignedLabels.clear();
+        }
+        return result;
+    }
+
     @Override
     @DataBoundSetter
     public void setLabelString(String labelString) throws IOException {
@@ -349,6 +368,9 @@ public void setLabelString(String labelString) throws IOException {
     }
 
     private void _setLabelString(String labelString) {
+        synchronized (this.previouslyAssignedLabels) {
+            this.previouslyAssignedLabels.addAll(getAssignedLabels().stream().filter(Objects::nonNull).collect(Collectors.toSet()));
+        }
         this.label = Util.fixNull(labelString).trim();
         this.labelAtomSet = Collections.unmodifiableSet(Label.parse(label));
     }
@@ -611,6 +633,7 @@ public int hashCode() {
     protected Object readResolve() {
         if (nodeProperties == null)
             nodeProperties = new DescribableList<>(this);
+        previouslyAssignedLabels = new HashSet<>();
         _setLabelString(label);
         return this;
     }

core/src/main/java/jenkins/model/Jenkins.java

@@ -2246,7 +2246,7 @@ public DescribableList<NodeProperty<?>, NodePropertyDescriptor> getGlobalNodePro
      */
     void trimLabels(Node... nodes) {
         Set<LabelAtom> includedLabels = new HashSet<>();
-        Arrays.stream(nodes).filter(Objects::nonNull).forEach(n -> includedLabels.addAll(n.getAssignedLabels()));
+        Arrays.stream(nodes).filter(Objects::nonNull).forEach(n -> includedLabels.addAll(n.drainLabelsToTrim()));
         trimLabels(includedLabels);
     }
 

test/src/test/java/hudson/model/labels/LabelAtomTest.java

@@ -1,9 +1,11 @@
 package hudson.model.labels;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertNotNull;
 
 import hudson.model.Label;
 import hudson.model.Node;
@@ -12,6 +14,7 @@
 import java.util.List;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 
 public class LabelAtomTest {
@@ -69,6 +72,43 @@ public void isEmpty() throws Exception {
         assertThat(l2.isEmpty(), is(false));
     }
 
+    @Test
+    @Issue("JENKINS-68155")
+    public void changeNodeLabel() throws Exception {
+        var slave = j.createSlave("node", "label linux", null);
+        var label = Label.get("label");
+        assertNotNull(label);
+        assertThat(label.getNodes(), contains(slave));
+        var osLabel = Label.get("linux");
+        assertNotNull(osLabel);
+        assertThat(osLabel.getNodes(), contains(slave));
+        slave.setLabelString("label2 linux");
+        j.jenkins.updateNode(slave);
+        label = Label.get("label");
+        assertNotNull(label);
+        assertThat(label.getNodes(), empty());
+        var label2 = Label.get("label2");
+        assertNotNull(label2);
+        assertThat(label2.getNodes(), contains(slave));
+        osLabel = Label.get("linux");
+        assertNotNull(osLabel);
+        assertThat(osLabel.getNodes(), contains(slave));
+    }
+
+    @Test
+    @Issue("JENKINS-68155")
+    public void removeNodeLabel() throws Exception {
+        var slave = j.createSlave("node", "label", null);
+        var label = Label.get("label");
+        assertNotNull(label);
+        assertThat(label.getNodes(), contains(slave));
+        slave.setLabelString(null);
+        j.jenkins.updateNode(slave);
+        label = Label.get("label");
+        assertNotNull(label);
+        assertThat(label.getNodes(), empty());
+    }
+
     private static class TestCloud extends Cloud {
 
         private final List<Label> labels;