Avoid maintaining routing policies for non-active load balancers

When deactivating a deployment, the config server moves the load balancer to
inactive. Since the LB was still present, we kept its routing policy (and DNS
record) even though both should've been removed.

5 files changed, 30 insertions, 46 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 da982fa67a8..967a821c506 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
@@ -15,7 +15,6 @@ import com.yahoo.security.KeyUtils;
 import com.yahoo.security.SignatureAlgorithm;
 import com.yahoo.security.X509CertificateBuilder;
 import com.yahoo.vespa.hosted.controller.Application;
-import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.ControllerTester;
 import com.yahoo.vespa.hosted.controller.Instance;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
@@ -53,7 +52,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
-import java.util.OptionalLong;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Supplier;
@@ -270,22 +268,6 @@ public class DeploymentContext {
         }
     }
 
-    /** Add a routing policy for this in given zone, with status set to inactive */
-    public DeploymentContext addInactiveRoutingPolicy(ZoneId zone) {
-        var clusterId = "default-inactive";
-        var id = new RoutingPolicyId(instanceId, Id.from(clusterId), zone);
-        var policies = new LinkedHashMap<>(tester.controller().routing().policies().read(instanceId).asMap());
-        policies.put(id, new RoutingPolicy(id, Optional.of(HostName.of("lb-host")),
-                                           Optional.empty(),
-                                           Optional.empty(),
-                                           Set.of(EndpointId.of("default")),
-                                           Set.of(),
-                                           new RoutingPolicy.Status(false, RoutingStatus.DEFAULT),
-                                           true));
-        tester.controller().curator().writeRoutingPolicies(instanceId, List.copyOf(policies.values()));
-        return this;
-    }
-
     /** Submit given application package for deployment */
     public DeploymentContext resubmit(ApplicationPackage applicationPackage) {
         return submit(applicationPackage, Optional.of(defaultSourceRevision), salt.get(), 0);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
index d542c06b5bf..fe74e305b63 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ConfigServerMock.java
@@ -103,7 +103,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     private final Map<DeploymentId, TestReport> testReport = new HashMap<>();
     private final Map<DeploymentId, CloudAccount> cloudAccounts = new HashMap<>();
     private final Map<DeploymentId, List<X509Certificate>> additionalCertificates = new HashMap<>();
-    private List<SearchNodeMetrics> searchnodeMetrics;
+    private List<SearchNodeMetrics> searchNodeMetrics;
 
     private Version lastPrepareVersion = null;
     private Consumer<ApplicationId> prepareException = null;
@@ -308,7 +308,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
     }
 
     public void setProtonMetrics(List<SearchNodeMetrics> searchnodeMetrics) {
-        this.searchnodeMetrics = searchnodeMetrics;
+        this.searchNodeMetrics = searchnodeMetrics;
     }
 
     public void deferLoadBalancerProvisioningIn(Set<Environment> environments) {
@@ -499,7 +499,15 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
 
         applications.remove(deployment);
         serviceStatus.remove(deployment);
-        removeLoadBalancers(deployment.applicationId(), deployment.zoneId());
+
+        // This simulates what a real config server does: It deactivates the LB. Actual removal happens in the background
+        loadBalancers.computeIfPresent(deployment.zoneId(), (k, old) ->
+                old.stream().map(lb -> lb.application().equals(deployment.applicationId())
+                           ? new LoadBalancer(lb.id(), lb.application(), lb.cluster(), lb.hostname(), lb.ipAddress(),
+                                              LoadBalancer.State.inactive, lb.dnsZone(), lb.cloudAccount(),
+                                              lb.service(), lb.isPublic())
+                           : lb)
+                   .collect(Collectors.toCollection(LinkedHashSet::new)));
     }
 
     @Override
