diff options
5 files changed, 83 insertions, 119 deletions
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java index 8c05b47e8e8..170547430cb 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java @@ -63,7 +63,7 @@ public class DeploymentSpec { Optional<AthenzDomain> athenzDomain, Optional<AthenzService> athenzService, String xmlForm) { - this.steps = List.copyOf(completeSteps(steps)); + this.steps = List.copyOf(steps); this.majorVersion = majorVersion; this.athenzDomain = athenzDomain; this.athenzService = athenzService; @@ -72,50 +72,6 @@ public class DeploymentSpec { validateUpgradePoliciesOfIncreasingConservativeness(steps); } - /** Adds missing required steps and reorders steps to a permissible order */ - private static List<DeploymentSpec.Step> completeSteps(List<DeploymentSpec.Step> inputSteps) { - List<Step> steps = new ArrayList<>(inputSteps); - - // Add staging if required and missing - if (steps.stream().anyMatch(step -> step.concerns(Environment.prod)) && - steps.stream().noneMatch(step -> step.concerns(Environment.staging))) { - steps.add(new DeploymentSpec.DeclaredZone(Environment.staging)); - } - - // Add test if required and missing - if (steps.stream().anyMatch(step -> step.concerns(Environment.staging)) && - steps.stream().noneMatch(step -> step.concerns(Environment.test))) { - steps.add(new DeploymentSpec.DeclaredZone(Environment.test)); - } - - // Enforce order test, staging, prod - DeploymentSpec.DeclaredZone testStep = remove(Environment.test, steps); - if (testStep != null) - steps.add(0, testStep); - DeploymentSpec.DeclaredZone stagingStep = remove(Environment.staging, steps); - if (stagingStep != null) - steps.add(1, stagingStep); - - return steps; - } - - /** - * Removes the first occurrence of a deployment step to the given environment and returns it. - * - * @return the removed step, or null if it is not present - */ - private static DeploymentSpec.DeclaredZone remove(Environment environment, List<DeploymentSpec.Step> steps) { - for (int i = 0; i < steps.size(); i++) { - if ( ! (steps.get(i) instanceof DeploymentSpec.DeclaredZone)) continue; - DeploymentSpec.DeclaredZone zoneStep = (DeploymentSpec.DeclaredZone)steps.get(i); - if (zoneStep.environment() == environment) { - steps.remove(i); - return zoneStep; - } - } - return null; - } - /** Throw an IllegalArgumentException if the total delay exceeds 24 hours */ private void validateTotalDelay(List<Step> steps) { long totalDelaySeconds = steps.stream().mapToLong(step -> (step.delay().getSeconds())).sum(); 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 e4f52f573b0..ac605add892 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 @@ -77,9 +77,8 @@ public class DeploymentSpecTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(2, spec.steps().size()); + assertEquals(1, spec.steps().size()); assertEquals(1, spec.requireInstance("default").steps().size()); - assertTrue(spec.steps().get(0).concerns(Environment.test)); assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.staging)); assertFalse(spec.requireInstance("default").deploysTo(Environment.test, Optional.empty())); assertTrue(spec.requireInstance("default").deploysTo(Environment.staging, Optional.empty())); @@ -101,13 +100,9 @@ public class DeploymentSpecTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(3, spec.steps().size()); + assertEquals(1, spec.steps().size()); assertEquals(2, spec.requireInstance("default").steps().size()); - assertTrue(spec.steps().get(0).concerns(Environment.test)); - - assertTrue(spec.steps().get(1).concerns(Environment.staging)); - assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.prod, Optional.of(RegionName.from("us-east1")))); assertFalse(((DeploymentSpec.DeclaredZone)spec.requireInstance("default").steps().get(0)).active()); @@ -500,6 +495,7 @@ public class DeploymentSpecTest { public void testNestedParallelAndSteps() { StringReader r = new StringReader( "<deployment athenz-domain='domain'>" + + " <staging />" + " <instance id='instance' athenz-service='in-service'>" + " <prod>" + " <parallel>" + @@ -529,13 +525,12 @@ public class DeploymentSpecTest { DeploymentSpec spec = DeploymentSpec.fromXml(r); List<DeploymentSpec.Step> steps = spec.steps(); - assertEquals(3, steps.size()); - assertEquals("test", steps.get(0).toString()); - assertEquals("staging", steps.get(1).toString()); - assertEquals("instance 'instance'", steps.get(2).toString()); - assertEquals(Duration.ofHours(4), steps.get(2).delay()); + assertEquals(2, steps.size()); + assertEquals("staging", steps.get(0).toString()); + assertEquals("instance 'instance'", steps.get(1).toString()); + assertEquals(Duration.ofHours(4), steps.get(1).delay()); - List<DeploymentSpec.Step> instanceSteps = steps.get(2).steps(); + List<DeploymentSpec.Step> instanceSteps = steps.get(1).steps(); assertEquals(2, instanceSteps.size()); assertEquals("4 parallel steps", instanceSteps.get(0).toString()); assertEquals("prod.us-north-7", instanceSteps.get(1).toString()); @@ -590,12 +585,10 @@ public class DeploymentSpecTest { DeploymentSpec spec = DeploymentSpec.fromXml(r); List<DeploymentSpec.Step> steps = spec.steps(); - assertEquals(3, steps.size()); - assertEquals("test", steps.get(0).toString()); - assertEquals("staging", steps.get(1).toString()); - assertEquals("2 parallel steps", steps.get(2).toString()); + assertEquals(1, steps.size()); + assertEquals("2 parallel steps", steps.get(0).toString()); - List<DeploymentSpec.Step> parallelSteps = steps.get(2).steps(); + List<DeploymentSpec.Step> parallelSteps = steps.get(0).steps(); assertEquals("instance 'instance0'", parallelSteps.get(0).toString()); assertEquals("instance 'instance1'", parallelSteps.get(1).toString()); } @@ -620,12 +613,10 @@ public class DeploymentSpecTest { DeploymentSpec spec = DeploymentSpec.fromXml(r); List<DeploymentSpec.Step> steps = spec.steps(); - assertEquals(5, steps.size()); - assertEquals("test", steps.get(0).toString()); - assertEquals("staging", steps.get(1).toString()); - assertEquals("instance 'instance0'", steps.get(2).toString()); - assertEquals("delay PT12H", steps.get(3).toString()); - assertEquals("instance 'instance1'", steps.get(4).toString()); + assertEquals(3, steps.size()); + assertEquals("instance 'instance0'", steps.get(0).toString()); + assertEquals("delay PT12H", steps.get(1).toString()); + assertEquals("instance 'instance1'", steps.get(2).toString()); } @Test diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java index 00f0a1bfa41..50999759b77 100644 --- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java +++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java @@ -71,9 +71,8 @@ public class DeploymentSpecWithoutInstanceTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(2, spec.steps().size()); + assertEquals(1, spec.steps().size()); assertEquals(1, spec.requireInstance("default").steps().size()); - assertTrue(spec.steps().get(0).concerns(Environment.test)); assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.staging)); assertFalse(spec.requireInstance("default").deploysTo(Environment.test, Optional.empty())); assertTrue(spec.requireInstance("default").deploysTo(Environment.staging, Optional.empty())); @@ -93,13 +92,9 @@ public class DeploymentSpecWithoutInstanceTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(3, spec.steps().size()); + assertEquals(1, spec.steps().size()); assertEquals(2, spec.requireInstance("default").steps().size()); - assertTrue(spec.steps().get(0).concerns(Environment.test)); - - assertTrue(spec.steps().get(1).concerns(Environment.staging)); - assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.prod, Optional.of(RegionName.from("us-east1")))); assertFalse(((DeploymentSpec.DeclaredZone)spec.requireInstance("default").steps().get(0)).active()); @@ -352,6 +347,7 @@ public class DeploymentSpecWithoutInstanceTest { public void testNestedParallelAndSteps() { StringReader r = new StringReader( "<deployment athenz-domain='domain' athenz-service='service'>" + + " <staging />" + " <prod>" + " <parallel>" + " <region active='true'>us-west-1</region>" + @@ -379,18 +375,17 @@ public class DeploymentSpecWithoutInstanceTest { DeploymentSpec spec = DeploymentSpec.fromXml(r); List<DeploymentSpec.Step> steps = spec.steps(); - assertEquals(3, steps.size()); - assertEquals("test", steps.get(0).toString()); - assertEquals("staging", steps.get(1).toString()); - assertEquals("instance 'default'", steps.get(2).toString()); - assertEquals(Duration.ofHours(4), steps.get(2).delay()); - - List<DeploymentSpec.Step> instanceSteps = steps.get(2).steps(); - assertEquals(2, instanceSteps.size()); - assertEquals("4 parallel steps", instanceSteps.get(0).toString()); - assertEquals("prod.us-north-7", instanceSteps.get(1).toString()); - - List<DeploymentSpec.Step> parallelSteps = instanceSteps.get(0).steps(); + assertEquals(1, steps.size()); + assertEquals("instance 'default'", steps.get(0).toString()); + assertEquals(Duration.ofHours(4), steps.get(0).delay()); + + List<DeploymentSpec.Step> instanceSteps = steps.get(0).steps(); + assertEquals(3, instanceSteps.size()); + assertEquals("staging", instanceSteps.get(0).toString()); + assertEquals("4 parallel steps", instanceSteps.get(1).toString()); + assertEquals("prod.us-north-7", instanceSteps.get(2).toString()); + + List<DeploymentSpec.Step> parallelSteps = instanceSteps.get(1).steps(); assertEquals(4, parallelSteps.size()); assertEquals("prod.us-west-1", parallelSteps.get(0).toString()); assertEquals("4 steps", parallelSteps.get(1).toString()); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java index 730f8829c6d..d8bad46fd3f 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java @@ -52,14 +52,17 @@ public class DeploymentStatus { private final JobList allJobs; private final SystemName system; private final Version systemVersion; - private final Map<JobId, StepStatus> steps; + private final Map<JobId, StepStatus> jobSteps; + private final List<StepStatus> allSteps; public DeploymentStatus(Application application, Map<JobId, JobStatus> allJobs, SystemName system, Version systemVersion) { this.application = requireNonNull(application); this.allJobs = JobList.from(allJobs.values()); this.system = system; this.systemVersion = systemVersion; - this.steps = jobDependencies(application.deploymentSpec()); + List<StepStatus> allSteps = new ArrayList<>(); + this.jobSteps = jobDependencies(application.deploymentSpec(), allSteps); + this.allSteps = List.copyOf(allSteps); } public Application application() { @@ -118,10 +121,10 @@ public class DeploymentStatus { return ImmutableMap.copyOf(jobs); } - public Map<JobId, StepStatus> stepStatus() { return steps; } + public Map<JobId, StepStatus> stepStatus() { return jobSteps; } private void addProductionJobs(Map<JobId, List<Versions>> jobs, Change change) { - steps.forEach((job, step) -> { + jobSteps.forEach((job, step) -> { Versions versions = Versions.from(change, application, deploymentFor(job), systemVersion); if ( job.type().isProduction() && step.completedAt(change, versions).isEmpty() @@ -137,7 +140,7 @@ public class DeploymentStatus { declaredTest(job.application(), systemTest).ifPresent(test -> { testJobs.merge(test, versions.stream() - .filter(version -> ! steps.get(test).isRunning(version)) + .filter(version -> ! jobSteps.get(test).isRunning(version)) .filter(version -> allJobs.successOn(version).get(test).isEmpty() && allJobs.triggeredOn(version).get(job).isEmpty()) .collect(toUnmodifiableList()), @@ -146,7 +149,7 @@ public class DeploymentStatus { declaredTest(job.application(), stagingTest).ifPresent(test -> { testJobs.merge(test, versions.stream() - .filter(version -> ! steps.get(test).isRunning(version)) + .filter(version -> ! jobSteps.get(test).isRunning(version)) .filter(version -> allJobs.successOn(version).get(test).isEmpty() && allJobs.triggeredOn(version).get(job).isEmpty()) .collect(toUnmodifiableList()), @@ -158,7 +161,7 @@ public class DeploymentStatus { if ( ! job.type().isTest() && ! testedOn(versions, systemTest, testJobs)) testJobs.merge(new JobId(job.application(), systemTest), versions.stream() - .filter(version -> steps.keySet().stream().noneMatch(id -> id.type() == systemTest && steps.get(id).isRunning(version))) + .filter(version -> jobSteps.keySet().stream().noneMatch(id -> id.type() == systemTest && jobSteps.get(id).isRunning(version))) .filter(version -> allJobs.successOn(version).type(systemTest).isEmpty() && allJobs.triggeredOn(version).get(job).isEmpty()) .collect(toUnmodifiableList()), @@ -166,7 +169,7 @@ public class DeploymentStatus { if ( ! job.type().isTest() && ! testedOn(versions, stagingTest, testJobs)) testJobs.merge(new JobId(job.application(), stagingTest), versions.stream() - .filter(version -> steps.keySet().stream().noneMatch(id -> id.type() == stagingTest && steps.get(id).isRunning(version))) + .filter(version -> jobSteps.keySet().stream().noneMatch(id -> id.type() == stagingTest && jobSteps.get(id).isRunning(version))) .filter(version -> allJobs.successOn(version).type(stagingTest).isEmpty() && allJobs.triggeredOn(version).get(job).isEmpty()) .collect(toUnmodifiableList()), @@ -176,10 +179,8 @@ public class DeploymentStatus { } private Optional<JobId> declaredTest(ApplicationId instanceId, JobType testJob) { - return steps.keySet().stream() - .filter(job -> job.type() == testJob) - .filter(job -> job.application().equals(instanceId)) - .findAny(); + JobId jobId = new JobId(instanceId, testJob); + return jobSteps.get(jobId).isDeclared() ? Optional.of(jobId) : Optional.empty(); } private boolean testedOn(List<Versions> versions, JobType testJob, Map<JobId, List<Versions>> testJobs) { @@ -197,31 +198,41 @@ public class DeploymentStatus { } /** Returns a DAG of the dependencies between the primitive steps in the spec, with iteration order equal to declaration order. */ - Map<JobId, StepStatus> jobDependencies(DeploymentSpec spec) { + Map<JobId, StepStatus> jobDependencies(DeploymentSpec spec, List<StepStatus> allSteps) { if (DeploymentSpec.empty.equals(spec)) return Map.of(); Map<JobId, StepStatus> dependencies = new LinkedHashMap<>(); List<StepStatus> previous = List.of(); for (DeploymentSpec.Step step : spec.steps()) - previous = fillStep(dependencies, step, previous, spec.instanceNames().get(0)); + previous = fillStep(dependencies, allSteps, step, previous, spec.instanceNames().get(0)); + for (InstanceName instance : spec.instanceNames()) + for (JobType test : List.of(systemTest, stagingTest)) { + JobId job = new JobId(application.id().instance(instance), test); + if ( ! dependencies.containsKey(job)) + dependencies.put(job, JobStepStatus.ofTestDeployment(new DeclaredZone(test.environment()), List.of(), + this, instance, test, false)); + } return ImmutableMap.copyOf(dependencies); } /** Adds the primitive steps contained in the given step, which depend on the given previous primitives, to the dependency graph. */ - List<StepStatus> fillStep(Map<JobId, StepStatus> dependencies, DeploymentSpec.Step step, - List<StepStatus> previous, InstanceName instance) { + List<StepStatus> fillStep(Map<JobId, StepStatus> dependencies, List<StepStatus> allSteps, + DeploymentSpec.Step step, List<StepStatus> previous, InstanceName instance) { if (step.steps().isEmpty()) { - if ( ! step.delay().isZero()) - return List.of(new DelayStatus((DeploymentSpec.Delay) step, previous)); + if ( ! step.delay().isZero()) { + StepStatus stepStatus = new DelayStatus((DeploymentSpec.Delay) step, previous); + allSteps.add(stepStatus); + return List.of(stepStatus); + } JobType jobType; StepStatus stepStatus; if (step.concerns(test) || step.concerns(staging)) { // SKIP? jobType = JobType.from(system, ((DeclaredZone) step).environment(), null) .orElseThrow(() -> new IllegalStateException("No job is known for " + step + " in " + system)); - stepStatus = JobStepStatus.ofTestDeployment((DeclaredZone) step, List.of(), this, instance, jobType); + stepStatus = JobStepStatus.ofTestDeployment((DeclaredZone) step, List.of(), this, instance, jobType, true); previous = new ArrayList<>(previous); previous.add(stepStatus); } @@ -240,6 +251,7 @@ public class DeploymentStatus { previous = List.of(stepStatus); } else return previous; // Empty container steps end up here. + allSteps.add(stepStatus); dependencies.put(new JobId(application.id().instance(instance), jobType), stepStatus); return previous; } @@ -250,28 +262,28 @@ public class DeploymentStatus { .map(DeploymentInstanceSpec::name); if (step.isOrdered()) { for (DeploymentSpec.Step nested : step.steps()) - previous = fillStep(dependencies, nested, previous, stepInstance.orElse(instance)); + previous = fillStep(dependencies, allSteps, nested, previous, stepInstance.orElse(instance)); return previous; } List<StepStatus> parallel = new ArrayList<>(); for (DeploymentSpec.Step nested : step.steps()) - parallel.addAll(fillStep(dependencies, nested, previous, stepInstance.orElse(instance))); + parallel.addAll(fillStep(dependencies, allSteps, nested, previous, stepInstance.orElse(instance))); return List.copyOf(parallel); } - private boolean isTested(JobId job, Versions versions) { + public boolean isTested(JobId job, Versions versions) { return allJobs.triggeredOn(versions).get(job).isPresent() || ! declaredTest(job.application(), systemTest).map(__ -> allJobs.instance(job.application().instance())) - .orElse(allJobs) - .type(systemTest) - .successOn(versions).isEmpty() + .orElse(allJobs) + .type(systemTest) + .successOn(versions).isEmpty() && ! declaredTest(job.application(), stagingTest).map(__ -> allJobs.instance(job.application().instance())) - .orElse(allJobs) - .type(stagingTest) - .successOn(versions).isEmpty(); + .orElse(allJobs) + .type(stagingTest) + .successOn(versions).isEmpty(); } /** @@ -284,6 +296,7 @@ public class DeploymentStatus { * * The completion criterion for each type of step is implemented in subclasses of this. */ + // TODO jonmv: Make the step status expose _what it is_. public static abstract class StepStatus { private final DeploymentSpec.Step step; @@ -321,8 +334,12 @@ public class DeploymentStatus { : Optional.empty(); } + /** Whether this step is currently running, with the given version parameters. */ public abstract boolean isRunning(Versions versions); + /** Whether this step is declared in the deployment spec, or is an implicit step. */ + public boolean isDeclared() { return true; } + } @@ -350,7 +367,8 @@ public class DeploymentStatus { private final JobStatus job; private final DeploymentStatus status; - protected JobStepStatus(DeploymentSpec.Step step, List<StepStatus> dependencies, JobStatus job, DeploymentStatus status) { + protected JobStepStatus(DeploymentSpec.Step step, List<StepStatus> dependencies, JobStatus job, + DeploymentStatus status) { super(step, dependencies, job.id().application().instance()); this.job = requireNonNull(job); this.status = requireNonNull(status); @@ -443,7 +461,8 @@ public class DeploymentStatus { } public static JobStepStatus ofTestDeployment(DeclaredZone step, List<StepStatus> dependencies, - DeploymentStatus status, InstanceName instance, JobType jobType) { + DeploymentStatus status, InstanceName instance, + JobType jobType, boolean declared) { JobStatus job = status.instanceJobs(instance).get(jobType); return new JobStepStatus(step, dependencies, job, status) { @Override @@ -455,6 +474,9 @@ public class DeploymentStatus { .map(run -> run.end().get()) .max(naturalOrder()); } + + @Override + public boolean isDeclared() { return declared; } }; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java index d6de0b06ccc..5798a11758d 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java @@ -700,7 +700,7 @@ public class InternalStepRunner implements StepRunner { if (step.concerns(id.type().environment())) return step.zones().get(0).testerFlavor(); - throw new IllegalStateException("No step deploys to the zone this run is for!"); + return Optional.empty(); } /** Returns the generated services.xml content for the tester application. */ |