Fix issue with rotations changing after assignment

- Make sure we only include production zones in the assignment
- Always run deployment through rotation assignment logic, so regions are cleared
  from persisted assignments even if there are no remaining rotations in the configuration.
- Add tests to expose this logic.
- Add tiny convenience method for AssignedRotation to create them in tests.

3 files changed, 143 insertions, 18 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 0756d3006ea..ae8a81caa17 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -462,7 +462,7 @@ public class ApplicationController {
 
     /** Makes sure the application has a global rotation, if eligible. */
     private LockedApplication withRotation(LockedApplication application, ZoneId zone) {
-        if (zone.environment() == Environment.prod && applicationNeedsRotations(application.get().deploymentSpec())) {
+        if (zone.environment() == Environment.prod) {
             try (RotationLock rotationLock = rotationRepository.lock()) {
                 final var rotations = rotationRepository.getOrAssignRotations(application.get(), rotationLock);
                 application = application.with(rotations);
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 7165d134d59..340a08e0ec6 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,14 @@
 package com.yahoo.vespa.hosted.controller.rotation;
 
 import com.yahoo.collections.Pair;
+import com.yahoo.config.application.api.Endpoint;
+import com.yahoo.config.model.api.ContainerEndpoint;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
+import com.yahoo.config.provision.RegionName;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
 import com.yahoo.vespa.hosted.controller.application.AssignedRotation;
-import com.yahoo.vespa.hosted.controller.application.Endpoint;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.vespa.hosted.rotation.config.RotationsConfig;
@@ -20,7 +22,9 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 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;
 
@@ -96,6 +100,7 @@ public class RotationRepository {
         // Support the older case of setting global-service-id
         if (application.deploymentSpec().globalServiceId().isPresent()) {
             final var regions = application.deploymentSpec().zones().stream()
+                    .filter(zone -> zone.environment().isProduction())
                     .flatMap(zone -> zone.region().stream())
                     .collect(Collectors.toSet());
 
@@ -112,31 +117,70 @@ public class RotationRepository {
         }
 
         final var availableRotations = new ArrayList<>(availableRotations(lock).values());
-        final var assignments = application.assignedRotations().stream()
+        final Map<EndpointId, AssignedRotation> existingAssignments = existingEndpointAssignments(application);
+        final Map<EndpointId, AssignedRotation> updatedAssignments = assignRotationsToEndpoints(application, existingAssignments, availableRotations);
+
+        existingAssignments.putAll(updatedAssignments);
+
+        return List.copyOf(existingAssignments.values());
+    }
+
+    private Map<EndpointId, AssignedRotation> assignRotationsToEndpoints(Application application, Map<EndpointId, AssignedRotation> existingAssignments, List<Rotation> availableRotations) {
+        return application.deploymentSpec().endpoints().stream()
+                .filter(Predicate.not(endpoint -> existingAssignments.containsKey(EndpointId.of(endpoint.endpointId()))))
+                .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); },
+                                (a, b) -> { throw new IllegalStateException("Duplicate entries:" + a + ", " + b); },
                                 LinkedHashMap::new
                         )
                 );
+    }
 
-        application.deploymentSpec().endpoints().stream()
-                .filter(endpoint -> ! assignments.containsKey(new EndpointId(endpoint.endpointId())))
-                .map(endpoint -> {
-                    return new AssignedRotation(
-                            new ClusterSpec.Id(endpoint.containerId()),
-                            EndpointId.of(endpoint.endpointId()),
-                            availableRotations.remove(0).id(),
-                            endpoint.regions()
-                    );
-                })
-                .forEach(assignment -> {
-                    assignments.put(assignment.endpointId(), assignment);
-                });
+    private Map<EndpointId, AssignedRotation> existingEndpointAssignments(Application application) {
+        //
+        // Get the regions that has been configured for an endpoint.  Empty set if the endpoint
+        // is no longer mentioned in the configuration file.
+        //
+        final Function<EndpointId, Set<RegionName>> configuredRegionsForEndpoint = endpointId -> {
+            return application.deploymentSpec().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
+        // when
+        final Function<AssignedRotation, AssignedRotation> assignedRotationWithConfiguredRegions = assignedRotation -> {
+            return new AssignedRotation(
+                    assignedRotation.clusterId(),
+                    assignedRotation.endpointId(),
+                    assignedRotation.rotationId(),
+                    configuredRegionsForEndpoint.apply(assignedRotation.endpointId())
+            );
+        };
 
-        return List.copyOf(assignments.values());
+        return application.assignedRotations().stream()
+                .collect(
+                        Collectors.toMap(
+                                AssignedRotation::endpointId,
+                                assignedRotationWithConfiguredRegions,
+                                (a, b) -> { throw new IllegalStateException("Duplicate entries: " + a + ", " + b); },
+                                LinkedHashMap::new
+                        )
+                );
     }
 
     /**
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 3a905f8c6d5..6e7ca423444 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
@@ -7,6 +7,7 @@ import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.application.api.ValidationOverrides;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ApplicationName;
+import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
@@ -23,6 +24,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.deployment.SourceRevisi
 import com.yahoo.vespa.hosted.controller.api.integration.dns.Record;
 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.DeploymentJobs.JobError;
 import com.yahoo.vespa.hosted.controller.application.DeploymentMetrics;
@@ -344,6 +346,85 @@ public class ControllerTest {
     }
 
     @Test
+    public void testDnsAliasRegistrationWithChangingZones() {
+        Application application = tester.createApplication("app1", "tenant1", 1, 1L);
+
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .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
+                .build();
+
+        tester.deployCompletely(application, applicationPackage);
+
+        assertEquals(
+                Set.of("rotation-id-01", "app1--tenant1.global.vespa.oath.cloud"),
+                tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-west-1")))
+        );
+
+        assertEquals(
+                Set.of("rotation-id-01", "app1--tenant1.global.vespa.oath.cloud"),
+                tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-central-1")))
+        );
+
+
+        ApplicationPackage applicationPackage2 = new ApplicationPackageBuilder()
+                .environment(Environment.prod)
+                .endpoint("default", "qrs", "us-west-1")
+                .region("us-west-1")
+                .region("us-central-1") // Two deployments should result in each DNS alias being registered once
+                .build();
+
+        tester.deployCompletely(application, applicationPackage2, BuildJob.defaultBuildNumber + 1);
+
+        assertEquals(
+                Set.of("rotation-id-01", "app1--tenant1.global.vespa.oath.cloud"),
+                tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-west-1")))
+        );
+
+        assertEquals(
+                Set.of(),
+                tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-central-1")))
+        );
+
+        assertEquals(Set.of(RegionName.from("us-west-1")), tester.application(application.id()).assignedRotations().get(0).regions());
+    }
+
+    @Test
+    public void testUnassignRotations() {
+        Application application = tester.createApplication("app1", "tenant1", 1, 1L);
+
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .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
+                .build();
+
+        tester.deployCompletely(application, applicationPackage);
+
+        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
+                .build();
+
+        tester.deployCompletely(application, applicationPackage2, BuildJob.defaultBuildNumber + 1);
+
+
+        assertEquals(
+                List.of(AssignedRotation.fromStrings("qrs", "default", "rotation-id-01", Set.of())),
+                tester.application(application.id()).assignedRotations()
+        );
+
+        assertEquals(
+                Set.of(),
+                tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-west-1")))
+        );
+    }
+
+        @Test
     public void testUpdatesExistingDnsAlias() {
         // Application 1 is deployed and deleted
         {