Handle updating cluster of existing endpoint

3 files changed, 67 insertions, 93 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
index 1a135b8d9a2..53ef786d3f6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
@@ -2,12 +2,9 @@
 package com.yahoo.vespa.hosted.controller.rotation;
 
 import com.yahoo.config.application.api.DeploymentSpec;
-import com.yahoo.config.application.api.Endpoint;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
-import com.yahoo.config.provision.InstanceName;
-import com.yahoo.config.provision.RegionName;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
 import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.application.AssignedRotation;
@@ -16,15 +13,14 @@ import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.vespa.hosted.rotation.config.RotationsConfig;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.Set;
 import java.util.function.Function;
-import java.util.function.Predicate;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
@@ -135,76 +131,27 @@ public class RotationRepository {
             );
         }
 
-        Map<EndpointId, AssignedRotation> existingAssignments = existingEndpointAssignments(deploymentSpec, instance);
-        Map<EndpointId, AssignedRotation> updatedAssignments = assignRotationsToEndpoints(deploymentSpec, existingAssignments, instance.name(), lock);
-
-        existingAssignments.putAll(updatedAssignments);
-
-        return List.copyOf(existingAssignments.values());
+        return assignRotations(deploymentSpec, instance, lock);
     }
 
-    private Map<EndpointId, AssignedRotation> assignRotationsToEndpoints(DeploymentSpec deploymentSpec,
-                                                                         Map<EndpointId, AssignedRotation> existingAssignments,
-                                                                         InstanceName instance,
-                                                                         RotationLock lock) {
+    private List<AssignedRotation> assignRotations(DeploymentSpec deploymentSpec, Instance instance, RotationLock lock) {
         var availableRotations = new ArrayList<>(availableRotations(lock).values());
-
-        var neededRotations = deploymentSpec.requireInstance(instance).endpoints().stream()
-                                            .filter(Predicate.not(endpoint -> existingAssignments.containsKey(EndpointId.of(endpoint.endpointId()))))
-                                            .collect(Collectors.toSet());
-
-        if (neededRotations.size() > availableRotations.size()) {
-            throw new IllegalStateException("Hosted Vespa ran out of rotations, unable to assign rotation: need " + neededRotations.size() + ", have " + availableRotations.size());
+        var assignedRotationsByEndpointId = instance.rotations().stream()
+                                                    .collect(Collectors.toMap(AssignedRotation::endpointId,
+                                                                              Function.identity()));
+        var assignments = new ArrayList<AssignedRotation>();
+        for (var endpoint : deploymentSpec.requireInstance(instance.name()).endpoints()) {
+            var endpointId = EndpointId.of(endpoint.endpointId());
+            var assignedRotation = assignedRotationsByEndpointId.get(endpointId);
+            RotationId rotationId;
+            if (assignedRotation == null) { // No rotation is assigned to this endpoint
+                rotationId = requireNonEmpty(availableRotations).remove(0).id();
+            } else { // Rotation already assigned to this endpoint, reuse it
+                rotationId = assignedRotation.rotationId();
+            }
+            assignments.add(new AssignedRotation(ClusterSpec.Id.from(endpoint.containerId()), endpointId, rotationId, endpoint.regions()));
         }
-
-        return neededRotations.stream()
-                .map(endpoint -> {
-                        return new AssignedRotation(
-                                new ClusterSpec.Id(endpoint.containerId()),
-                                EndpointId.of(endpoint.endpointId()),
-                                availableRotations.remove(0).id(),
-                                endpoint.regions()
-                        );
-                })
-                .collect(
-                        Collectors.toMap(
-                                AssignedRotation::endpointId,
-                                Function.identity(),
-                                (a, b) -> { throw new IllegalStateException("Duplicate entries:" + a + ", " + b); },
-                                LinkedHashMap::new
-                        )
-                );
-    }
-
-    private Map<EndpointId, AssignedRotation> existingEndpointAssignments(DeploymentSpec deploymentSpec, Instance instance) {
-        // Get the regions that has been configured for an endpoint.  Empty set if the endpoint
-        // is no longer mentioned in the configuration file.
-        Function<EndpointId, Set<RegionName>> configuredRegionsForEndpoint = endpointId ->
-            deploymentSpec.requireInstance(instance.name()).endpoints().stream()
-                                 .filter(endpoint -> endpointId.id().equals(endpoint.endpointId()))
-                                 .map(Endpoint::regions)
-                                 .findFirst()
-                                 .orElse(Set.of());
-
-        // Build a new AssignedRotation instance where we update set of regions from the configuration instead
-        // of using the one already mentioned in the assignment.  This allows us to overwrite the set of regions.
-        Function<AssignedRotation, AssignedRotation> assignedRotationWithConfiguredRegions = assignedRotation ->
-            new AssignedRotation(
-                    assignedRotation.clusterId(),
-                    assignedRotation.endpointId(),
-                    assignedRotation.rotationId(),
-                    configuredRegionsForEndpoint.apply(assignedRotation.endpointId()));
-
-        return instance.rotations().stream()
-                       .collect(Collectors.toMap(
-                                          AssignedRotation::endpointId,
-                                          assignedRotationWithConfiguredRegions,
-                                          (a, b) -> {
-                                              throw new IllegalStateException("Duplicate entries: " + a + ", " + b);
-                                          },
-                                          LinkedHashMap::new
-                                  )
-                          );
+        return Collections.unmodifiableList(assignments);
     }
 
     /**
@@ -224,12 +171,8 @@ public class RotationRepository {
 
     private Rotation findAvailableRotation(ApplicationId id, RotationLock lock) {
         Map<RotationId, Rotation> availableRotations = availableRotations(lock);
-        if (availableRotations.isEmpty()) {
-            throw new IllegalStateException("Unable to assign global rotation to " + id
-                                            + " - no rotations available");
-        }
         // Return first available rotation
-        RotationId rotation = availableRotations.keySet().iterator().next();
+        RotationId rotation = requireNonEmpty(availableRotations.keySet()).iterator().next();
         log.info(String.format("Offering %s to application %s", rotation, id));
         return allRotations.get(rotation);
     }
@@ -246,4 +189,9 @@ public class RotationRepository {
                                                         Collections::unmodifiableMap));
     }
 
+    private static <T extends Collection<?>> T requireNonEmpty(T rotations) {
+        if (rotations.isEmpty()) throw new IllegalStateException("Hosted Vespa ran out of rotations, unable to assign rotation");
+        return rotations;
+    }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 78ea77fb549..875e067156d 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -9,6 +9,7 @@ import com.yahoo.config.application.api.ValidationOverrides;
 import com.yahoo.config.provision.AthenzDomain;
 import com.yahoo.config.provision.AthenzService;
 import com.yahoo.config.provision.CloudName;
+import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
@@ -27,7 +28,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordData;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName;
 import com.yahoo.vespa.hosted.controller.api.integration.routing.RoutingEndpoint;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
-import com.yahoo.vespa.hosted.controller.application.AssignedRotation;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.application.DeploymentMetrics;
 import com.yahoo.vespa.hosted.controller.application.Endpoint;
@@ -285,10 +285,10 @@ public class ControllerTest {
         var context = tester.newDeploymentContext("tenant1", "app1", "default");
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
                 .environment(Environment.prod)
-                .endpoint("foobar", "qrs", "us-west-1", "us-central-1")
-                .endpoint("default", "qrs", "us-west-1", "us-central-1")
-                .endpoint("all", "qrs")
-                .endpoint("west", "qrs", "us-west-1")
+                .endpoint("foobar", "qrs", "us-west-1", "us-central-1")  // Rotation 01
+                .endpoint("default", "qrs", "us-west-1", "us-central-1") // Rotation 02
+                .endpoint("all", "qrs")                                  // Rotation 03
+                .endpoint("west", "qrs", "us-west-1")                    // Rotation 04
                 .region("us-west-1")
                 .region("us-central-1")
                 .build();
@@ -300,9 +300,9 @@ public class ControllerTest {
         var notWest = Set.of(
                 "rotation-id-01", "foobar--app1--tenant1.global.vespa.oath.cloud",
                 "rotation-id-02", "app1--tenant1.global.vespa.oath.cloud",
-                "rotation-id-04", "all--app1--tenant1.global.vespa.oath.cloud"
+                "rotation-id-03", "all--app1--tenant1.global.vespa.oath.cloud"
         );
-        var west = Sets.union(notWest, Set.of("rotation-id-03", "west--app1--tenant1.global.vespa.oath.cloud"));
+        var west = Sets.union(notWest, Set.of("rotation-id-04", "west--app1--tenant1.global.vespa.oath.cloud"));
 
         for (Deployment deployment : deployments) {
             assertEquals("Rotation names are passed to config server in " + deployment.zone(),
@@ -316,7 +316,7 @@ public class ControllerTest {
         var record1 = tester.controllerTester().findCname("app1--tenant1.global.vespa.oath.cloud");
         assertTrue(record1.isPresent());
         assertEquals("app1--tenant1.global.vespa.oath.cloud", record1.get().name().asString());
-        assertEquals("rotation-fqdn-04.", record1.get().data().asString());
+        assertEquals("rotation-fqdn-02.", record1.get().data().asString());
 
         var record2 = tester.controllerTester().findCname("foobar--app1--tenant1.global.vespa.oath.cloud");
         assertTrue(record2.isPresent());
@@ -326,12 +326,12 @@ public class ControllerTest {
         var record3 = tester.controllerTester().findCname("all--app1--tenant1.global.vespa.oath.cloud");
         assertTrue(record3.isPresent());
         assertEquals("all--app1--tenant1.global.vespa.oath.cloud", record3.get().name().asString());
-        assertEquals("rotation-fqdn-02.", record3.get().data().asString());
+        assertEquals("rotation-fqdn-03.", record3.get().data().asString());
 
         var record4 = tester.controllerTester().findCname("west--app1--tenant1.global.vespa.oath.cloud");
         assertTrue(record4.isPresent());
         assertEquals("west--app1--tenant1.global.vespa.oath.cloud", record4.get().name().asString());
-        assertEquals("rotation-fqdn-03.", record4.get().data().asString());
+        assertEquals("rotation-fqdn-04.", record4.get().data().asString());
     }
 
     @Test
@@ -463,23 +463,20 @@ public class ControllerTest {
                 .environment(Environment.prod)
                 .endpoint("default", "qrs", "us-west-1", "us-central-1")
                 .region("us-west-1")
-                .region("us-central-1") // Two deployments should result in each DNS alias being registered once
+                .region("us-central-1")
                 .build();
         context.submit(applicationPackage).deploy();
 
         ApplicationPackage applicationPackage2 = new ApplicationPackageBuilder()
                 .environment(Environment.prod)
                 .region("us-west-1")
-                .region("us-central-1") // Two deployments should result in each DNS alias being registered once
+                .region("us-central-1")
                 .allow(ValidationId.globalEndpointChange)
                 .build();
 
         context.submit(applicationPackage2).deploy();
 
-        assertEquals(
-                List.of(AssignedRotation.fromStrings("qrs", "default", "rotation-id-01", Set.of())),
-                context.instance().rotations()
-        );
+        assertEquals(List.of(), context.instance().rotations());
 
         assertEquals(
                 Set.of(),
@@ -892,4 +889,33 @@ public class ControllerTest {
         assertEquals(Set.of(RoutingMethod.shared, RoutingMethod.sharedLayer4), routingMethods.get());
     }
 
+    @Test
+    public void testChangeEndpointCluster() {
+        var context = tester.newDeploymentContext();
+        var west = ZoneId.from("prod", "us-west-1");
+        var east = ZoneId.from("prod", "us-east-3");
+
+        // Deploy application
+        var applicationPackage = new ApplicationPackageBuilder()
+                .environment(Environment.prod)
+                .endpoint("default", "foo")
+                .region(west.region().value())
+                .region(east.region().value())
+                .build();
+        context.submit(applicationPackage).deploy();
+        assertEquals(ClusterSpec.Id.from("foo"), tester.applications().requireInstance(context.instanceId())
+                                                       .rotations().get(0).clusterId());
+
+        // Redeploy with endpoint cluster changed
+        applicationPackage = new ApplicationPackageBuilder()
+                .environment(Environment.prod)
+                .endpoint("default", "bar")
+                .region(west.region().value())
+                .region(east.region().value())
+                .build();
+        context.submit(applicationPackage).deploy();
+        assertEquals(ClusterSpec.Id.from("bar"), tester.applications().requireInstance(context.instanceId())
+                                                       .rotations().get(0).clusterId());
+    }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java
index b67422bc06b..0bcf25dd4b8 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepositoryTest.java
@@ -111,7 +111,7 @@ public class RotationRepositoryTest {
 
         // We're now out of rotations
         thrown.expect(IllegalStateException.class);
-        thrown.expectMessage("no rotations available");
+        thrown.expectMessage("out of rotations");
         var application3 = tester.newDeploymentContext("tenant3", "app3", "default");
         application3.submit(applicationPackage);
     }