Control maintenance jobs with feature flag

7 files changed, 50 insertions, 76 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
index 9e4600a1bdb..9750c322242 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Controller.java
@@ -23,6 +23,7 @@ import com.yahoo.vespa.hosted.controller.deployment.JobController;
 import com.yahoo.vespa.hosted.controller.dns.NameServiceForwarder;
 import com.yahoo.vespa.hosted.controller.metric.ConfigServerMetrics;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
+import com.yahoo.vespa.hosted.controller.persistence.JobControlFlags;
 import com.yahoo.vespa.hosted.controller.security.AccessControl;
 import com.yahoo.vespa.hosted.controller.versions.ControllerVersion;
 import com.yahoo.vespa.hosted.controller.versions.OsVersion;
@@ -112,7 +113,7 @@ public class Controller extends AbstractComponent {
         tenantController = new TenantController(this, curator, accessControl);
         routingController = new RoutingController(this, Objects.requireNonNull(rotationsConfig, "RotationsConfig cannot be null"));
         auditLogger = new AuditLogger(curator, clock);
-        jobControl = new JobControl(curator);
+        jobControl = new JobControl(new JobControlFlags(curator, flagSource));
         this.controllerConfig = controllerConfig;
 
         // Record the version of this controller
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
index 3058037ccc0..0a9dd2e55a6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
@@ -5,8 +5,6 @@ import com.google.common.util.concurrent.UncheckedTimeoutException;
 import com.google.inject.Inject;
 import com.yahoo.collections.Pair;
 import com.yahoo.component.Version;
-import com.yahoo.concurrent.maintenance.JobControl;
-import com.yahoo.concurrent.maintenance.StringSetSerializer;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.TenantName;
@@ -44,7 +42,6 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.NavigableMap;
@@ -72,7 +69,7 @@ import static java.util.stream.Collectors.toUnmodifiableList;
  * @author mpolden
  * @author jonmv
  */
-public class CuratorDb implements JobControl.Db {
+public class CuratorDb {
 
     private static final Logger log = Logger.getLogger(CuratorDb.class.getName());
     private static final Duration deployLockTimeout = Duration.ofMinutes(30);
@@ -89,7 +86,6 @@ public class CuratorDb implements JobControl.Db {
     private static final Path zoneRoutingPoliciesRoot = root.append("zoneRoutingPolicies");
     private static final Path endpointCertificateRoot = root.append("applicationCertificates");
 
-    private final StringSetSerializer stringSetSerializer = new StringSetSerializer();
     private final NodeVersionSerializer nodeVersionSerializer = new NodeVersionSerializer();
     private final VersionStatusSerializer versionStatusSerializer = new VersionStatusSerializer(nodeVersionSerializer);
     private final ControllerVersionSerializer controllerVersionSerializer = new ControllerVersionSerializer();
@@ -159,12 +155,6 @@ public class CuratorDb implements JobControl.Db {
         return curator.lock(lockRoot.append("confidenceOverrides"), defaultLockTimeout);
     }
 
-    @Override
-    public Lock lockInactiveJobs() {
-        return curator.lock(lockRoot.append("inactiveJobsLock"), defaultLockTimeout);
-    }
-
-    @Override
     public Lock lockMaintenanceJob(String jobName) {
         try {
             return tryLock(lockRoot.append("maintenanceJobLocks").append(jobName));
@@ -229,25 +219,6 @@ public class CuratorDb implements JobControl.Db {
         }
     }
 
-    // -------------- Maintenance jobs ----------------------------------------
-
-    @Override
-    public Set<String> readInactiveJobs() {
-        try {
-            return read(inactiveJobsPath(), stringSetSerializer::fromJson).orElseGet(HashSet::new);
-        }
-        catch (RuntimeException e) {
-            log.log(Level.WARNING, "Error reading inactive jobs, deleting inactive state");
-            writeInactiveJobs(Collections.emptySet());
-            return new HashSet<>();
-        }
-    }
-
-    @Override
-    public void writeInactiveJobs(Set<String> inactiveJobs) {
-        curator.set(inactiveJobsPath(), stringSetSerializer.toJson(inactiveJobs));
-    }
-
     // -------------- Deployment orchestration --------------------------------
 
     public double readUpgradesPerMinute() {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/JobControlFlags.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/JobControlFlags.java
new file mode 100644
index 00000000000..1cb23d0c515
--- /dev/null
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/JobControlFlags.java
@@ -0,0 +1,37 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.controller.persistence;
+
+import com.yahoo.concurrent.maintenance.JobControlState;
+import com.yahoo.transaction.Mutex;
+import com.yahoo.vespa.flags.FlagSource;
+import com.yahoo.vespa.flags.Flags;
+import com.yahoo.vespa.flags.ListFlag;
+
+import java.util.Set;
+
+/**
+ * An implementation of {@link JobControlState} that uses a feature flag to control maintenance jobs.
+ *
+ * @author mpolden
+ */
+public class JobControlFlags implements JobControlState {
+
+    private final CuratorDb curator;
+    private final ListFlag<String> inactiveJobsFlag;
+
+    public JobControlFlags(CuratorDb curator, FlagSource flagSource) {
+        this.curator = curator;
+        this.inactiveJobsFlag = Flags.INACTIVE_MAINTENANCE_JOBS.bindTo(flagSource);
+    }
+
+    @Override
+    public Set<String> readInactiveJobs() {
+        return Set.copyOf(inactiveJobsFlag.value());
+    }
+
+    @Override
+    public Mutex lockMaintenanceJob(String job) {
+        return curator.lockMaintenanceJob(job);
+    }
+
+}
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java
index 8b1beb91b83..10c7452a6af 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiHandler.java
@@ -6,16 +6,15 @@ import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.container.jdisc.LoggingRequestHandler;
 import com.yahoo.io.IOUtils;
+import com.yahoo.restapi.ErrorResponse;
 import com.yahoo.restapi.Path;
+import com.yahoo.restapi.ResourceResponse;
 import com.yahoo.slime.Inspector;
 import com.yahoo.slime.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLoggingRequestHandler;
 import com.yahoo.vespa.hosted.controller.maintenance.ControllerMaintenance;
 import com.yahoo.vespa.hosted.controller.maintenance.Upgrader;
-import com.yahoo.restapi.ErrorResponse;
-import com.yahoo.restapi.MessageResponse;
-import com.yahoo.restapi.ResourceResponse;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence;
 import com.yahoo.yolean.Exceptions;
 
@@ -76,14 +75,12 @@ public class ControllerApiHandler extends AuditLoggingRequestHandler {
 
     private HttpResponse post(HttpRequest request) {
         Path path = new Path(request.getUri());
-        if (path.matches("/controller/v1/maintenance/inactive/{jobName}")) return setActive(path.get("jobName"), false);
         if (path.matches("/controller/v1/jobs/upgrader/confidence/{version}")) return overrideConfidence(request, path.get("version"));
         return notFound(path);
     }
 
     private HttpResponse delete(HttpRequest request) {
         Path path = new Path(request.getUri());
-        if (path.matches("/controller/v1/maintenance/inactive/{jobName}")) return setActive(path.get("jobName"), true);
         if (path.matches("/controller/v1/jobs/upgrader/confidence/{version}")) return removeConfidenceOverride(path.get("version"));
         return notFound(path);
     }
@@ -100,14 +97,6 @@ public class ControllerApiHandler extends AuditLoggingRequestHandler {
         return new ResourceResponse(request, "auditlog", "jobs/upgrader", "maintenance");
     }
 
-    private HttpResponse setActive(String jobName, boolean active) {
-        boolean activatingInactiveJob = active && !controller.jobControl().isActive(jobName);
-        if (!activatingInactiveJob && !controller.jobControl().jobs().contains(jobName))
-            return ErrorResponse.notFoundError("No job named '" + jobName + "'");
-        controller.jobControl().setActive(jobName, active);
-        return new MessageResponse((active ? "Re-activated" : "Deactivated" ) + " job '" + jobName + "'");
-    }
-
     private HttpResponse configureUpgrader(HttpRequest request) {
         String upgradesPerMinuteField = "upgradesPerMinute";
         String targetMajorVersionField = "targetMajorVersion";
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/JobsResponse.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/JobsResponse.java
index df259b2c6f3..0ac90349eaf 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/JobsResponse.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/controller/JobsResponse.java
@@ -6,6 +6,8 @@ import com.yahoo.restapi.SlimeJsonResponse;
 import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Slime;
 
+import java.util.TreeSet;
+
 /**
  * A response containing maintenance job status
  *
@@ -26,7 +28,7 @@ public class JobsResponse extends SlimeJsonResponse {
             jobArray.addObject().setString("name", jobName);
 
         Cursor inactiveArray = root.setArray("inactive");
-        for (String jobName : jobControl.inactiveJobs())
+        for (String jobName : new TreeSet<>(jobControl.inactiveJobs()))
             inactiveArray.addString(jobName);
 
         return slime;
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
index 993fb919594..41d10015411 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
@@ -19,6 +19,7 @@ import com.yahoo.vespa.hosted.controller.maintenance.JobRunnerTest;
 import com.yahoo.vespa.hosted.controller.maintenance.OutstandingChangeDeployer;
 import com.yahoo.vespa.hosted.controller.maintenance.ReadyJobsTrigger;
 import com.yahoo.vespa.hosted.controller.maintenance.Upgrader;
+import com.yahoo.vespa.hosted.controller.persistence.JobControlFlags;
 
 import java.time.DayOfWeek;
 import java.time.Duration;
@@ -76,7 +77,7 @@ public class DeploymentTester {
         tester = controllerTester;
         jobs = tester.controller().jobController();
         cloud = (MockTesterCloud) tester.controller().jobController().cloud();
-        var jobControl = new JobControl(tester.controller().curator());
+        var jobControl = new JobControl(new JobControlFlags(tester.controller().curator(), tester.controller().flagSource()));
         runner = new JobRunner(tester.controller(), Duration.ofDays(1),
                                JobRunnerTest.inThreadExecutor(), new InternalStepRunner(tester.controller()));
         upgrader = new Upgrader(tester.controller(), maintenanceInterval, tester.curator());
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java
index 8029d650b75..c061bfed21a 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/controller/ControllerApiTest.java
@@ -6,6 +6,8 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.test.ManualClock;
+import com.yahoo.vespa.flags.Flags;
+import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.controller.api.integration.resource.ResourceSnapshot;
 import com.yahoo.vespa.hosted.controller.auditlog.AuditLogger;
 import com.yahoo.vespa.hosted.controller.restapi.ContainerTester;
@@ -19,7 +21,6 @@ import java.nio.charset.StandardCharsets;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.List;
-import java.util.Set;
 
 import static org.junit.Assert.assertFalse;
 
@@ -41,39 +42,11 @@ public class ControllerApiTest extends ControllerContainerTest {
     public void testControllerApi() {
         tester.assertResponse(authenticatedRequest("http://localhost:8080/controller/v1/", "", Request.Method.GET), new File("root.json"));
 
-        // POST deactivates a maintenance job
-        tester.assertResponse(operatorRequest("http://localhost:8080/controller/v1/maintenance/inactive/DeploymentExpirer",
-                                            "", Request.Method.POST),
-                       "{\"message\":\"Deactivated job 'DeploymentExpirer'\"}", 200);
+        ((InMemoryFlagSource) tester.controller().flagSource()).withListFlag(Flags.INACTIVE_MAINTENANCE_JOBS.id(), List.of("DeploymentExpirer"), String.class);
 
         // GET a list of all maintenance jobs
         tester.assertResponse(authenticatedRequest("http://localhost:8080/controller/v1/maintenance/", "", Request.Method.GET),
                               new File("maintenance.json"));
-
-        // DELETE activates maintenance job
-        tester.assertResponse(operatorRequest("http://localhost:8080/controller/v1/maintenance/inactive/DeploymentExpirer",
-                                            "", Request.Method.DELETE),
-                       "{\"message\":\"Re-activated job 'DeploymentExpirer'\"}",
-                              200);
-
-        // DELETE fails to activate unknown maintenance job
-        tester.assertResponse(operatorRequest("http://localhost:8080/controller/v1/maintenance/inactive/foo",
-                                                    "", Request.Method.DELETE),
-                              "{\"error-code\":\"NOT_FOUND\",\"message\":\"No job named 'foo'\"}",
-                              404);
-
-        // DELETE clears inactive flag for maintenance job that has been removed from the code base
-        tester.controller().curator().writeInactiveJobs(Set.of("bar"));
-        tester.assertResponse(operatorRequest("http://localhost:8080/controller/v1/maintenance/inactive/bar",
-                                                    "", Request.Method.DELETE),
-                              "{\"message\":\"Re-activated job 'bar'\"}",
-                              200);
-        tester.assertResponse(operatorRequest("http://localhost:8080/controller/v1/maintenance/inactive/bar",
-                                                    "", Request.Method.DELETE),
-                              "{\"error-code\":\"NOT_FOUND\",\"message\":\"No job named 'bar'\"}",
-                              404);
-
-        assertFalse("Actions are logged to audit log", tester.controller().auditLogger().readLog().entries().isEmpty());
     }
 
     @Test