diff options
author | Martin Polden <mpolden@mpolden.no> | 2021-11-30 11:41:03 +0100 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2021-11-30 13:37:36 +0100 |
commit | 7ba1844cc7eb58886a1926ba14642ab2854decbd (patch) | |
tree | 1a0f4aede1eceba7f35631768dfe72396418df2e /controller-server | |
parent | 7b88787787b6cb6c698db1980c9be41d9224f046 (diff) |
Extract RoutingPolicyList and refactor
Diffstat (limited to 'controller-server')
11 files changed, 265 insertions, 180 deletions
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java index 6ef0df9f099..943d6ac7b18 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java @@ -118,7 +118,7 @@ public class RoutingController { // Avoid reading application more than once per call to this Supplier<DeploymentSpec> deploymentSpec = Suppliers.memoize(() -> controller.applications().requireApplication(TenantAndApplicationId.from(deployment.applicationId())).deploymentSpec()); // To discover the cluster name for a zone-scoped endpoint, we need to read routing policies - for (var policy : routingPolicies.get(deployment).values()) { + for (var policy : routingPolicies.read(deployment)) { if (!policy.status().isActive()) continue; for (var routingMethod : controller.zoneRegistry().routingMethods(policy.id().zone())) { if (routingMethod.isDirect() && !isSystemApplication && !canRouteDirectlyTo(deployment, deploymentSpec.get())) continue; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java index f7050a6280f..c6dd8bab309 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java @@ -20,6 +20,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveBucket; import com.yahoo.vespa.hosted.controller.api.integration.certificates.EndpointCertificateMetadata; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; +import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest; import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId; import com.yahoo.vespa.hosted.controller.auditlog.AuditLog; import com.yahoo.vespa.hosted.controller.deployment.RetriggerEntry; @@ -27,11 +28,9 @@ import com.yahoo.vespa.hosted.controller.deployment.RetriggerEntrySerializer; import com.yahoo.vespa.hosted.controller.deployment.Run; import com.yahoo.vespa.hosted.controller.deployment.Step; import com.yahoo.vespa.hosted.controller.dns.NameServiceQueue; -import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest; import com.yahoo.vespa.hosted.controller.notification.Notification; -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 com.yahoo.vespa.hosted.controller.routing.ZoneRoutingPolicy; import com.yahoo.vespa.hosted.controller.support.access.SupportAccess; import com.yahoo.vespa.hosted.controller.tenant.Tenant; @@ -514,15 +513,21 @@ public class CuratorDb { // -------------- Routing policies ---------------------------------------- - public void writeRoutingPolicies(ApplicationId application, Map<RoutingPolicyId, RoutingPolicy> policies) { + public void writeRoutingPolicies(ApplicationId application, List<RoutingPolicy> policies) { + for (var policy : policies) { + if (!policy.id().owner().equals(application)) { + throw new IllegalArgumentException(policy.id() + " does not belong to the application being written: " + + application.toShortString()); + } + } curator.set(routingPolicyPath(application), asJson(routingPolicySerializer.toSlime(policies))); } - public Map<ApplicationId, Map<RoutingPolicyId, RoutingPolicy>> readRoutingPolicies() { + public Map<ApplicationId, List<RoutingPolicy>> readRoutingPolicies() { return readRoutingPolicies((instance) -> true); } - public Map<ApplicationId, Map<RoutingPolicyId, RoutingPolicy>> readRoutingPolicies(Predicate<ApplicationId> filter) { + public Map<ApplicationId, List<RoutingPolicy>> readRoutingPolicies(Predicate<ApplicationId> filter) { return curator.getChildren(routingPoliciesRoot).stream() .map(ApplicationId::fromSerializedForm) .filter(filter) @@ -530,9 +535,9 @@ public class CuratorDb { this::readRoutingPolicies)); } - public Map<RoutingPolicyId, RoutingPolicy> readRoutingPolicies(ApplicationId application) { + public List<RoutingPolicy> readRoutingPolicies(ApplicationId application) { return readSlime(routingPolicyPath(application)).map(slime -> routingPolicySerializer.fromSlime(application, slime)) - .orElseGet(Map::of); + .orElseGet(List::of); } public void writeZoneRoutingPolicy(ZoneRoutingPolicy policy) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java index 04d1a4c7433..17337f823c0 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializer.java @@ -11,15 +11,15 @@ import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; import com.yahoo.slime.SlimeUtils; 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 java.time.Instant; +import java.util.ArrayList; import java.util.Collections; -import java.util.LinkedHashMap; import java.util.LinkedHashSet; -import java.util.Map; +import java.util.List; import java.util.Set; /** @@ -50,11 +50,11 @@ public class RoutingPolicySerializer { private static final String changedAtField = "changedAt"; private static final String statusField = "status"; - public Slime toSlime(Map<RoutingPolicyId, RoutingPolicy> routingPolicies) { + public Slime toSlime(List<RoutingPolicy> routingPolicies) { var slime = new Slime(); var root = slime.setObject(); var policyArray = root.setArray(routingPoliciesField); - routingPolicies.values().forEach(policy -> { + routingPolicies.forEach(policy -> { var policyObject = policyArray.addObject(); policyObject.setString(clusterField, policy.id().cluster().value()); policyObject.setString(zoneField, policy.id().zone().value()); @@ -70,8 +70,8 @@ public class RoutingPolicySerializer { return slime; } - public Map<RoutingPolicyId, RoutingPolicy> fromSlime(ApplicationId owner, Slime slime) { - var policies = new LinkedHashMap<RoutingPolicyId, RoutingPolicy>(); + public List<RoutingPolicy> fromSlime(ApplicationId owner, Slime slime) { + List<RoutingPolicy> policies = new ArrayList<>(); var root = slime.get(); var field = root.field(routingPoliciesField); field.traverse((ArrayTraverser) (i, inspect) -> { @@ -82,15 +82,15 @@ public class RoutingPolicySerializer { RoutingPolicyId id = new RoutingPolicyId(owner, ClusterSpec.Id.from(inspect.field(clusterField).asString()), ZoneId.from(inspect.field(zoneField).asString())); - policies.put(id, new RoutingPolicy(id, - HostName.from(inspect.field(canonicalNameField).asString()), - SlimeUtils.optionalString(inspect.field(dnsZoneField)), - instanceEndpoints, - applicationEndpoints, - new RoutingPolicy.Status(inspect.field(loadBalancerActiveField).asBool(), - globalRoutingFromSlime(inspect.field(globalRoutingField))))); + policies.add(new RoutingPolicy(id, + HostName.from(inspect.field(canonicalNameField).asString()), + SlimeUtils.optionalString(inspect.field(dnsZoneField)), + instanceEndpoints, + applicationEndpoints, + new RoutingPolicy.Status(inspect.field(loadBalancerActiveField).asBool(), + globalRoutingFromSlime(inspect.field(globalRoutingField))))); }); - return Collections.unmodifiableMap(policies); + return Collections.unmodifiableList(policies); } public void globalRoutingToSlime(RoutingStatus routingStatus, Cursor object) { 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 6694ac1d5a2..b98ef717dd3 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,7 +3,6 @@ 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; @@ -27,10 +26,8 @@ import com.yahoo.vespa.hosted.controller.dns.NameServiceQueue.Priority; import com.yahoo.vespa.hosted.controller.dns.NameServiceRequest; 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; @@ -38,7 +35,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.function.Function; import java.util.stream.Collectors; /** @@ -62,61 +58,61 @@ public class RoutingPolicies { } } - /** Read all known routing policies for given instance */ - public Map<RoutingPolicyId, RoutingPolicy> get(ApplicationId instance) { - return db.readRoutingPolicies(instance); + /** Read all routing policies for given deployment */ + public RoutingPolicyList read(DeploymentId deployment) { + return read(deployment.applicationId()).deployment(deployment); } - /** Read all known routing policies for given application */ - public Map<RoutingPolicyId, RoutingPolicy> get(TenantAndApplicationId application) { + /** Read all routing policies for given instance */ + public RoutingPolicyList read(ApplicationId instance) { + return RoutingPolicyList.copyOf(db.readRoutingPolicies(instance)); + } + + /** Read all routing policies for given application */ + private RoutingPolicyList read(TenantAndApplicationId application) { return db.readRoutingPolicies((instance) -> TenantAndApplicationId.from(instance).equals(application)) .values() .stream() - .map(Map::values) .flatMap(Collection::stream) - // Collect to LinkedHashMap because we want to preserve the order of routing policy within - // each instance - .collect(Collectors.toMap(RoutingPolicy::id, - Function.identity(), - (p1, p2) -> { - throw new IllegalArgumentException("Duplicate key " + p1.id()); - }, - LinkedHashMap::new)); + .collect(Collectors.collectingAndThen(Collectors.toList(), RoutingPolicyList::copyOf)); } - /** Read all known routing policies for given deployment */ - public Map<RoutingPolicyId, RoutingPolicy> get(DeploymentId deployment) { - return db.readRoutingPolicies(deployment.applicationId()).entrySet() + /** Read all routing policies */ + private RoutingPolicyList readAll() { + return db.readRoutingPolicies() + .values() .stream() - .filter(kv -> kv.getKey().zone().equals(deployment.zoneId())) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + .flatMap(Collection::stream) + .collect(Collectors.collectingAndThen(Collectors.toList(), RoutingPolicyList::copyOf)); } /** Read routing policy for given zone */ - public ZoneRoutingPolicy get(ZoneId zone) { + public ZoneRoutingPolicy read(ZoneId zone) { return db.readZoneRoutingPolicy(zone); } /** * Refresh routing policies for instance in given zone. This is idempotent and changes will only be performed if - * load balancers for given instance have changed. + * routing configuration affecting given deployment has changed. */ - public void refresh(ApplicationId instance, DeploymentSpec deploymentSpec, ZoneId zone) { - LoadBalancerAllocation allocation = new LoadBalancerAllocation(instance, zone, controller.serviceRegistry().configServer() - .getLoadBalancers(instance, zone), - deploymentSpec); + public void refresh(DeploymentId deployment, DeploymentSpec deploymentSpec) { + ApplicationId instance = deployment.applicationId(); + List<LoadBalancer> loadBalancers = controller.serviceRegistry().configServer() + .getLoadBalancers(instance, deployment.zoneId()); + LoadBalancerAllocation allocation = new LoadBalancerAllocation(loadBalancers, deployment, deploymentSpec); Set<ZoneId> inactiveZones = inactiveZones(instance, deploymentSpec); try (var lock = db.lockRoutingPolicies()) { - removeGlobalDnsUnreferencedBy(allocation, lock); - removeApplicationDnsUnreferencedBy(allocation, lock); + RoutingPolicyList applicationPolicies = read(TenantAndApplicationId.from(instance)); + RoutingPolicyList instancePolicies = applicationPolicies.instance(instance); + RoutingPolicyList deploymentPolicies = applicationPolicies.deployment(allocation.deployment); + + removeGlobalDnsUnreferencedBy(allocation, deploymentPolicies, lock); + removeApplicationDnsUnreferencedBy(allocation, deploymentPolicies, lock); - storePoliciesOf(allocation, lock); - removePoliciesUnreferencedBy(allocation, lock); + instancePolicies = storePoliciesOf(allocation, instancePolicies, lock); + instancePolicies = removePoliciesUnreferencedBy(allocation, instancePolicies, lock); - Collection<RoutingPolicy> applicationPolicies = get(TenantAndApplicationId.from(instance)).values(); - List<RoutingPolicy> instancePolicies = applicationPolicies.stream() - .filter(policy -> policy.id().owner().equals(instance)) - .collect(Collectors.toUnmodifiableList()); + applicationPolicies = applicationPolicies.replace(instance, instancePolicies); updateGlobalDnsOf(instancePolicies, inactiveZones, lock); updateApplicationDnsOf(applicationPolicies, inactiveZones, lock); } @@ -127,9 +123,9 @@ public class RoutingPolicies { try (var lock = db.lockRoutingPolicies()) { db.writeZoneRoutingPolicy(new ZoneRoutingPolicy(zone, RoutingStatus.create(value, RoutingStatus.Agent.operator, controller.clock().instant()))); - Map<ApplicationId, Map<RoutingPolicyId, RoutingPolicy>> allPolicies = db.readRoutingPolicies(); - for (var applicationPolicies : allPolicies.values()) { - updateGlobalDnsOf(applicationPolicies.values(), Set.of(), lock); + Map<ApplicationId, RoutingPolicyList> allPolicies = readAll().groupingBy(policy -> policy.id().owner()); + for (var instancePolicies : allPolicies.values()) { + updateGlobalDnsOf(instancePolicies, Set.of(), lock); } } } @@ -138,28 +134,26 @@ public class RoutingPolicies { public void setRoutingStatus(DeploymentId deployment, RoutingStatus.Value value, RoutingStatus.Agent agent) { ApplicationId instance = deployment.applicationId(); try (var lock = db.lockRoutingPolicies()) { - Map<RoutingPolicyId, RoutingPolicy> policies = get(TenantAndApplicationId.from(instance)); - Map<RoutingPolicyId, RoutingPolicy> effectivePolicies = new LinkedHashMap<>(policies); - for (var policy : policies.values()) { - if (!policy.appliesTo(deployment)) continue; + RoutingPolicyList applicationPolicies = read(TenantAndApplicationId.from(instance)); + RoutingPolicyList deploymentPolicies = applicationPolicies.deployment(deployment); + Map<RoutingPolicyId, RoutingPolicy> updatedPolicies = new LinkedHashMap<>(applicationPolicies.asMap()); + for (var policy : deploymentPolicies) { var newPolicy = policy.with(policy.status().with(RoutingStatus.create(value, agent, controller.clock().instant()))); - effectivePolicies.put(policy.id(), newPolicy); + updatedPolicies.put(policy.id(), newPolicy); } - // Group policies by their instance - Map<ApplicationId, Map<RoutingPolicyId, RoutingPolicy>> policiesByInstance = new LinkedHashMap<>(); - effectivePolicies.forEach((id, policy) -> policiesByInstance.computeIfAbsent(id.owner(), (k) -> new LinkedHashMap<>()) - .put(id, policy)); - policiesByInstance.forEach(db::writeRoutingPolicies); - policiesByInstance.forEach((ignored, instancePolicies) -> updateGlobalDnsOf(instancePolicies.values(), Set.of(), lock)); - updateApplicationDnsOf(effectivePolicies.values(), Set.of(), lock); + RoutingPolicyList effectivePolicies = RoutingPolicyList.copyOf(updatedPolicies.values()); + Map<ApplicationId, RoutingPolicyList> policiesByInstance = effectivePolicies.groupingBy(policy -> policy.id().owner()); + policiesByInstance.forEach((owner, instancePolicies) -> db.writeRoutingPolicies(owner, instancePolicies.asList())); + policiesByInstance.forEach((ignored, instancePolicies) -> updateGlobalDnsOf(instancePolicies, Set.of(), lock)); + updateApplicationDnsOf(effectivePolicies, Set.of(), lock); } } /** Update global DNS records for given policies */ - private void updateGlobalDnsOf(Collection<RoutingPolicy> routingPolicies, Set<ZoneId> inactiveZones, @SuppressWarnings("unused") Lock lock) { - Map<RoutingId, List<RoutingPolicy>> routingTable = instanceRoutingTable(routingPolicies); + private void updateGlobalDnsOf(RoutingPolicyList instancePolicies, Set<ZoneId> inactiveZones, @SuppressWarnings("unused") Lock lock) { + Map<RoutingId, List<RoutingPolicy>> routingTable = instancePolicies.asInstanceRoutingTable(); for (Map.Entry<RoutingId, List<RoutingPolicy>> routeEntry : routingTable.entrySet()) { RoutingId routingId = routeEntry.getKey(); controller.routing().readDeclaredEndpointsOf(routingId.instance()) @@ -231,12 +225,12 @@ public class RoutingPolicies { } - private void updateApplicationDnsOf(Collection<RoutingPolicy> routingPolicies, Set<ZoneId> inactiveZones, @SuppressWarnings("unused") Lock lock) { + private void updateApplicationDnsOf(RoutingPolicyList routingPolicies, Set<ZoneId> inactiveZones, @SuppressWarnings("unused") Lock lock) { // In the context of single deployment (which this is) there is only one routing policy per routing ID. I.e. // there is no scenario where more than one deployment within an instance can be a member the same // 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); + Map<RoutingId, List<RoutingPolicy>> routingTable = routingPolicies.asApplicationRoutingTable(); if (routingTable.isEmpty()) return; Application application = controller.applications().requireApplication(routingTable.keySet().iterator().next().application()); @@ -308,9 +302,13 @@ public class RoutingPolicies { }); } - /** Store routing policies for given load balancers */ - private void storePoliciesOf(LoadBalancerAllocation allocation, @SuppressWarnings("unused") Lock lock) { - var policies = new LinkedHashMap<>(get(allocation.deployment.applicationId())); + /** + * Store routing policies for given load balancers + * + * @return the updated policies + */ + private RoutingPolicyList storePoliciesOf(LoadBalancerAllocation allocation, RoutingPolicyList instancePolicies, @SuppressWarnings("unused") Lock lock) { + Map<RoutingPolicyId, RoutingPolicy> policies = new LinkedHashMap<>(instancePolicies.asMap()); for (LoadBalancer loadBalancer : allocation.loadBalancers) { if (loadBalancer.hostname().isEmpty()) continue; var policyId = new RoutingPolicyId(loadBalancer.application(), loadBalancer.cluster(), allocation.deployment.zoneId()); @@ -326,7 +324,9 @@ public class RoutingPolicies { updateZoneDnsOf(newPolicy); policies.put(newPolicy.id(), newPolicy); } - db.writeRoutingPolicies(allocation.deployment.applicationId(), policies); + RoutingPolicyList updated = RoutingPolicyList.copyOf(policies.values()); + db.writeRoutingPolicies(allocation.deployment.applicationId(), updated.asList()); + return updated; } /** Update zone DNS record for given policy */ @@ -338,14 +338,17 @@ public class RoutingPolicies { } } - /** Remove policies and zone DNS records unreferenced by given load balancers */ - private void removePoliciesUnreferencedBy(LoadBalancerAllocation allocation, @SuppressWarnings("unused") Lock lock) { - var policies = get(allocation.deployment.applicationId()); - var newPolicies = new LinkedHashMap<>(policies); - var activeIds = allocation.asPolicyIds(); - for (var policy : policies.values()) { - // Leave active load balancers and irrelevant zones alone - if (activeIds.contains(policy.id()) || !policy.appliesTo(allocation.deployment)) continue; + /** + * Remove policies and zone DNS records unreferenced by given load balancers + * + * @return the updated policies + */ + private RoutingPolicyList removePoliciesUnreferencedBy(LoadBalancerAllocation allocation, RoutingPolicyList instancePolicies, @SuppressWarnings("unused") Lock lock) { + Map<RoutingPolicyId, RoutingPolicy> newPolicies = new LinkedHashMap<>(instancePolicies.asMap()); + Set<RoutingPolicyId> activeIds = allocation.asPolicyIds(); + RoutingPolicyList removable = instancePolicies.deployment(allocation.deployment) + .not().matching(policy -> activeIds.contains(policy.id())); + for (var policy : removable) { for (var endpoint : policy.zoneEndpointsIn(controller.system(), RoutingMethod.exclusive, controller.zoneRegistry())) { var dnsName = endpoint.dnsName(); nameServiceForwarderIn(allocation.deployment.zoneId()).removeRecords(Record.Type.CNAME, @@ -354,13 +357,14 @@ public class RoutingPolicies { } newPolicies.remove(policy.id()); } - db.writeRoutingPolicies(allocation.deployment.applicationId(), newPolicies); + RoutingPolicyList updated = RoutingPolicyList.copyOf(newPolicies.values()); + db.writeRoutingPolicies(allocation.deployment.applicationId(), updated.asList()); + return updated; } /** Remove unreferenced instance endpoints from DNS */ - private void removeGlobalDnsUnreferencedBy(LoadBalancerAllocation allocation, @SuppressWarnings("unused") Lock lock) { - Collection<RoutingPolicy> zonePolicies = get(allocation.deployment).values(); - Set<RoutingId> removalCandidates = new HashSet<>(instanceRoutingTable(zonePolicies).keySet()); + private void removeGlobalDnsUnreferencedBy(LoadBalancerAllocation allocation, RoutingPolicyList deploymentPolicies, @SuppressWarnings("unused") Lock lock) { + Set<RoutingId> removalCandidates = new HashSet<>(deploymentPolicies.asInstanceRoutingTable().keySet()); Set<RoutingId> activeRoutingIds = instanceRoutingIds(allocation); removalCandidates.removeAll(activeRoutingIds); for (var id : removalCandidates) { @@ -376,9 +380,8 @@ public class RoutingPolicies { } /** Remove unreferenced application endpoints in given allocation from DNS */ - private void removeApplicationDnsUnreferencedBy(LoadBalancerAllocation allocation, @SuppressWarnings("unused") Lock lock) { - Collection<RoutingPolicy> zonePolicies = get(allocation.deployment).values(); - Map<RoutingId, List<RoutingPolicy>> routingTable = applicationRoutingTable(zonePolicies); + private void removeApplicationDnsUnreferencedBy(LoadBalancerAllocation allocation, RoutingPolicyList deploymentPolicies, @SuppressWarnings("unused") Lock lock) { + Map<RoutingId, List<RoutingPolicy>> routingTable = deploymentPolicies.asApplicationRoutingTable(); Set<RoutingId> removalCandidates = new HashSet<>(routingTable.keySet()); Set<RoutingId> activeRoutingIds = applicationRoutingIds(allocation); removalCandidates.removeAll(activeRoutingIds); @@ -399,20 +402,6 @@ public class RoutingPolicies { } } - /** 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); } @@ -435,29 +424,6 @@ public class RoutingPolicies { return Collections.unmodifiableSet(routingIds); } - /** Compute a routing table for instance-level endpoints from given policies */ - private static Map<RoutingId, List<RoutingPolicy>> instanceRoutingTable(Collection<RoutingPolicy> routingPolicies) { - return routingTable(routingPolicies, false); - } - - /** Compute a routing table for application-level endpoints from given policies */ - private static Map<RoutingId, List<RoutingPolicy>> applicationRoutingTable(Collection<RoutingPolicy> routingPolicies) { - return routingTable(routingPolicies, true); - } - - private static Map<RoutingId, List<RoutingPolicy>> routingTable(Collection<RoutingPolicy> routingPolicies, boolean applicationLevel) { - Map<RoutingId, List<RoutingPolicy>> routingTable = new LinkedHashMap<>(); - for (var policy : routingPolicies) { - Set<EndpointId> endpoints = applicationLevel ? policy.applicationEndpoints() : policy.instanceEndpoints(); - for (var endpoint : endpoints) { - RoutingId id = RoutingId.of(policy.id().owner(), endpoint); - routingTable.computeIfAbsent(id, k -> new ArrayList<>()) - .add(policy); - } - } - return Collections.unmodifiableMap(routingTable); - } - /** Returns whether the endpoints of given policy are configured {@link RoutingStatus.Value#out} */ private static boolean isConfiguredOut(ZoneRoutingPolicy zonePolicy, RoutingPolicy policy, Set<ZoneId> inactiveZones) { // A deployment can be configured out from endpoints at any of the following levels: @@ -522,9 +488,9 @@ public class RoutingPolicies { private final List<LoadBalancer> loadBalancers; private final DeploymentSpec deploymentSpec; - private LoadBalancerAllocation(ApplicationId application, ZoneId zone, List<LoadBalancer> loadBalancers, + private LoadBalancerAllocation(List<LoadBalancer> loadBalancers, DeploymentId deployment, DeploymentSpec deploymentSpec) { - this.deployment = new DeploymentId(application, zone); + this.deployment = deployment; this.loadBalancers = List.copyOf(loadBalancers); this.deploymentSpec = deploymentSpec; } 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 e9cbdbd9b75..d64241b1239 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 @@ -60,4 +60,9 @@ public class RoutingPolicyId { return Objects.hash(owner, cluster, zone); } + @Override + public String toString() { + return "routing policy for " + cluster + ", in " + zone + ", owned by " + owner; + } + } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicyList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicyList.java new file mode 100644 index 00000000000..a5efc016c68 --- /dev/null +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicyList.java @@ -0,0 +1,99 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.controller.routing; + +import com.yahoo.collections.AbstractFilteringList; +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId; +import com.yahoo.vespa.hosted.controller.application.EndpointId; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** + * A filterable list of {@link RoutingPolicy}'s. + * + * This is immutable. + * + * @author mpolden + */ +public class RoutingPolicyList extends AbstractFilteringList<RoutingPolicy, RoutingPolicyList> { + + private final Map<RoutingPolicyId, RoutingPolicy> policiesById; + + protected RoutingPolicyList(Collection<RoutingPolicy> items, boolean negate) { + super(items, negate, RoutingPolicyList::new); + this.policiesById = items.stream().collect(Collectors.collectingAndThen( + Collectors.toMap(RoutingPolicy::id, + Function.identity(), + (p1, p2) -> { + throw new IllegalArgumentException("Duplicate key " + p1.id()); + }, + LinkedHashMap::new), + Collections::unmodifiableMap) + ); + } + + /** Returns the subset of policies owned by given instance */ + public RoutingPolicyList instance(ApplicationId instance) { + return matching(policy -> policy.id().owner().equals(instance)); + } + + /** Returns the subset of policies applying to given deployment */ + public RoutingPolicyList deployment(DeploymentId deployment) { + return matching(policy -> policy.appliesTo(deployment)); + } + + /** Returns the policy with given ID, if any */ + public Optional<RoutingPolicy> of(RoutingPolicyId id) { + return Optional.ofNullable(policiesById.get(id)); + } + + /** Returns this grouped by policy ID */ + public Map<RoutingPolicyId, RoutingPolicy> asMap() { + return policiesById; + } + + /** Returns a copy of this with all policies for instance replaced with given policies */ + public RoutingPolicyList replace(ApplicationId instance, RoutingPolicyList policies) { + List<RoutingPolicy> copy = new ArrayList<>(asList()); + copy.removeIf(policy -> policy.id().owner().equals(instance)); + policies.forEach(copy::add); + return copyOf(copy); + } + + /** Create a routing table for instance-level endpoints backed by routing policies in this */ + Map<RoutingId, List<RoutingPolicy>> asInstanceRoutingTable() { + return asRoutingTable(false); + } + + /** Create a routing table for application-level endpoints backed by routing policies in this */ + Map<RoutingId, List<RoutingPolicy>> asApplicationRoutingTable() { + return asRoutingTable(true); + } + + private Map<RoutingId, List<RoutingPolicy>> asRoutingTable(boolean applicationLevel) { + Map<RoutingId, List<RoutingPolicy>> routingTable = new LinkedHashMap<>(); + for (var policy : this) { + Set<EndpointId> endpoints = applicationLevel ? policy.applicationEndpoints() : policy.instanceEndpoints(); + for (var endpoint : endpoints) { + RoutingId id = RoutingId.of(policy.id().owner(), endpoint); + routingTable.computeIfAbsent(id, k -> new ArrayList<>()) + .add(policy); + } + } + return Collections.unmodifiableMap(routingTable); + } + + public static RoutingPolicyList copyOf(Collection<RoutingPolicy> policies) { + return new RoutingPolicyList(policies, false); + } + +} diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java index 28fbeee28f5..6fd8a3a84d5 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/DeploymentRoutingContext.java @@ -49,7 +49,7 @@ public abstract class DeploymentRoutingContext implements RoutingContext { /** Configure routing for the deployment in this context, using given deployment spec */ public final void configure(DeploymentSpec deploymentSpec) { - controller.policies().refresh(deployment.applicationId(), deploymentSpec, deployment.zoneId()); + controller.policies().refresh(deployment, deploymentSpec); } /** Routing method of this context */ @@ -60,7 +60,7 @@ public abstract class DeploymentRoutingContext implements RoutingContext { /** Read the routing policy for given cluster in this deployment */ public final Optional<RoutingPolicy> routingPolicy(ClusterSpec.Id cluster) { RoutingPolicyId id = new RoutingPolicyId(deployment.applicationId(), cluster, deployment.zoneId()); - return Optional.ofNullable(controller.policies().get(deployment).get(id)); + return controller.policies().read(deployment).of(id); } /** @@ -142,8 +142,8 @@ public abstract class DeploymentRoutingContext implements RoutingContext { public RoutingStatus routingStatus() { // Status for a deployment applies to all clusters within the deployment, so we use the status from the // first matching policy here - return controller.policies().get(deployment).values().stream() - .findFirst() + return controller.policies().read(deployment) + .first() .map(RoutingPolicy::status) .map(RoutingPolicy.Status::routingStatus) .orElse(RoutingStatus.DEFAULT); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/ExclusiveZoneRoutingContext.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/ExclusiveZoneRoutingContext.java index e29fb5ab404..75009e0b37a 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/ExclusiveZoneRoutingContext.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/context/ExclusiveZoneRoutingContext.java @@ -30,7 +30,7 @@ public class ExclusiveZoneRoutingContext implements RoutingContext { @Override public RoutingStatus routingStatus() { - return policies.get(zone).routingStatus(); + return policies.read(zone).routingStatus(); } @Override 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 c5c8eec2aac..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); @@ -931,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() { |