Support setting PERMANENTLY_DOWN at end of retirement

7 files changed, 68 insertions, 27 deletions

diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java
index 17dfb924973..eb6a4119f8a 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorContext.java
@@ -30,6 +30,7 @@ public class OrchestratorContext implements AutoCloseable {
     private final TimeBudget timeBudget;
     private final boolean probe;
     private final boolean largeLocks;
+    private final boolean usePermanentlyDownStatus;
 
     // The key set is the set of applications locked by this context tree: Only the
     // root context has a non-empty set. The value is an unlock callback to be called
@@ -38,24 +39,32 @@ public class OrchestratorContext implements AutoCloseable {
 
     /** Create an OrchestratorContext for operations on multiple applications. */
     public static OrchestratorContext createContextForMultiAppOp(Clock clock, boolean largeLocks) {
-        return new OrchestratorContext(null, clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_BATCH_OP), false, largeLocks);
+        return new OrchestratorContext(null, clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_BATCH_OP),
+                false, largeLocks, false);
     }
 
     /** Create an OrchestratorContext for an operation on a single application. */
     public static OrchestratorContext createContextForSingleAppOp(Clock clock) {
-        return new OrchestratorContext(null, clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_SINGLE_OP), false, false);
+        return createContextForSingleAppOp(clock, false);
+    }
+
+    public static OrchestratorContext createContextForSingleAppOp(Clock clock, boolean usePermanentlyDownStatus) {
+        return new OrchestratorContext(null, clock, TimeBudget.fromNow(clock, DEFAULT_TIMEOUT_FOR_SINGLE_OP),
+                false, false, usePermanentlyDownStatus);
     }
 
     private OrchestratorContext(OrchestratorContext parentOrNull,
                                 Clock clock,
                                 TimeBudget timeBudget,
                                 boolean probe,
-                                boolean largeLocks) {
+                                boolean largeLocks,
+                                boolean usePermanentlyDownStatus) {
         this.parent = Optional.ofNullable(parentOrNull);
         this.clock = clock;
         this.timeBudget = timeBudget;
         this.probe = probe;
         this.largeLocks = largeLocks;
+        this.usePermanentlyDownStatus = usePermanentlyDownStatus;
     }
 
     public Duration getTimeLeft() {
@@ -74,6 +83,9 @@ public class OrchestratorContext implements AutoCloseable {
     /** Whether application locks acquired during probing of a batch suspend should be closed after the non-probe is done. */
     public boolean largeLocks() { return largeLocks; }
 
+    /** Whether the PERMANENTLY_DOWN host status should be used (where appropriate). */
+    public boolean usePermanentlyDownStatus() { return usePermanentlyDownStatus; }
+
     /**
      * Returns true if 1. large locks is enabled, and 2.
      * {@link #registerLockAcquisition(ApplicationInstanceReference, Runnable) registerLockAcquisition}
@@ -111,7 +123,7 @@ public class OrchestratorContext implements AutoCloseable {
         // Move deadline towards past by a fixed amount to ensure there's time to process exceptions and
         // access ZooKeeper before the lock times out.
         TimeBudget subTimeBudget = timeBudget.withDeadline(timeBudget.deadline().get().minus(TIMEOUT_OVERHEAD));
-        return new OrchestratorContext(this, clock, subTimeBudget, probe, largeLocks);
+        return new OrchestratorContext(this, clock, subTimeBudget, probe, largeLocks, usePermanentlyDownStatus);
     }
 
     /** Create an OrchestratorContext for an operation on a single application, but limited to current timeout. */
@@ -124,7 +136,7 @@ public class OrchestratorContext implements AutoCloseable {
         }
 
         TimeBudget timeBudget = TimeBudget.from(clock, now, Optional.of(Duration.between(now, deadline)));
-        return new OrchestratorContext(this, clock, timeBudget, probe, largeLocks);
+        return new OrchestratorContext(this, clock, timeBudget, probe, largeLocks, usePermanentlyDownStatus);
     }
 
     @Override
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
index 244fd4d9b5d..414548f8bdc 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
@@ -65,6 +65,7 @@ public class OrchestratorImpl implements Orchestrator {
     private final Clock clock;
     private final ApplicationApiFactory applicationApiFactory;
     private final BooleanFlag enableLargeOrchestratorLocks;
+    private final BooleanFlag retireWithPermanentlyDownFlag;
 
     @Inject
     public OrchestratorImpl(ClusterControllerClientFactory clusterControllerClientFactory,
@@ -101,6 +102,7 @@ public class OrchestratorImpl implements Orchestrator {
         this.clock = clock;
         this.applicationApiFactory = applicationApiFactory;
         this.enableLargeOrchestratorLocks = Flags.ENABLE_LARGE_ORCHESTRATOR_LOCKS.bindTo(flagSource);
+        this.retireWithPermanentlyDownFlag = Flags.RETIRE_WITH_PERMANENTLY_DOWN.bindTo(flagSource);
     }
 
     @Override
@@ -150,6 +152,12 @@ public class OrchestratorImpl implements Orchestrator {
         * monitoring will have had time to catch up. Since we don't want do the delay with the lock held,
         * and the host status service's locking functionality does not support something like condition
         * variables or Object.wait(), we break out here, releasing the lock before delaying.
+        *
+        * 2020-02-07: We should utilize suspendedSince timestamp on the HostInfo: The above
+        * is equivalent to guaranteeing a minimum time after suspendedSince, before checking
+        * the health with service monitor. This should for all practical purposes remove
+        * the amount of time in this sleep.
+        * Caveat: Cannot be implemented before lingering HostInfo has been fixed (VESPA-17546).
         */
         sleep(serviceMonitorConvergenceLatencySeconds, TimeUnit.SECONDS);
 
@@ -159,15 +167,21 @@ public class OrchestratorImpl implements Orchestrator {
         try (MutableStatusRegistry statusRegistry = statusService
                 .lockApplicationInstance_forCurrentThreadOnly(context, appInstance.reference())) {
             HostStatus currentHostState = statusRegistry.getHostInfo(hostName).status();
-
-            if (HostStatus.NO_REMARKS == currentHostState) {
+            if (currentHostState == HostStatus.NO_REMARKS) {
                 return;
             }
 
-            ApplicationInstanceStatus appStatus = statusRegistry.getStatus();
-            if (appStatus == ApplicationInstanceStatus.NO_REMARKS) {
-                policy.releaseSuspensionGrant(context.createSubcontextWithinLock(), appInstance, hostName, statusRegistry);
+            // In 2 cases the resume will appear to succeed (no exception thrown),
+            // but the host status and content cluster states will not be changed accordingly:
+            //  1. When host is permanently down: the host will be removed from the application asap.
+            //  2. The whole application is down: the content cluster states are set to maintenance,
+            //     and the host may be taken down manually at any moment.
+            if (currentHostState == HostStatus.PERMANENTLY_DOWN ||
+                    statusRegistry.getStatus() == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) {
+                return;
             }
+
+            policy.releaseSuspensionGrant(context.createSubcontextWithinLock(), appInstance, hostName, statusRegistry);
         }
     }
 
@@ -183,7 +197,10 @@ public class OrchestratorImpl implements Orchestrator {
         ApplicationInstance appInstance = getApplicationInstance(hostName);
         NodeGroup nodeGroup = new NodeGroup(appInstance, hostName);
 
-        OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock);
+        boolean usePermanentlyDownStatus = retireWithPermanentlyDownFlag
+                .with(FetchVector.Dimension.HOSTNAME, hostName.s())
+                .value();
+        OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock, usePermanentlyDownStatus);
         try (MutableStatusRegistry statusRegistry = statusService
                 .lockApplicationInstance_forCurrentThreadOnly(context, appInstance.reference())) {
             ApplicationApi applicationApi = applicationApiFactory.create(nodeGroup, statusRegistry,
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApi.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApi.java
index 2e85713d323..a6353e39610 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApi.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApi.java
@@ -8,6 +8,7 @@ import com.yahoo.vespa.orchestrator.status.ApplicationInstanceStatus;
 import com.yahoo.vespa.orchestrator.status.HostStatus;
 
 import java.util.List;
+import java.util.function.Predicate;
 
 /**
  * The API a Policy has access to
@@ -29,10 +30,12 @@ public interface ApplicationApi {
     ApplicationInstanceStatus getApplicationStatus();
 
     void setHostState(OrchestratorContext context, HostName hostName, HostStatus status);
-    List<HostName> getNodesInGroupWithStatus(HostStatus status);
+    List<HostName> getNodesInGroupWith(Predicate<HostStatus> statusPredicate);
+    default List<HostName> getNodesInGroupWithStatus(HostStatus requiredStatus) {
+        return getNodesInGroupWith(status -> status == requiredStatus);
+    }
 
     List<StorageNode> getStorageNodesInGroupInClusterOrder();
     List<StorageNode> getUpStorageNodesInGroupInClusterOrder();
-    List<StorageNode> getStorageNodesAllowedToBeDownInGroupInReverseClusterOrder();
-
+    List<StorageNode> getSuspendedStorageNodesInGroupInReverseClusterOrder();
 }
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java
index def5edd0a97..cf6946fa7f8 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java
@@ -20,6 +20,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 import static com.yahoo.vespa.orchestrator.OrchestratorUtil.getHostsUsedByApplicationInstance;
@@ -62,10 +63,9 @@ public class ApplicationApiImpl implements ApplicationApi {
     }
 
     @Override
-    public List<StorageNode> getStorageNodesAllowedToBeDownInGroupInReverseClusterOrder() {
+    public List<StorageNode> getSuspendedStorageNodesInGroupInReverseClusterOrder() {
         return getStorageNodesInGroupInClusterOrder().stream()
-                // PERMANENTLY_DOWN nodes are NOT included.
-                .filter(storageNode -> getHostStatus(storageNode.hostName()) == HostStatus.ALLOWED_TO_BE_DOWN)
+                .filter(storageNode -> getHostStatus(storageNode.hostName()).isSuspended())
                 .sorted(Comparator.reverseOrder())
                 .collect(Collectors.toList());
     }
@@ -104,9 +104,9 @@ public class ApplicationApiImpl implements ApplicationApi {
     }
 
     @Override
-    public List<HostName> getNodesInGroupWithStatus(HostStatus status) {
+    public List<HostName> getNodesInGroupWith(Predicate<HostStatus> statusPredicate) {
         return nodeGroup.getHostNames().stream()
-                .filter(hostName -> getHostStatus(hostName) == status)
+                .filter(hostName -> statusPredicate.test(getHostStatus(hostName)))
                 .collect(Collectors.toList());
     }
 
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java
index a9b9736ebfb..f6a1e4f91f0 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicy.java
@@ -30,7 +30,9 @@ public class HostedVespaPolicy implements Policy {
     private final ClusterControllerClientFactory clusterControllerClientFactory;
     private final ApplicationApiFactory applicationApiFactory;
 
-    public HostedVespaPolicy(HostedVespaClusterPolicy clusterPolicy, ClusterControllerClientFactory clusterControllerClientFactory, ApplicationApiFactory applicationApiFactory) {
+    public HostedVespaPolicy(HostedVespaClusterPolicy clusterPolicy,
+                             ClusterControllerClientFactory clusterControllerClientFactory,
+                             ApplicationApiFactory applicationApiFactory) {
         this.clusterPolicy = clusterPolicy;
         this.clusterControllerClientFactory = clusterControllerClientFactory;
         this.applicationApiFactory = applicationApiFactory;
@@ -60,10 +62,11 @@ public class HostedVespaPolicy implements Policy {
     public void releaseSuspensionGrant(OrchestratorContext context, ApplicationApi application)
             throws HostStateChangeDeniedException {
         // Always defer to Cluster Controller whether it's OK to resume storage node
-        for (StorageNode storageNode : application.getStorageNodesAllowedToBeDownInGroupInReverseClusterOrder()) {
+        for (StorageNode storageNode : application.getSuspendedStorageNodesInGroupInReverseClusterOrder()) {
             storageNode.setNodeState(context, ClusterControllerNodeState.UP);
         }
 
+        // In particular, we're not modifying the state of PERMANENTLY_DOWN nodes.
         for (HostName hostName : application.getNodesInGroupWithStatus(HostStatus.ALLOWED_TO_BE_DOWN)) {
             application.setHostState(context, hostName, HostStatus.NO_REMARKS);
         }
@@ -92,9 +95,15 @@ public class HostedVespaPolicy implements Policy {
             storageNode.setNodeState(context, ClusterControllerNodeState.DOWN);
         }
 
-        // Ensure all nodes in the group are marked as allowed to be down
-        for (HostName hostName : applicationApi.getNodesInGroupWithStatus(HostStatus.NO_REMARKS)) {
-            applicationApi.setHostState(context, hostName, HostStatus.ALLOWED_TO_BE_DOWN);
+        if (context.usePermanentlyDownStatus()) {
+            // Ensure all nodes in the group are marked as permanently down
+            for (HostName hostName : applicationApi.getNodesInGroupWith(status -> status != HostStatus.PERMANENTLY_DOWN)) {
+                applicationApi.setHostState(context, hostName, HostStatus.PERMANENTLY_DOWN);
+            }
+        } else {
+            for (HostName hostName : applicationApi.getNodesInGroupWith(status -> status != HostStatus.ALLOWED_TO_BE_DOWN)) {
+                applicationApi.setHostState(context, hostName, HostStatus.ALLOWED_TO_BE_DOWN);
+            }
         }
     }
 
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java
index d5734a73de0..ee33a92367b 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImplTest.java
@@ -327,7 +327,7 @@ public class ApplicationApiImplTest {
     private void verifyStorageNodesAllowedToBeDown(
             ApplicationApi applicationApi, HostName... hostNames) {
         List<HostName> actualStorageNodes =
-                applicationApi.getStorageNodesAllowedToBeDownInGroupInReverseClusterOrder().stream()
+                applicationApi.getSuspendedStorageNodesInGroupInReverseClusterOrder().stream()
                 .map(storageNode -> storageNode.hostName())
                 .collect(Collectors.toList());
         assertEquals(Arrays.asList(hostNames), actualStorageNodes);
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicyTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicyTest.java
index b27e37ac034..ed6917a3a4e 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicyTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/policy/HostedVespaPolicyTest.java
@@ -120,7 +120,7 @@ public class HostedVespaPolicyTest {
         when(applicationApi.getStorageNodesInGroupInClusterOrder()).thenReturn(upStorageNodes);
 
         List<HostName> noRemarksHostNames = Arrays.asList(hostName1, hostName2, hostName3);
-        when(applicationApi.getNodesInGroupWithStatus(HostStatus.NO_REMARKS)).thenReturn(noRemarksHostNames);
+        when(applicationApi.getNodesInGroupWith(any())).thenReturn(noRemarksHostNames);
 
         InOrder order = inOrder(applicationApi, clusterPolicy, storageNode1, storageNode3);
 
@@ -136,7 +136,7 @@ public class HostedVespaPolicyTest {
         order.verify(storageNode1).setNodeState(context, ClusterControllerNodeState.DOWN);
         order.verify(storageNode3).setNodeState(context, ClusterControllerNodeState.DOWN);
 
-        order.verify(applicationApi).getNodesInGroupWithStatus(HostStatus.NO_REMARKS);
+        order.verify(applicationApi).getNodesInGroupWith(any());
         order.verify(applicationApi).setHostState(context, hostName1, HostStatus.ALLOWED_TO_BE_DOWN);
         order.verify(applicationApi).setHostState(context, hostName2, HostStatus.ALLOWED_TO_BE_DOWN);
         order.verify(applicationApi).setHostState(context, hostName3, HostStatus.ALLOWED_TO_BE_DOWN);