Config-retired should not override explicit Down or Maintenance states

Previously, a config-retired node marked as Down by the Orchestrator
would remain as Retired in the cluster state until the node was
actually taken down entirely.

4 files changed, 62 insertions, 6 deletions

diff --git a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java
index 3cdbfba282c..15650f0f4aa 100644
--- a/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java
+++ b/clustercontroller-core/src/main/java/com/yahoo/vespa/clustercontroller/core/NodeInfo.java
@@ -258,7 +258,11 @@ abstract public class NodeInfo implements Comparable<NodeInfo> {
 
     /** Returns the wanted state of this node - which can either be set by a user or configured */
     public NodeState getWantedState() {
-        if (configuredRetired) return new NodeState(node.getType(), State.RETIRED);
+        NodeState retiredState = new NodeState(node.getType(), State.RETIRED);
+        // Don't let configure retired state override explicitly set Down and Maintenance.
+        if (configuredRetired && wantedState.above(retiredState)) {
+            return retiredState;
+        }
         return wantedState;
     }
 
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFixture.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFixture.java
index 9ad405d6f90..16e9675d586 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFixture.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterFixture.java
@@ -12,8 +12,10 @@ import com.yahoo.vespa.clustercontroller.core.listeners.NodeStateOrHostInfoChang
 import com.yahoo.vespa.clustercontroller.utils.util.NoMetricReporter;
 
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.TreeMap;
 
 import static org.mockito.Mockito.mock;
@@ -162,6 +164,14 @@ class ClusterFixture {
         this.params.transitionTimes(transitionTimeMs);
     }
 
+    ClusterFixture markNodeAsConfigRetired(int nodeIndex) {
+        Set<ConfiguredNode> configuredNodes = new HashSet<>(cluster.getConfiguredNodes().values());
+        configuredNodes.remove(new ConfiguredNode(nodeIndex, false));
+        configuredNodes.add(new ConfiguredNode(nodeIndex, true));
+        cluster.setNodes(configuredNodes);
+        return this;
+    }
+
     AnnotatedClusterState annotatedGeneratedClusterState() {
         params.currentTimeInMilllis(timer.getCurrentTimeInMillis());
         return ClusterStateGenerator.generatedStateFrom(params);
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStateGeneratorTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStateGeneratorTest.java
index b1b219b58d4..958fc1ae232 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStateGeneratorTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/ClusterStateGeneratorTest.java
@@ -272,16 +272,29 @@ public class ClusterStateGeneratorTest {
 
     @Test
     public void config_retired_mode_is_reflected_in_generated_state() {
-        final ClusterFixture fixture = ClusterFixture.forFlatCluster(5).bringEntireClusterUp();
-        List<ConfiguredNode> nodes = DistributionBuilder.buildConfiguredNodes(5);
-        nodes.set(2, new ConfiguredNode(2, true));
-        fixture.cluster.setNodes(nodes);
+        ClusterFixture fixture = ClusterFixture.forFlatCluster(5)
+                .markNodeAsConfigRetired(2)
+                .bringEntireClusterUp();
 
-        final AnnotatedClusterState state = generateFromFixtureWithDefaultParams(fixture);
+        AnnotatedClusterState state = generateFromFixtureWithDefaultParams(fixture);
 
         assertThat(state.toString(), equalTo("distributor:5 storage:5 .2.s:r"));
     }
 
+    @Test
+    public void config_retired_mode_is_overridden_by_worse_wanted_state() {
+        ClusterFixture fixture = ClusterFixture.forFlatCluster(5)
+                .markNodeAsConfigRetired(2)
+                .markNodeAsConfigRetired(3)
+                .bringEntireClusterUp()
+                .proposeStorageNodeWantedState(2, State.DOWN)
+                .proposeStorageNodeWantedState(3, State.MAINTENANCE);
+
+        AnnotatedClusterState state = generateFromFixtureWithDefaultParams(fixture);
+
+        assertThat(state.toString(), equalTo("distributor:5 storage:5 .2.s:d .3.s:m"));
+    }
+
     private void do_test_change_within_node_transition_time_window_generates_maintenance(State reportedState) {
         final ClusterFixture fixture = ClusterFixture.forFlatCluster(5).bringEntireClusterUp();
         final ClusterStateGenerator.Params params = fixture.generatorParams()
diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeInfoTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeInfoTest.java
index c0253d8a126..e56d8b02cd4 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeInfoTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/NodeInfoTest.java
@@ -6,6 +6,7 @@ import com.yahoo.vdslib.state.NodeType;
 import com.yahoo.vdslib.state.State;
 import org.junit.Test;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -77,4 +78,32 @@ public class NodeInfoTest {
         assertFalse(nodeInfo.recentlyObservedUnstableDuringInit());
     }
 
+    @Test
+    public void down_wanted_state_overrides_config_retired_state() {
+        ClusterFixture fixture = ClusterFixture.forFlatCluster(3)
+                .markNodeAsConfigRetired(1)
+                .proposeStorageNodeWantedState(1, State.DOWN);
+
+        NodeInfo nodeInfo = fixture.cluster.getNodeInfo(new Node(NodeType.STORAGE, 1));
+        assertEquals(State.DOWN, nodeInfo.getWantedState().getState());
+    }
+
+    @Test
+    public void maintenance_wanted_state_overrides_config_retired_state() {
+        ClusterFixture fixture = ClusterFixture.forFlatCluster(3)
+                .markNodeAsConfigRetired(1)
+                .proposeStorageNodeWantedState(1, State.MAINTENANCE);
+
+        NodeInfo nodeInfo = fixture.cluster.getNodeInfo(new Node(NodeType.STORAGE, 1));
+        assertEquals(State.MAINTENANCE, nodeInfo.getWantedState().getState());
+    }
+
+    @Test
+    public void retired_state_overrides_default_up_wanted_state() {
+        final ClusterFixture fixture = ClusterFixture.forFlatCluster(3).markNodeAsConfigRetired(1);
+
+        NodeInfo nodeInfo = fixture.cluster.getNodeInfo(new Node(NodeType.STORAGE, 1));
+        assertEquals(State.RETIRED, nodeInfo.getWantedState().getState());
+    }
+
 }