From 1e55cb6ec6422ad9f3e2b7b656756f3cf1d148b4 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Tue, 6 Jun 2023 15:08:42 +0200 Subject: 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. --- .../controller/deployment/DeploymentContext.java | 18 -------------- .../controller/integration/ConfigServerMock.java | 16 +++++++++---- .../persistence/RoutingPolicySerializerTest.java | 13 +++++----- .../restapi/application/ApplicationApiTest.java | 1 - .../controller/routing/RoutingPoliciesTest.java | 28 ++++++++++------------ 5 files changed, 30 insertions(+), 46 deletions(-) (limited to 'controller-server/src/test') 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 testReport = new HashMap<>(); private final Map cloudAccounts = new HashMap<>(); private final Map> additionalCertificates = new HashMap<>(); - private List searchnodeMetrics; + private List searchNodeMetrics; private Version lastPrepareVersion = null; private Consumer prepareException = null; @@ -308,7 +308,7 @@ public class ConfigServerMock extends AbstractComponent implements ConfigServer } public void setProtonMetrics(List searchnodeMetrics) { - this.searchnodeMetrics = searchnodeMetrics; + this.searchNodeMetrics = searchnodeMetrics; } public void deferLoadBalancerProvisioningIn(Set 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 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); } -- cgit v1.2.3