Merge pull request #20290 from vespa-engine/mpolden/remove-inactive-deployment

Remove DNS record for inactive deployment in application endpoint

3 files changed, 68 insertions, 63 deletions

diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
index d98789591ab..699721b128c 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
@@ -240,13 +240,13 @@ public class DeploymentContext {
     public DeploymentContext addInactiveRoutingPolicy(ZoneId zone) {
         var clusterId = "default-inactive";
         var id = new RoutingPolicyId(instanceId, ClusterSpec.Id.from(clusterId), zone);
-        var policies = new LinkedHashMap<>(tester.controller().curator().readRoutingPolicies(instanceId));
+        var policies = new LinkedHashMap<>(tester.controller().routing().policies().read(instanceId).asMap());
         policies.put(id, new RoutingPolicy(id, HostName.from("lb-host"),
                                            Optional.empty(),
                                            Set.of(EndpointId.of("default")),
                                            Set.of(),
                                            new RoutingPolicy.Status(false, RoutingStatus.DEFAULT)));
-        tester.controller().curator().writeRoutingPolicies(instanceId, policies);
+        tester.controller().curator().writeRoutingPolicies(instanceId, List.copyOf(policies.values()));
         return this;
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
index 2e36b8969ba..422188420bd 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
@@ -1,19 +1,19 @@
 // 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.google.common.collect.ImmutableMap;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
-import com.yahoo.vespa.hosted.controller.routing.RoutingStatus;
 import com.yahoo.vespa.hosted.controller.routing.RoutingPolicy;
 import com.yahoo.vespa.hosted.controller.routing.RoutingPolicyId;
+import com.yahoo.vespa.hosted.controller.routing.RoutingStatus;
 import org.junit.Test;
 
 import java.time.Instant;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 
@@ -37,24 +37,24 @@ public class RoutingPolicySerializerTest {
         var id2 = new RoutingPolicyId(owner,
                                       ClusterSpec.Id.from("my-cluster2"),
                                       ZoneId.from("prod", "us-north-2"));
-        var policies = ImmutableMap.of(id1, new RoutingPolicy(id1,
-                                                              HostName.from("long-and-ugly-name"),
-                                                              Optional.of("zone1"),
-                                                              instanceEndpoints,
-                                                              applicationEndpoints,
-                                                              new RoutingPolicy.Status(true, RoutingStatus.DEFAULT)),
-                                       id2, new RoutingPolicy(id2,
-                                                              HostName.from("long-and-ugly-name-2"),
-                                                              Optional.empty(),
-                                                              instanceEndpoints,
-                                                              Set.of(),
-                                                              new RoutingPolicy.Status(false,
-                                                                                       new RoutingStatus(RoutingStatus.Value.out,
-                                                                                                         RoutingStatus.Agent.tenant,
-                                                                                                         Instant.ofEpochSecond(123)))));
+        var policies = List.of(new RoutingPolicy(id1,
+                                                 HostName.from("long-and-ugly-name"),
+                                                 Optional.of("zone1"),
+                                                 instanceEndpoints,
+                                                 applicationEndpoints,
+                                                 new RoutingPolicy.Status(true, RoutingStatus.DEFAULT)),
+                               new RoutingPolicy(id2,
+                                                 HostName.from("long-and-ugly-name-2"),
+                                                 Optional.empty(),
+                                                 instanceEndpoints,
+                                                 Set.of(),
+                                                 new RoutingPolicy.Status(false,
+                                                                          new RoutingStatus(RoutingStatus.Value.out,
+                                                                                            RoutingStatus.Agent.tenant,
+                                                                                            Instant.ofEpochSecond(123)))));
         var serialized = serializer.fromSlime(owner, serializer.toSlime(policies));
         assertEquals(policies.size(), serialized.size());
-        for (Iterator<RoutingPolicy> it1 = policies.values().iterator(), it2 = serialized.values().iterator(); it1.hasNext();) {
+        for (Iterator<RoutingPolicy> it1 = policies.iterator(), it2 = serialized.iterator(); it1.hasNext();) {
             var expected = it1.next();
             var actual = it2.next();
             assertEquals(expected.id(), actual.id());
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
index c40cb20a0bc..1919de33e8b 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
@@ -41,7 +41,6 @@ import java.time.Duration;
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
@@ -124,22 +123,33 @@ public class RoutingPoliciesTest {
         context2.submit(applicationPackage3).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
         tester.assertTargets(context2.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
 
+        // A deployment of app2 is removed
+        var applicationPackage4 = applicationPackageBuilder()
+                .region(zone1.region())
+                .endpoint("r0", "c0")
+                .allow(ValidationId.globalEndpointChange)
+                .allow(ValidationId.deploymentRemoval)
+                .build();
+        context2.submit(applicationPackage4).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+        tester.assertTargets(context2.instanceId(), EndpointId.of("r0"), 0, zone1);
+        assertEquals(1, tester.policiesOf(context2.instanceId()).size());
+
         // All endpoints for app1 are removed
-        ApplicationPackage applicationPackage4 = applicationPackageBuilder()
+        ApplicationPackage applicationPackage5 = applicationPackageBuilder()
                 .region(zone1.region())
                 .region(zone2.region())
                 .region(zone3.region())
                 .allow(ValidationId.globalEndpointChange)
                 .build();
-        context1.submit(applicationPackage4).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+        context1.submit(applicationPackage5).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
         tester.assertTargets(context1.instanceId(), EndpointId.of("r0"), 0);
         tester.assertTargets(context1.instanceId(), EndpointId.of("r1"), 0);
         tester.assertTargets(context1.instanceId(), EndpointId.of("r2"), 0);
         var policies = tester.policiesOf(context1.instanceId());
         assertEquals(clustersPerZone * numberOfDeployments, policies.size());
         assertTrue("Rotation membership is removed from all policies",
-                   policies.stream().allMatch(policy -> policy.instanceEndpoints().isEmpty()));
-        assertEquals("Rotations for " + context2.application() + " are not removed", 2, tester.aliasDataOf(endpoint4).size());
+                   policies.asList().stream().allMatch(policy -> policy.instanceEndpoints().isEmpty()));
+        assertEquals("Rotations for " + context2.application() + " are not removed", 1, tester.aliasDataOf(endpoint4).size());
     }
 
     @Test
@@ -306,8 +316,8 @@ public class RoutingPoliciesTest {
                 "c1.app1.tenant1.us-central-1.vespa.oath.cloud"
         );
         assertEquals(expectedRecords, tester.recordNames());
-        assertTrue("Removes stale routing policies " + context2.application(), tester.routingPolicies().get(context2.instanceId()).isEmpty());
-        assertEquals("Keeps routing policies for " + context1.application(), 4, tester.routingPolicies().get(context1.instanceId()).size());
+        assertTrue("Removes stale routing policies " + context2.application(), tester.routingPolicies().read(context2.instanceId()).isEmpty());
+        assertEquals("Keeps routing policies for " + context1.application(), 4, tester.routingPolicies().read(context1.instanceId()).size());
     }
 
     @Test
@@ -490,13 +500,13 @@ public class RoutingPoliciesTest {
         tester.assertTargets(context.instanceId(), EndpointId.of("r1"), 0, zone2);
 
         // Status details is stored in policy
-        var policy1 = tester.routingPolicies().get(context.deploymentIdIn(zone1)).values().iterator().next();
+        var policy1 = tester.routingPolicies().read(context.deploymentIdIn(zone1)).first().get();
         assertEquals(RoutingStatus.Value.out, policy1.status().routingStatus().value());
         assertEquals(RoutingStatus.Agent.tenant, policy1.status().routingStatus().agent());
         assertEquals(changedAt.truncatedTo(ChronoUnit.MILLIS), policy1.status().routingStatus().changedAt());
 
         // Other zone remains in
-        var policy2 = tester.routingPolicies().get(context.deploymentIdIn(zone2)).values().iterator().next();
+        var policy2 = tester.routingPolicies().read(context.deploymentIdIn(zone2)).first().get();
         assertEquals(RoutingStatus.Value.in, policy2.status().routingStatus().value());
         assertEquals(RoutingStatus.Agent.system, policy2.status().routingStatus().agent());
         assertEquals(Instant.EPOCH, policy2.status().routingStatus().changedAt());
@@ -515,7 +525,7 @@ public class RoutingPoliciesTest {
         tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
         tester.assertTargets(context.instanceId(), EndpointId.of("r1"), 0, zone1, zone2);
 
-        policy1 = tester.routingPolicies().get(context.deploymentIdIn(zone1)).values().iterator().next();
+        policy1 = tester.routingPolicies().read(context.deploymentIdIn(zone1)).first().get();
         assertEquals(RoutingStatus.Value.in, policy1.status().routingStatus().value());
         assertEquals(RoutingStatus.Agent.tenant, policy1.status().routingStatus().agent());
         assertEquals(changedAt.truncatedTo(ChronoUnit.MILLIS), policy1.status().routingStatus().changedAt());
@@ -568,9 +578,9 @@ public class RoutingPoliciesTest {
         tester.assertTargets(context1.instanceId(), EndpointId.defaultId(), 0, zone1);
         tester.assertTargets(context2.instanceId(), EndpointId.defaultId(), 0, zone1);
         for (var context : contexts) {
-            var policies = tester.routingPolicies().get(context.instanceId());
+            var policies = tester.routingPolicies().read(context.instanceId());
             assertTrue("Global routing status for policy remains " + RoutingStatus.Value.in,
-                       policies.values().stream()
+                       policies.asList().stream()
                                .map(RoutingPolicy::status)
                                .map(RoutingPolicy.Status::routingStatus)
                                .map(RoutingStatus::value)
@@ -668,7 +678,7 @@ public class RoutingPoliciesTest {
         // Setting zone (containing active deployment) out puts all deployments in
         tester.routingPolicies().setRoutingStatus(zone1, RoutingStatus.Value.out);
         context.flushDnsUpdates();
-        assertEquals(RoutingStatus.Value.out, tester.routingPolicies().get(zone1).routingStatus().value());
+        assertEquals(RoutingStatus.Value.out, tester.routingPolicies().read(zone1).routingStatus().value());
         tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, ImmutableMap.of(zone1, 0L, zone2, 0L));
 
         // Setting zone back in removes the currently inactive deployment
@@ -680,7 +690,7 @@ public class RoutingPoliciesTest {
         tester.routingPolicies().setRoutingStatus(context.deploymentIdIn(zone2), RoutingStatus.Value.in,
                                                   RoutingStatus.Agent.tenant);
         context.flushDnsUpdates();
-        for (var policy : tester.routingPolicies().get(context.instanceId()).values()) {
+        for (var policy : tester.routingPolicies().read(context.instanceId())) {
             assertSame(RoutingStatus.Value.in, policy.status().routingStatus().value());
         }
         tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
@@ -693,7 +703,7 @@ public class RoutingPoliciesTest {
         RecordName name = RecordName.from("cfg.prod.us-west-1.test.vip");
 
         tester.provisionLoadBalancers(1, app, zone1);
-        tester.routingPolicies().refresh(app, DeploymentSpec.empty, zone1);
+        tester.routingPolicies().refresh(new DeploymentId(app, zone1), DeploymentSpec.empty);
         new NameServiceDispatcher(tester.tester.controller(), Duration.ofSeconds(Integer.MAX_VALUE)).run();
 
         List<Record> records = tester.controllerTester().nameService().findRecords(Record.Type.CNAME, name);
@@ -793,18 +803,12 @@ public class RoutingPoliciesTest {
         var applicationPackage = applicationPackageBuilder()
                 .instances("beta,main")
                 .region(zone1.region())
-                .region(zone2.region())
                 .applicationEndpoint("a0", "c0", "us-west-1",
                                      Map.of(betaInstance.instance(), 2,
                                             mainInstance.instance(), 8))
-                .applicationEndpoint("a1", "c1", "us-central-1",
-                                     Map.of(betaInstance.instance(), 4,
-                                            mainInstance.instance(), 0))
                 .build();
-        for (var zone : List.of(zone1, zone2)) {
-            tester.provisionLoadBalancers(2, betaInstance, zone);
-            tester.provisionLoadBalancers(2, mainInstance, zone);
-        }
+        tester.provisionLoadBalancers(1, betaInstance, zone1);
+        tester.provisionLoadBalancers(1, mainInstance, zone1);
 
         // Deploy both instances
         betaContext.submit(applicationPackage).deploy();
@@ -812,35 +816,36 @@ public class RoutingPoliciesTest {
         // Application endpoint points to both instances with correct weights
         DeploymentId betaZone1 = betaContext.deploymentIdIn(zone1);
         DeploymentId mainZone1 = mainContext.deploymentIdIn(zone1);
-        DeploymentId betaZone2 = betaContext.deploymentIdIn(zone2);
-        DeploymentId mainZone2 = mainContext.deploymentIdIn(zone2);
         tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
                              Map.of(betaZone1, 2,
                                     mainZone1, 8));
-        tester.assertTargets(application, EndpointId.of("a1"), ClusterSpec.Id.from("c1"), 1,
-                             Map.of(betaZone2, 4,
-                                    mainZone2, 0));
 
-        // Changing routing status updates weight
+        // Changing routing status removes deployment from DNS
         tester.routingPolicies().setRoutingStatus(mainZone1, RoutingStatus.Value.out, RoutingStatus.Agent.tenant);
         betaContext.flushDnsUpdates();
         tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
-                             Map.of(betaZone1, 2,
-                                    mainZone1, 0));
-        tester.routingPolicies().setRoutingStatus(mainZone1, RoutingStatus.Value.in, RoutingStatus.Agent.tenant);
+                             Map.of(betaZone1, 2));
+
+        // Changing routing status for remaining deployment adds back all deployments, because removing all deployments
+        // puts all IN
+        tester.routingPolicies().setRoutingStatus(betaZone1, RoutingStatus.Value.out, RoutingStatus.Agent.tenant);
         betaContext.flushDnsUpdates();
         tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
                              Map.of(betaZone1, 2,
                                     mainZone1, 8));
 
-        // Changing routing status preserves weights if change in routing status would result in a zero weight sum
-        // Otherwise this would result in both targets have weight 0 and thus traffic would be distributed evenly across
-        // all targets which does not match intention of taking out a deployment
-        tester.routingPolicies().setRoutingStatus(betaZone2, RoutingStatus.Value.out, RoutingStatus.Agent.tenant);
+        // Activating main deployment allows us to deactivate the beta deployment
+        tester.routingPolicies().setRoutingStatus(mainZone1, RoutingStatus.Value.in, RoutingStatus.Agent.tenant);
         betaContext.flushDnsUpdates();
-        tester.assertTargets(application, EndpointId.of("a1"), ClusterSpec.Id.from("c1"), 1,
-                             Map.of(betaZone2, 4,
-                                    mainZone2, 0));
+        tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
+                             Map.of(mainZone1, 8));
+
+        // Activate all deployments again
+        tester.routingPolicies().setRoutingStatus(betaZone1, RoutingStatus.Value.in, RoutingStatus.Agent.tenant);
+        betaContext.flushDnsUpdates();
+        tester.assertTargets(application, EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
+                             Map.of(betaZone1, 2,
+                                    mainZone1, 8));
     }
 
     /** Returns an application package builder that satisfies requirements for a directly routed endpoint */
@@ -936,8 +941,8 @@ public class RoutingPoliciesTest {
             provisionLoadBalancers(clustersPerZone, application, false, zones);
         }
 
-        private Collection<RoutingPolicy> policiesOf(ApplicationId instance) {
-            return tester.controller().curator().readRoutingPolicies(instance).values();
+        private RoutingPolicyList policiesOf(ApplicationId instance) {
+            return tester.controller().routing().policies().read(instance);
         }
 
         private Set<String> recordNames() {
@@ -995,8 +1000,8 @@ public class RoutingPoliciesTest {
             for (var zone : zoneWeights.keySet()) {
                 DeploymentId deployment = new DeploymentId(instance, zone);
                 EndpointList regionEndpoints = tester.controller().routing().readEndpointsOf(deployment)
-                                                    .cluster(cluster)
-                                                    .scope(Endpoint.Scope.weighted);
+                                                     .cluster(cluster)
+                                                     .scope(Endpoint.Scope.weighted);
                 Endpoint regionEndpoint = regionEndpoints.first().orElseThrow(() -> new IllegalArgumentException("No region endpoint found for " + cluster + " in " + deployment));
                 zonesByRegionEndpoint.computeIfAbsent(regionEndpoint.dnsName(), (k) -> new ArrayList<>())
                                      .add(zone);