Merge pull request #24064 from vespa-engine/mpolden/one-lock-scheme

Use only most recent lock scheme

2 files changed, 45 insertions, 72 deletions

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 ff374c9bc8d..cb814a30c22 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
@@ -14,10 +14,6 @@ import com.yahoo.slime.Slime;
 import com.yahoo.slime.SlimeUtils;
 import com.yahoo.transaction.Mutex;
 import com.yahoo.vespa.curator.Curator;
-import com.yahoo.vespa.curator.MultiplePathsLock;
-import com.yahoo.vespa.flags.FlagSource;
-import com.yahoo.vespa.flags.Flags;
-import com.yahoo.vespa.flags.StringFlag;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.api.identifiers.ControllerVersion;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
@@ -53,7 +49,6 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
-import java.util.Locale;
 import java.util.Map;
 import java.util.NavigableMap;
 import java.util.Optional;
@@ -119,7 +114,6 @@ public class CuratorDb {
 
     private final Curator curator;
     private final Duration tryLockTimeout;
-    private final StringFlag lockScheme;
 
     // For each application id (path), store the ZK node version and its deserialised data - update when version changes.
     // This will grow to keep all applications in memory, but this should be OK
@@ -128,15 +122,43 @@ public class CuratorDb {
     // For each job id (path), store the ZK node version and its deserialised data - update when version changes.
     private final Map<Path, Pair<Integer, NavigableMap<RunId, Run>>> cachedHistoricRuns = new ConcurrentHashMap<>();
 
+    private static final Set<String> knownLockRootChildren = Set.of(
+            "applications",
+            "archiveBuckets",
+            "auditLog",
+            "changeRequests",
+            "confidenceOverrides",
+            "deploymentRetriggerQueue",
+            "instances",
+            "jobs",
+            "maintenanceJobLocks",
+            "meteringRefreshTime",
+            "nameServiceQueue",
+            "notifications",
+            "osTargetVersion",
+            "osVersionStatus",
+            "provisioning",
+            "rotations",
+            "routingPolicies",
+            "steps",
+            "supportAccess",
+            "tenants"
+    );
+
     @Inject
-    public CuratorDb(Curator curator, FlagSource flagSource) {
-        this(curator, defaultTryLockTimeout, flagSource);
+    public CuratorDb(Curator curator) {
+        this(curator, defaultTryLockTimeout);
     }
 
-    CuratorDb(Curator curator, Duration tryLockTimeout, FlagSource flagSource) {
+    CuratorDb(Curator curator, Duration tryLockTimeout) {
         this.curator = curator;
         this.tryLockTimeout = tryLockTimeout;
-        this.lockScheme = Flags.CONTROLLER_LOCK_SCHEME.bindTo(flagSource);
+        // TODO(mpolden): Remove after 2022-09-26. This cleans up the immediate children of lock root which are no longer used
+        for (var path : curator.getChildren(lockRoot)) {
+            if (!knownLockRootChildren.contains(path)) {
+                curator.delete(lockRoot.append(path));
+            }
+        }
     }
 
     /** Returns all hostnames configured to be part of this ZooKeeper cluster */
@@ -149,47 +171,29 @@ public class CuratorDb {
 
     // -------------- Locks ---------------------------------------------------
 
-    private enum LockScheme {
-
-        OLD, BOTH, NEW;
-
-        boolean isLegacy() { return this == OLD; }
-
-    }
-
-    /** Acquire the appropriate lock according to the lock scheme set by feature flag */
-    private Mutex lock(Function<LockScheme, Path> pathFactory, Duration timeout) {
-        LockScheme scheme = LockScheme.valueOf(lockScheme.value().toUpperCase(Locale.ENGLISH));
-        return switch (scheme) {
-            case OLD, NEW -> curator.lock(pathFactory.apply(scheme), timeout);
-            case BOTH -> new MultiplePathsLock(curator.lock(pathFactory.apply(LockScheme.OLD), timeout),
-                                               curator.lock(pathFactory.apply(LockScheme.NEW), timeout));
-        };
-    }
-
     public Mutex lock(TenantName name) {
-        return lock(scheme -> lockPath(name, scheme.isLegacy()), defaultLockTimeout.multipliedBy(2));
+        return curator.lock(lockRoot.append("tenants").append(name.value()), defaultLockTimeout.multipliedBy(2));
     }
 
     public Mutex lock(TenantAndApplicationId id) {
-        return lock(scheme -> lockPath(id, scheme.isLegacy()), defaultLockTimeout.multipliedBy(2));
+        return curator.lock(lockRoot.append("applications").append(id.tenant().value() + ":" +
+                                                                   id.application().value()),
+                            defaultLockTimeout.multipliedBy(2));
     }
 
     public Mutex lockForDeployment(ApplicationId id, ZoneId zone) {
-        return lock(scheme -> lockPath(id, zone, scheme.isLegacy()), deployLockTimeout);
+        return curator.lock(lockRoot.append("instances").append(id.serializedForm() + ":" + zone.environment().value() +
+                                                                ":" + zone.region().value()),
+                            deployLockTimeout);
     }
 
     public Mutex lock(ApplicationId id, JobType type) {
-        return lock(scheme -> lockPath(id, type, scheme.isLegacy()), defaultLockTimeout);
+        return curator.lock(lockRoot.append("jobs").append(id.serializedForm() + ":" + type.jobName()),
+                            defaultLockTimeout);
     }
 
     public Mutex lock(ApplicationId id, JobType type, Step step) throws TimeoutException {
-        try {
-            // TODO(mpolden): Revert to tryLock once lock scheme is removed
-            return lock(scheme -> lockPath(id, type, step, scheme.isLegacy()), tryLockTimeout);
-        } catch (UncheckedTimeoutException e) {
-            throw new TimeoutException(e.getMessage());
-        }
+        return tryLock(lockRoot.append("steps").append(id.serializedForm() + ":" + type.jobName() + ":" + step.name()));
     }
 
     public Mutex lockRotations() {
@@ -209,7 +213,7 @@ public class CuratorDb {
     }
 
     public Mutex lockProvisionState(String provisionStateId) {
-        return curator.lock(lockPath(provisionStateId), Duration.ofSeconds(1));
+        return curator.lock(lockRoot.append(provisionStatePath()).append(provisionStateId), Duration.ofSeconds(1));
     }
 
     public Mutex lockOsVersions() {
@@ -682,36 +686,6 @@ public class CuratorDb {
 
     // -------------- Paths ---------------------------------------------------
 
-    private Path lockPath(TenantName tenant, boolean legacy) {
-        Path currentRoot = legacy ? lockRoot : lockRoot.append("tenants");
-        return currentRoot.append(tenant.value());
-    }
-
-    private Path lockPath(TenantAndApplicationId application, boolean legacy) {
-        Path currentRoot = legacy ? lockRoot : lockRoot.append("applications");
-        return currentRoot.append(application.tenant().value() + ":" + application.application().value());
-    }
-
-    private Path lockPath(ApplicationId instance, ZoneId zone, boolean legacy) {
-        Path currentRoot = legacy ? lockRoot : lockRoot.append("instances");
-        return currentRoot.append(instance.serializedForm() + ":" + zone.environment().value() + ":" + zone.region().value());
-    }
-
-    private Path lockPath(ApplicationId instance, JobType type, boolean legacy) {
-        Path currentRoot = legacy ? lockRoot : lockRoot.append("jobs");
-        return currentRoot.append(instance.serializedForm() + ":" + type.jobName());
-    }
-
-    private Path lockPath(ApplicationId instance, JobType type, Step step, boolean legacy) {
-        Path currentRoot = legacy ? lockRoot : lockRoot.append("steps");
-        return currentRoot.append(instance.serializedForm() + ":" + type.jobName() + ":" + step.name());
-    }
-
-    private Path lockPath(String provisionId) {
-        return lockRoot.append(provisionStatePath())
-                       .append(provisionId);
-    }
-
     private static Path upgradesPerMinutePath() {
         return root.append("upgrader").append("upgradesPerMinute");
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MockCuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MockCuratorDb.java
index 85feb97f39b..3ec639a5529 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MockCuratorDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MockCuratorDb.java
@@ -1,11 +1,10 @@
 // Copyright Yahoo. 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.component.annotation.Inject;
 import com.yahoo.cloud.config.ConfigserverConfig;
+import com.yahoo.component.annotation.Inject;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.vespa.curator.mock.MockCurator;
-import com.yahoo.vespa.flags.InMemoryFlagSource;
 
 import java.time.Duration;
 
@@ -33,7 +32,7 @@ public class MockCuratorDb extends CuratorDb {
     }
 
     public MockCuratorDb(MockCurator curator) {
-        super(curator, Duration.ofMillis(100), new InMemoryFlagSource());
+        super(curator, Duration.ofMillis(100));
         this.curator = curator;
     }