From a0e109c58240fd06e24b8ee708442dc9e837fac1 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Thu, 1 Feb 2018 09:58:54 +0100 Subject: Let outstanding change know the application version --- .../yahoo/vespa/hosted/controller/Application.java | 16 ++++++-------- .../vespa/hosted/controller/LockedApplication.java | 14 ++++++------ .../hosted/controller/application/Change.java | 6 +++++- .../controller/deployment/DeploymentTrigger.java | 12 ++++------- .../maintenance/OutstandingChangeDeployer.java | 6 ++---- .../persistence/ApplicationSerializer.java | 25 +++++++++++++++------- .../vespa/hosted/controller/ControllerTest.java | 8 +++---- .../maintenance/OutstandingChangeDeployerTest.java | 17 +++++++++++---- .../persistence/ApplicationSerializerTest.java | 12 +++++++++-- 9 files changed, 67 insertions(+), 49 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java index f26502e4630..2c9c4261cbd 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java @@ -42,7 +42,7 @@ public class Application { private final Map deployments; private final DeploymentJobs deploymentJobs; private final Change change; - private final boolean outstandingChange; + private final Change outstandingChange; private final Optional ownershipIssueId; private final ApplicationMetrics metrics; private final Optional rotation; @@ -51,14 +51,14 @@ public class Application { public Application(ApplicationId id) { this(id, DeploymentSpec.empty, ValidationOverrides.empty, Collections.emptyMap(), new DeploymentJobs(Optional.empty(), Collections.emptyList(), Optional.empty()), - Change.empty(), false, Optional.empty(), new ApplicationMetrics(0, 0), + Change.empty(), Change.empty(), Optional.empty(), new ApplicationMetrics(0, 0), Optional.empty()); } /** Used from persistence layer: Do not use */ public Application(ApplicationId id, DeploymentSpec deploymentSpec, ValidationOverrides validationOverrides, List deployments, DeploymentJobs deploymentJobs, Change change, - boolean outstandingChange, Optional ownershipIssueId, ApplicationMetrics metrics, + Change outstandingChange, Optional ownershipIssueId, ApplicationMetrics metrics, Optional rotation) { this(id, deploymentSpec, validationOverrides, deployments.stream().collect(Collectors.toMap(Deployment::zone, d -> d)), @@ -67,7 +67,7 @@ public class Application { Application(ApplicationId id, DeploymentSpec deploymentSpec, ValidationOverrides validationOverrides, Map deployments, DeploymentJobs deploymentJobs, Change change, - boolean outstandingChange, Optional ownershipIssueId, ApplicationMetrics metrics, + Change outstandingChange, Optional ownershipIssueId, ApplicationMetrics metrics, Optional rotation) { Objects.requireNonNull(id, "id cannot be null"); Objects.requireNonNull(deploymentSpec, "deploymentSpec cannot be null"); @@ -129,7 +129,7 @@ public class Application { * Returns whether this has an outstanding change (in the source repository), which * has currently not started deploying (because a deployment is (or was) already in progress */ - public boolean hasOutstandingChange() { return outstandingChange; } + public Change outstandingChange() { return outstandingChange; } public Optional ownershipIssueId() { return ownershipIssueId; @@ -173,11 +173,7 @@ public class Application { /** Returns the application version a deployment to this zone should use, or empty if we don't know */ public Optional deployApplicationVersionIn(ZoneId zone) { if (change().application().isPresent()) { - ApplicationVersion version = change().application().get(); - if (version.isUnknown()) - return Optional.empty(); - else - return Optional.of(version); + return Optional.of(change().application().get()); } return applicationVersionIn(zone); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java index 4b6213733a0..10752b74967 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java @@ -49,7 +49,7 @@ public class LockedApplication extends Application { private LockedApplication(Builder builder) { super(builder.applicationId, builder.deploymentSpec, builder.validationOverrides, builder.deployments, builder.deploymentJobs, builder.deploying, - builder.hasOutstandingChange, builder.ownershipIssueId, builder.metrics, builder.rotation); + builder.outstandingChange, builder.ownershipIssueId, builder.metrics, builder.rotation); } public LockedApplication withProjectId(long projectId) { @@ -126,8 +126,8 @@ public class LockedApplication extends Application { return new LockedApplication(new Builder(this).withChange(change)); } - public LockedApplication withOutstandingChange(boolean outstandingChange) { - return new LockedApplication(new Builder(this).with(outstandingChange)); + public LockedApplication withOutstandingChange(Change outstandingChange) { + return new LockedApplication(new Builder(this).withOutstandingChange(outstandingChange)); } public LockedApplication withOwnershipIssueId(IssueId issueId) { @@ -181,7 +181,7 @@ public class LockedApplication extends Application { private Map deployments; private DeploymentJobs deploymentJobs; private Change deploying; - private boolean hasOutstandingChange; + private Change outstandingChange; private Optional ownershipIssueId; private ApplicationMetrics metrics; private Optional rotation; @@ -193,7 +193,7 @@ public class LockedApplication extends Application { this.deployments = application.deployments(); this.deploymentJobs = application.deploymentJobs(); this.deploying = application.change(); - this.hasOutstandingChange = application.hasOutstandingChange(); + this.outstandingChange = application.outstandingChange(); this.ownershipIssueId = application.ownershipIssueId(); this.metrics = application.metrics(); this.rotation = application.rotation().map(ApplicationRotation::id); @@ -224,8 +224,8 @@ public class LockedApplication extends Application { return this; } - private Builder with(boolean hasOutstandingChange) { - this.hasOutstandingChange = hasOutstandingChange; + private Builder withOutstandingChange(Change outstandingChange) { + this.outstandingChange = outstandingChange; return this; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java index 13d66c8d083..216025215d3 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java @@ -31,6 +31,10 @@ public final class Change { private Change(Optional platform, Optional application) { Objects.requireNonNull(platform, "platform cannot be null"); Objects.requireNonNull(application, "application cannot be null"); + if (application.isPresent() && application.get().isUnknown()) { + // TODO: Require version to be known for application change + //throw new IllegalArgumentException("Application version to deploy must be a known version"); + } this.platform = platform; this.application = application; } @@ -42,7 +46,7 @@ public final class Change { return false; } - /** Returns whether a change shoudl currently be deployed */ + /** Returns whether a change should currently be deployed */ public boolean isPresent() { return platform.isPresent() || application.isPresent(); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java index 6560423f3b9..271fbe65dbb 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java @@ -78,8 +78,8 @@ public class DeploymentTrigger { */ public void triggerFromCompletion(JobReport report) { applications().lockOrThrow(report.applicationId(), application -> { - application = application.withJobCompletion(report, applicationVersionFrom(report), clock.instant(), - controller); + ApplicationVersion applicationVersion = applicationVersionFrom(report); + application = application.withJobCompletion(report, applicationVersion, clock.instant(), controller); application = application.withProjectId(report.projectId()); // Handle successful starting and ending @@ -88,15 +88,11 @@ public class DeploymentTrigger { if (acceptNewApplicationVersionNow(application)) { // Set this as the change we are doing, unless we are already pushing a platform change if ( ! ( application.change().platform().isPresent())) { - ApplicationVersion applicationVersion = ApplicationVersion.unknown; - if (report.sourceRevision().isPresent()) - applicationVersion = ApplicationVersion.from(report.sourceRevision().get(), - report.buildNumber()); application = application.withChange(Change.of(applicationVersion)); } } else { // postpone - applications().store(application.withOutstandingChange(true)); + applications().store(application.withOutstandingChange(Change.of(applicationVersion))); return; } } @@ -264,7 +260,7 @@ public class DeploymentTrigger { application.change() + " is already in progress"); application = application.withChange(change); if (change.application().isPresent()) - application = application.withOutstandingChange(false); + application = application.withOutstandingChange(Change.empty()); application = trigger(JobType.systemTest, application, false, change.toString()); applications().store(application); }); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java index 3dd63a511e1..9f1738f9560 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java @@ -4,8 +4,6 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.application.ApplicationList; -import com.yahoo.vespa.hosted.controller.application.ApplicationVersion; -import com.yahoo.vespa.hosted.controller.application.Change; import java.time.Duration; @@ -24,9 +22,9 @@ public class OutstandingChangeDeployer extends Maintainer { protected void maintain() { ApplicationList applications = ApplicationList.from(controller().applications().asList()).notPullRequest(); for (Application application : applications.asList()) { - if (application.hasOutstandingChange() && ! application.change().isPresent()) + if (!application.change().isPresent() && application.outstandingChange().isPresent()) controller().applications().deploymentTrigger().triggerChange(application.id(), - Change.of(ApplicationVersion.unknown)); + application.outstandingChange()); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java index 4b7993b3b7d..670338d10e5 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java @@ -10,6 +10,7 @@ import com.yahoo.slime.ArrayTraverser; import com.yahoo.slime.Cursor; import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; +import com.yahoo.slime.Type; import com.yahoo.vespa.config.SlimeUtils; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.api.integration.MetricsService.ApplicationMetrics; @@ -126,8 +127,8 @@ public class ApplicationSerializer { root.setString(validationOverridesField, application.validationOverrides().xmlForm()); deploymentsToSlime(application.deployments().values(), root.setArray(deploymentsField)); toSlime(application.deploymentJobs(), root.setObject(deploymentJobsField)); - toSlime(application.change(), root); - root.setBool(outstandingChangeField, application.hasOutstandingChange()); + toSlime(application.change(), root, deployingField); + toSlime(application.outstandingChange(), root, outstandingChangeField); application.ownershipIssueId().ifPresent(issueId -> root.setString(ownershipIssueIdField, issueId.value())); root.setDouble(queryQualityField, application.metrics().queryServiceQuality()); root.setDouble(writeQualityField, application.metrics().writeServiceQuality()); @@ -243,20 +244,19 @@ public class ApplicationSerializer { Cursor object = parent.setObject(jobRunObjectName); object.setLong(jobRunIdField, jobRun.get().id()); object.setString(versionField, jobRun.get().version().toString()); - if ( ! jobRun.get().applicationVersion().isUnknown()) - toSlime(jobRun.get().applicationVersion(), object.setObject(revisionField)); + toSlime(jobRun.get().applicationVersion(), object.setObject(revisionField)); object.setBool(upgradeField, jobRun.get().upgrade()); object.setString(reasonField, jobRun.get().reason()); object.setLong(atField, jobRun.get().at().toEpochMilli()); } - private void toSlime(Change deploying, Cursor parentObject) { + private void toSlime(Change deploying, Cursor parentObject, String fieldName) { if ( ! deploying.isPresent()) return; - Cursor object = parentObject.setObject(deployingField); + Cursor object = parentObject.setObject(fieldName); if (deploying.platform().isPresent()) object.setString(versionField, deploying.platform().get().toString()); - if (deploying.application().isPresent() && !deploying.application().get().isUnknown()) + if (deploying.application().isPresent()) toSlime(deploying.application().get(), object); } @@ -271,7 +271,7 @@ public class ApplicationSerializer { List deployments = deploymentsFromSlime(root.field(deploymentsField)); DeploymentJobs deploymentJobs = deploymentJobsFromSlime(root.field(deploymentJobsField)); Change deploying = changeFromSlime(root.field(deployingField)); - boolean outstandingChange = root.field(outstandingChangeField).asBool(); + Change outstandingChange = outstandingChangeFromSlime(root.field(outstandingChangeField)); Optional ownershipIssueId = optionalString(root.field(ownershipIssueIdField)).map(IssueId::from); ApplicationMetrics metrics = new ApplicationMetrics(root.field(queryQualityField).asDouble(), root.field(writeQualityField).asDouble()); @@ -391,6 +391,15 @@ public class ApplicationSerializer { return change; } + // TODO: Remove and inline after 2018-03-01 + private Change outstandingChangeFromSlime(Inspector object) { + if (object.type() == Type.BOOL) { + boolean outstandingChange = object.asBool(); + return outstandingChange ? Change.of(ApplicationVersion.unknown) : Change.empty(); + } + return changeFromSlime(object); + } + private List jobStatusListFromSlime(Inspector array) { List jobStatusList = new ArrayList<>(); array.traverse((ArrayTraverser) (int i, Inspector item) -> jobStatusList.add(jobStatusFromSlime(item))); 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 97bbaec6427..5b71d12a68b 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 @@ -848,8 +848,7 @@ public class ControllerTest { } - private void runDeployment(DeploymentTester tester, ApplicationId application, - ApplicationVersion expectedVersion, + private void runDeployment(DeploymentTester tester, ApplicationId application, ApplicationVersion expectedVersion, ApplicationPackage applicationPackage, Optional sourceRevision, long initialBuildNumber) { Version vespaVersion = Version.fromString("6.1"); // Set in config server mock @@ -857,9 +856,8 @@ public class ControllerTest { // Component notifies completion tester.notifyJobCompletion(component, app, Optional.empty(), sourceRevision, initialBuildNumber); - ApplicationVersion change = - sourceRevision.map(sr -> ApplicationVersion.from(sr, initialBuildNumber)) - .orElse(ApplicationVersion.unknown); + ApplicationVersion change = sourceRevision.map(sr -> ApplicationVersion.from(sr, initialBuildNumber)) + .orElse(ApplicationVersion.unknown); assertEquals(change.id(), tester.controller().applications() .require(application) .change().application().get().id()); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java index 3d34e78c759..4bed276fcac 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java @@ -2,15 +2,18 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.component.Version; +import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.api.integration.BuildService; import com.yahoo.vespa.hosted.controller.application.Change; import com.yahoo.vespa.hosted.controller.application.DeploymentJobs; +import com.yahoo.vespa.hosted.controller.application.SourceRevision; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; import org.junit.Test; import java.time.Duration; import java.util.List; +import java.util.Optional; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -34,9 +37,15 @@ public class OutstandingChangeDeployerTest { tester.deploymentTrigger().triggerChange(tester.application("app1").id(), Change.of(version)); assertEquals(Change.of(version), tester.application("app1").change()); - assertFalse(tester.application("app1").hasOutstandingChange()); - tester.notifyJobCompletion(DeploymentJobs.JobType.component, tester.application("app1"), true); - assertTrue(tester.application("app1").hasOutstandingChange()); + assertFalse(tester.application("app1").outstandingChange().isPresent()); + tester.notifyJobCompletion(DeploymentJobs.JobType.component, tester.application("app1"), + Optional.empty(), Optional.of(new SourceRevision("repo", "master", + "cafed00d")), + 42); + + Application app = tester.application("app1"); + assertTrue(app.outstandingChange().isPresent()); + assertEquals("1.0.42-cafed00d", app.outstandingChange().application().get().id()); assertEquals(1, tester.buildSystem().jobs().size()); deployer.maintain(); @@ -50,7 +59,7 @@ public class OutstandingChangeDeployerTest { assertEquals(1, jobs.size()); assertEquals(11, jobs.get(0).projectId()); assertEquals(DeploymentJobs.JobType.systemTest.jobName(), jobs.get(0).jobName()); - assertFalse(tester.application("app1").hasOutstandingChange()); + assertFalse(tester.application("app1").outstandingChange().isPresent()); } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java index 35bd63745c4..9c19cdb66ac 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java @@ -86,7 +86,7 @@ public class ApplicationSerializerTest { validationOverrides, deployments, deploymentJobs, Change.of(Version.fromString("6.7")), - true, + Change.of(ApplicationVersion.from(new SourceRevision("repo", "master", "deadcafe"), 42)), Optional.of(IssueId.from("1234")), new MetricsService.ApplicationMetrics(0.5, 0.9), Optional.of(new RotationId("my-rotation"))); @@ -113,7 +113,7 @@ public class ApplicationSerializerTest { assertEquals( original.deploymentJobs().jobStatus().get(DeploymentJobs.JobType.stagingTest), serialized.deploymentJobs().jobStatus().get(DeploymentJobs.JobType.stagingTest)); - assertEquals(original.hasOutstandingChange(), serialized.hasOutstandingChange()); + assertEquals(original.outstandingChange(), serialized.outstandingChange()); assertEquals(original.ownershipIssueId(), serialized.ownershipIssueId()); @@ -163,6 +163,14 @@ public class ApplicationSerializerTest { Application original4 = writable(original).withChange(Change.empty()); Application serialized4 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original4)); assertEquals(original4.change(), serialized4.change()); + + Application original5 = writable(original).withChange(Change.of(ApplicationVersion.unknown)); + Application serialized5 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original5)); + assertEquals(original5.change(), serialized5.change()); + + Application original6 = writable(original).withOutstandingChange(Change.of(ApplicationVersion.unknown)); + Application serialized6 = applicationSerializer.fromSlime(applicationSerializer.toSlime(original6)); + assertEquals(original6.outstandingChange(), serialized6.outstandingChange()); } } -- cgit v1.2.3