From 7e2bec9f5d8fb2ca503ded993a10d65e19f1d0f1 Mon Sep 17 00:00:00 2001 From: Øyvind Grønnesby Date: Tue, 2 Jul 2019 11:25:13 +0200 Subject: Fix issue with rotations changing after assignment - Make sure we only include production zones in the assignment - Always run deployment through rotation assignment logic, so regions are cleared from persisted assignments even if there are no remaining rotations in the configuration. - Add tests to expose this logic. - Add tiny convenience method for AssignedRotation to create them in tests. --- .../hosted/controller/ApplicationController.java | 2 +- .../controller/rotation/RotationRepository.java | 78 ++++++++++++++++----- .../vespa/hosted/controller/ControllerTest.java | 81 ++++++++++++++++++++++ 3 files changed, 143 insertions(+), 18 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java index 0756d3006ea..ae8a81caa17 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java @@ -462,7 +462,7 @@ public class ApplicationController { /** Makes sure the application has a global rotation, if eligible. */ private LockedApplication withRotation(LockedApplication application, ZoneId zone) { - if (zone.environment() == Environment.prod && applicationNeedsRotations(application.get().deploymentSpec())) { + if (zone.environment() == Environment.prod) { try (RotationLock rotationLock = rotationRepository.lock()) { final var rotations = rotationRepository.getOrAssignRotations(application.get(), rotationLock); application = application.with(rotations); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java index 7165d134d59..340a08e0ec6 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java @@ -2,12 +2,14 @@ package com.yahoo.vespa.hosted.controller.rotation; import com.yahoo.collections.Pair; +import com.yahoo.config.application.api.Endpoint; +import com.yahoo.config.model.api.ContainerEndpoint; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.RegionName; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.ApplicationController; import com.yahoo.vespa.hosted.controller.application.AssignedRotation; -import com.yahoo.vespa.hosted.controller.application.Endpoint; import com.yahoo.vespa.hosted.controller.application.EndpointId; import com.yahoo.vespa.hosted.controller.persistence.CuratorDb; import com.yahoo.vespa.hosted.rotation.config.RotationsConfig; @@ -20,7 +22,9 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.function.Function; +import java.util.function.Predicate; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -96,6 +100,7 @@ public class RotationRepository { // Support the older case of setting global-service-id if (application.deploymentSpec().globalServiceId().isPresent()) { final var regions = application.deploymentSpec().zones().stream() + .filter(zone -> zone.environment().isProduction()) .flatMap(zone -> zone.region().stream()) .collect(Collectors.toSet()); @@ -112,31 +117,70 @@ public class RotationRepository { } final var availableRotations = new ArrayList<>(availableRotations(lock).values()); - final var assignments = application.assignedRotations().stream() + final Map existingAssignments = existingEndpointAssignments(application); + final Map updatedAssignments = assignRotationsToEndpoints(application, existingAssignments, availableRotations); + + existingAssignments.putAll(updatedAssignments); + + return List.copyOf(existingAssignments.values()); + } + + private Map assignRotationsToEndpoints(Application application, Map existingAssignments, List availableRotations) { + return application.deploymentSpec().endpoints().stream() + .filter(Predicate.not(endpoint -> existingAssignments.containsKey(EndpointId.of(endpoint.endpointId())))) + .map(endpoint -> { + return new AssignedRotation( + new ClusterSpec.Id(endpoint.containerId()), + EndpointId.of(endpoint.endpointId()), + availableRotations.remove(0).id(), + endpoint.regions() + ); + }) .collect( Collectors.toMap( AssignedRotation::endpointId, Function.identity(), - (a, b) -> { throw new IllegalStateException("Duplicate entries: " + a + ", " + b); }, + (a, b) -> { throw new IllegalStateException("Duplicate entries:" + a + ", " + b); }, LinkedHashMap::new ) ); + } - application.deploymentSpec().endpoints().stream() - .filter(endpoint -> ! assignments.containsKey(new EndpointId(endpoint.endpointId()))) - .map(endpoint -> { - return new AssignedRotation( - new ClusterSpec.Id(endpoint.containerId()), - EndpointId.of(endpoint.endpointId()), - availableRotations.remove(0).id(), - endpoint.regions() - ); - }) - .forEach(assignment -> { - assignments.put(assignment.endpointId(), assignment); - }); + private Map existingEndpointAssignments(Application application) { + // + // Get the regions that has been configured for an endpoint. Empty set if the endpoint + // is no longer mentioned in the configuration file. + // + final Function> configuredRegionsForEndpoint = endpointId -> { + return application.deploymentSpec().endpoints().stream() + .filter(endpoint -> endpointId.id().equals(endpoint.endpointId())) + .map(Endpoint::regions) + .findFirst() + .orElse(Set.of()); + }; + + // + // Build a new AssignedRotation instance where we update set of regions from the configuration instead + // of using the one already mentioned in the assignment. This allows us to overwrite the set of regions + // when + final Function assignedRotationWithConfiguredRegions = assignedRotation -> { + return new AssignedRotation( + assignedRotation.clusterId(), + assignedRotation.endpointId(), + assignedRotation.rotationId(), + configuredRegionsForEndpoint.apply(assignedRotation.endpointId()) + ); + }; - return List.copyOf(assignments.values()); + return application.assignedRotations().stream() + .collect( + Collectors.toMap( + AssignedRotation::endpointId, + assignedRotationWithConfiguredRegions, + (a, b) -> { throw new IllegalStateException("Duplicate entries: " + a + ", " + b); }, + LinkedHashMap::new + ) + ); } /** diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java index 3a905f8c6d5..6e7ca423444 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java @@ -7,6 +7,7 @@ import com.yahoo.config.application.api.ValidationId; import com.yahoo.config.application.api.ValidationOverrides; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.ApplicationName; +import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.RegionName; @@ -23,6 +24,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.deployment.SourceRevisi import com.yahoo.vespa.hosted.controller.api.integration.dns.Record; import com.yahoo.vespa.hosted.controller.api.integration.routing.RoutingEndpoint; import com.yahoo.vespa.hosted.controller.application.ApplicationPackage; +import com.yahoo.vespa.hosted.controller.application.AssignedRotation; import com.yahoo.vespa.hosted.controller.application.Deployment; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobError; import com.yahoo.vespa.hosted.controller.application.DeploymentMetrics; @@ -344,6 +346,85 @@ public class ControllerTest { } @Test + public void testDnsAliasRegistrationWithChangingZones() { + Application application = tester.createApplication("app1", "tenant1", 1, 1L); + + ApplicationPackage applicationPackage = new ApplicationPackageBuilder() + .environment(Environment.prod) + .endpoint("default", "qrs", "us-west-1", "us-central-1") + .region("us-west-1") + .region("us-central-1") // Two deployments should result in each DNS alias being registered once + .build(); + + tester.deployCompletely(application, applicationPackage); + + assertEquals( + Set.of("rotation-id-01", "app1--tenant1.global.vespa.oath.cloud"), + tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-west-1"))) + ); + + assertEquals( + Set.of("rotation-id-01", "app1--tenant1.global.vespa.oath.cloud"), + tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-central-1"))) + ); + + + ApplicationPackage applicationPackage2 = new ApplicationPackageBuilder() + .environment(Environment.prod) + .endpoint("default", "qrs", "us-west-1") + .region("us-west-1") + .region("us-central-1") // Two deployments should result in each DNS alias being registered once + .build(); + + tester.deployCompletely(application, applicationPackage2, BuildJob.defaultBuildNumber + 1); + + assertEquals( + Set.of("rotation-id-01", "app1--tenant1.global.vespa.oath.cloud"), + tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-west-1"))) + ); + + assertEquals( + Set.of(), + tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-central-1"))) + ); + + assertEquals(Set.of(RegionName.from("us-west-1")), tester.application(application.id()).assignedRotations().get(0).regions()); + } + + @Test + public void testUnassignRotations() { + Application application = tester.createApplication("app1", "tenant1", 1, 1L); + + ApplicationPackage applicationPackage = new ApplicationPackageBuilder() + .environment(Environment.prod) + .endpoint("default", "qrs", "us-west-1", "us-central-1") + .region("us-west-1") + .region("us-central-1") // Two deployments should result in each DNS alias being registered once + .build(); + + tester.deployCompletely(application, applicationPackage); + + ApplicationPackage applicationPackage2 = new ApplicationPackageBuilder() + .environment(Environment.prod) + .region("us-west-1") + .region("us-central-1") // Two deployments should result in each DNS alias being registered once + .build(); + + tester.deployCompletely(application, applicationPackage2, BuildJob.defaultBuildNumber + 1); + + + assertEquals( + List.of(AssignedRotation.fromStrings("qrs", "default", "rotation-id-01", Set.of())), + tester.application(application.id()).assignedRotations() + ); + + assertEquals( + Set.of(), + tester.configServer().rotationNames().get(new DeploymentId(application.id(), ZoneId.from("prod", "us-west-1"))) + ); + } + + @Test public void testUpdatesExistingDnsAlias() { // Application 1 is deployed and deleted { -- cgit v1.2.3