@@ -509,7 +517,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer
 
     @Override
     public List<SearchNodeMetrics> getSearchNodeMetrics(DeploymentId deployment) {
-        return this.searchnodeMetrics;
+        return this.searchNodeMetrics;
     }
 
     @Override
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 8e0b1dd1d4e..c1267ad5edf 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
@@ -43,7 +43,7 @@ public class RoutingPolicySerializerTest {
                                                  Optional.of("zone1"),
                                                  Set.of(),
                                                  Set.of(),
-                                                 new RoutingPolicy.Status(true, RoutingStatus.DEFAULT),
+                                                 RoutingStatus.DEFAULT,
                                                  false),
                                new RoutingPolicy(id2,
                                                  Optional.of(HostName.of("long-and-ugly-name-2")),
@@ -51,10 +51,9 @@ public class RoutingPolicySerializerTest {
                                                  Optional.empty(),
                                                  instanceEndpoints,
                                                  Set.of(),
-                                                 new RoutingPolicy.Status(false,
-                                                                          new RoutingStatus(RoutingStatus.Value.out,
-                                                                                            RoutingStatus.Agent.tenant,
-                                                                                            Instant.ofEpochSecond(123))),
+                                                 new RoutingStatus(RoutingStatus.Value.out,
+                                                                   RoutingStatus.Agent.tenant,
+                                                                   Instant.ofEpochSecond(123)),
                                                  true),
                                new RoutingPolicy(id1,
                                                  Optional.empty(),
@@ -62,7 +61,7 @@ public class RoutingPolicySerializerTest {
                                                  Optional.of("zone2"),
                                                  instanceEndpoints,
                                                  applicationEndpoints,
-                                                 new RoutingPolicy.Status(true, RoutingStatus.DEFAULT),
+                                                 RoutingStatus.DEFAULT,
                                                  true));
         var serialized = serializer.fromSlime(owner, serializer.toSlime(policies));
         assertEquals(policies.size(), serialized.size());
@@ -75,7 +74,7 @@ public class RoutingPolicySerializerTest {
             assertEquals(expected.dnsZone(), actual.dnsZone());
             assertEquals(expected.instanceEndpoints(), actual.instanceEndpoints());
             assertEquals(expected.applicationEndpoints(), actual.applicationEndpoints());
-            assertEquals(expected.status(), actual.status());
+            assertEquals(expected.routingStatus(), actual.routingStatus());
             assertEquals(expected.isPublic(), actual.isPublic());
         }
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index 0d1c2c5487f..ac16aa727d5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -1625,7 +1625,6 @@ public class ApplicationApiTest extends ControllerContainerTest {
                 .region(zone.region().value())
                 .build();
         app.submit(applicationPackage).deploy();
-        app.addInactiveRoutingPolicy(zone);
 
         // GET application
         tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance1", GET)
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 24d5e02240d..772877de8e3 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
@@ -309,10 +309,7 @@ public class RoutingPoliciesTest {
 
         // Remove app2 completely
         tester.controllerTester().controller().applications().requireInstance(context2.instanceId()).deployments().keySet()
-              .forEach(zone -> {
-                  tester.controllerTester().configServer().removeLoadBalancers(context2.instanceId(), zone);
-                  tester.controllerTester().controller().applications().deactivate(context2.instanceId(), zone);
-              });
+              .forEach(zone -> tester.controllerTester().controller().applications().deactivate(context2.instanceId(), zone));
         context2.flushDnsUpdates();
         expectedRecords = Set.of(
                 "c0.app1.tenant1.us-west-1.vespa.oath.cloud",
@@ -573,15 +570,15 @@ public class RoutingPoliciesTest {
 
         // Status details is stored in policy
         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());
+        assertEquals(RoutingStatus.Value.out, policy1.routingStatus().value());
+        assertEquals(RoutingStatus.Agent.tenant, policy1.routingStatus().agent());
+        assertEquals(changedAt.truncatedTo(ChronoUnit.MILLIS), policy1.routingStatus().changedAt());
 
         // Other zone remains in
         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());
+        assertEquals(RoutingStatus.Value.in, policy2.routingStatus().value());
+        assertEquals(RoutingStatus.Agent.system, policy2.routingStatus().agent());
+        assertEquals(Instant.EPOCH, policy2.routingStatus().changedAt());
 
         // Next deployment does not affect status
         context.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
@@ -598,9 +595,9 @@ public class RoutingPoliciesTest {
         tester.assertTargets(context.instanceId(), EndpointId.of("r1"), 0, zone1, zone2);
 
         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());
+        assertEquals(RoutingStatus.Value.in, policy1.routingStatus().value());
+        assertEquals(RoutingStatus.Agent.tenant, policy1.routingStatus().agent());
+        assertEquals(changedAt.truncatedTo(ChronoUnit.MILLIS), policy1.routingStatus().changedAt());
 
         // Deployment is set out through a new deployment.xml
         var applicationPackage2 = applicationPackageBuilder()
@@ -652,8 +649,7 @@ public class RoutingPoliciesTest {
         for (var context : contexts) {
             var policies = tester.routingPolicies().read(context.instanceId());
             assertTrue(policies.asList().stream()
-                            .map(RoutingPolicy::status)
-                            .map(RoutingPolicy.Status::routingStatus)
+                            .map(RoutingPolicy::routingStatus)
                             .map(RoutingStatus::value)
                             .allMatch(status -> status == RoutingStatus.Value.in),
                     "Global routing status for policy remains " + RoutingStatus.Value.in);
@@ -763,7 +759,7 @@ public class RoutingPoliciesTest {
                 RoutingStatus.Agent.tenant);
         context.flushDnsUpdates();
         for (var policy : tester.routingPolicies().read(context.instanceId())) {
-            assertSame(RoutingStatus.Value.in, policy.status().routingStatus().value());
+            assertSame(RoutingStatus.Value.in, policy.routingStatus().value());
         }
         tester.assertTargets(context.instanceId(), EndpointId.of("r0"), 0, zone1, zone2);
     }