diff options
5 files changed, 118 insertions, 15 deletions
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java index 4ca9ee1dc2f..6f77dce8fc5 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java @@ -281,6 +281,7 @@ public class DeploymentSpecXmlReader { List<Endpoint.Target> targets = new ArrayList<>(); if (level == Endpoint.Level.application) { String region = requireStringAttribute("region", endpointElement); + int weightSum = 0; for (var instanceElement : XML.getChildren(endpointElement, "instance")) { String instanceName = instanceElement.getTextContent(); String weightFromAttribute = requireStringAttribute("weight", instanceElement); @@ -291,10 +292,12 @@ public class DeploymentSpecXmlReader { } catch (NumberFormatException e) { throw new IllegalArgumentException(msgPrefix + "invalid weight value '" + weightFromAttribute + "'"); } + weightSum += weight; targets.add(new Endpoint.Target(RegionName.from(region), InstanceName.from(instanceName), weight)); } + if (weightSum == 0) illegal(msgPrefix + "sum of all weights must be positive, got " + weightSum); } else { if (stringAttribute("region", endpointElement).isPresent()) illegal(msgPrefix + "invalid 'region' attribute"); for (var regionElement : XML.getChildren(endpointElement, "region")) { diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java index 8f9c9bccfcd..74e79d5e8cf 100644 --- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java +++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java @@ -1200,7 +1200,7 @@ public class DeploymentSpecTest { } @Test - public void applicationLevelEndpointRequiresAttributes() { + public void applicationLevelEndpointValidation() { String xmlForm = "<deployment>\n" + " <instance id=\"beta\">\n" + " <prod>\n" + @@ -1226,6 +1226,7 @@ public class DeploymentSpecTest { assertInvalid(String.format(xmlForm, "region='invalid'", "weight='1'", "main", ""), "Application-level endpoint 'foo': targets undeclared region 'invalid' in instance 'main'"); assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='foo'", "main", ""), "Application-level endpoint 'foo': invalid weight value 'foo'"); assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='1'", "main", "<region>us-east-3</region>"), "Application-level endpoint 'foo': invalid element 'region'"); + assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='0'", "main", ""), "Application-level endpoint 'foo': sum of all weights must be positive, got 0"); } @Test diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java index a11ea88a6a8..2a39ed08014 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java @@ -3,10 +3,12 @@ package com.yahoo.vespa.hosted.controller.routing; import com.yahoo.config.application.api.DeploymentSpec; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.zone.RoutingMethod; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.curator.Lock; +import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import com.yahoo.vespa.hosted.controller.api.integration.configserver.LoadBalancer; @@ -28,6 +30,7 @@ import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -208,10 +211,14 @@ public class RoutingPolicies { // application-level endpoint. However, to allow this in the future the routing table remains // Map<RoutingId, List<RoutingPolicy>> instead of Map<RoutingId, RoutingPolicy>. Map<RoutingId, List<RoutingPolicy>> routingTable = applicationRoutingTable(routingPolicies); + if (routingTable.isEmpty()) return; + + Application application = controller.applications().requireApplication(routingTable.keySet().iterator().next().application()); + Map<DeploymentId, Map<EndpointId, Integer>> targetWeights = targetWeights(application); Map<String, Set<AliasTarget>> targetsByEndpoint = new LinkedHashMap<>(); for (Map.Entry<RoutingId, List<RoutingPolicy>> routeEntry : routingTable.entrySet()) { RoutingId routingId = routeEntry.getKey(); - EndpointList endpoints = controller.routing().readDeclaredEndpointsOf(routingId.application()) + EndpointList endpoints = controller.routing().declaredEndpointsOf(application) .scope(Endpoint.Scope.application) .named(routingId.endpointId()); if (endpoints.isEmpty()) continue; @@ -224,7 +231,7 @@ public class RoutingPolicies { for (var target : endpoint.targets()) { if (!policy.appliesTo(target.deployment())) continue; int weight = target.weight(); - if (isConfiguredOut(policy, inactiveZones)) { + if (isConfiguredOut(policy, inactiveZones) && removableFromApplicationEndpoint(policy, application, targetWeights)) { weight = 0; } WeightedAliasTarget weightedAliasTarget = new WeightedAliasTarget(policy.canonicalName(), policy.dnsZone().get(), @@ -330,6 +337,42 @@ public class RoutingPolicies { } } + /** Returns whether we disable given policy from its application endpoints, taking weights and status of other instances into account */ + private boolean removableFromApplicationEndpoint(RoutingPolicy policy, Application application, Map<DeploymentId, Map<EndpointId, Integer>> targetWeights) { + List<RoutingPolicy> relatedPolicies = application.productionInstances().keySet().stream() + .filter(instanceName -> !policy.id().owner().instance().equals(instanceName)) + .map(instanceName -> application.id().instance(instanceName)) + .flatMap(instance -> get(instance).values().stream()) + .filter(relatedPolicy -> relatedPolicy.id().zone().equals(policy.id().zone()) && + relatedPolicy.id().cluster().equals(policy.id().cluster())) + .collect(Collectors.toUnmodifiableList()); + for (var endpointId : policy.applicationEndpoints()) { + boolean anyIn = relatedPolicies.stream() + .anyMatch(rp -> rp.applicationEndpoints().contains(endpointId) && + rp.status().routingStatus().value() == RoutingStatus.Value.in && + targetWeights.get(rp.id().deployment()) + .get(endpointId) > 0); + if (!anyIn) { + return false; + } + } + return true; + } + + /** Returns target weights of application endpoints in given application, grouped by deployment */ + private Map<DeploymentId, Map<EndpointId, Integer>> targetWeights(Application application) { + Map<DeploymentId, Map<EndpointId, Integer>> weights = new HashMap<>(); + for (var endpoint : application.deploymentSpec().endpoints()) { + for (var target : endpoint.targets()) { + weights.computeIfAbsent(new DeploymentId(application.id().instance(target.instance()), + ZoneId.from(Environment.prod, target.region())), + (k) -> new HashMap<>()) + .put(EndpointId.of(endpoint.endpointId()), target.weight()); + } + } + return weights; + } + private Set<RoutingId> instanceRoutingIds(LoadBalancerAllocation allocation) { return routingIdsFrom(allocation, false); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicyId.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicyId.java index b7daa9fb6f7..e9cbdbd9b75 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicyId.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicyId.java @@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.controller.routing; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; import java.util.Objects; @@ -34,6 +35,11 @@ public class RoutingPolicyId { return zone; } + /** The deployment this applies to */ + public DeploymentId deployment() { + return new DeploymentId(owner, zone); + } + /** The cluster this applies to */ public ClusterSpec.Id cluster() { return cluster; 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 c777b659461..c40cb20a0bc 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 @@ -762,18 +762,6 @@ public class RoutingPoliciesTest { Map.of(betaZone2, 1, mainZone2, 9)); - // Changing routing status updates weight - tester.routingPolicies().setRoutingStatus(mainZone2, RoutingStatus.Value.out, RoutingStatus.Agent.tenant); - betaContext.flushDnsUpdates(); - tester.assertTargets(application, EndpointId.of("a1"), ClusterSpec.Id.from("c1"), 1, - Map.of(betaZone2, 1, - mainZone2, 0)); - tester.routingPolicies().setRoutingStatus(mainZone2, RoutingStatus.Value.in, RoutingStatus.Agent.tenant); - betaContext.flushDnsUpdates(); - tester.assertTargets(application, EndpointId.of("a1"), ClusterSpec.Id.from("c1"), 1, - Map.of(betaZone2, 1, - mainZone2, 9)); - // An endpoint is removed applicationPackage = applicationPackageBuilder() .instances("beta,main") @@ -793,6 +781,68 @@ public class RoutingPoliciesTest { .named(EndpointId.of("a1")).isEmpty()); } + @Test + public void application_endpoint_routing_status() { + RoutingPoliciesTester tester = new RoutingPoliciesTester(); + TenantAndApplicationId application = TenantAndApplicationId.from("tenant1", "app1"); + ApplicationId betaInstance = application.instance("beta"); + ApplicationId mainInstance = application.instance("main"); + + DeploymentContext betaContext = tester.newDeploymentContext(betaInstance); + DeploymentContext mainContext = tester.newDeploymentContext(mainInstance); + 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); + } + + // Deploy both instances + betaContext.submit(applicationPackage).deploy(); + + // 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 + 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); + 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); + betaContext.flushDnsUpdates(); + tester.assertTargets(application, EndpointId.of("a1"), ClusterSpec.Id.from("c1"), 1, + Map.of(betaZone2, 4, + mainZone2, 0)); + } + /** Returns an application package builder that satisfies requirements for a directly routed endpoint */ private static ApplicationPackageBuilder applicationPackageBuilder() { return new ApplicationPackageBuilder().athenzIdentity(AthenzDomain.from("domain"), |