diff options
5 files changed, 50 insertions, 18 deletions
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/VespaChangeRequest.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/VespaChangeRequest.java index ea56521dc3b..a8be4a77c71 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/VespaChangeRequest.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/VespaChangeRequest.java @@ -48,6 +48,10 @@ public class VespaChangeRequest extends ChangeRequest { return new VespaChangeRequest(getId(), source, getImpactedSwitches(), getImpactedHosts(), getApproval(), getImpact(), status, hostActionPlan, zoneId); } + public VespaChangeRequest withApproval(Approval approval) { + return new VespaChangeRequest(getId(), getChangeRequestSource(), getImpactedSwitches(), getImpactedHosts(), approval, getImpact(), status, hostActionPlan, zoneId); + } + public VespaChangeRequest withActionPlan(List<HostAction> hostActionPlan) { return new VespaChangeRequest(getId(), getChangeRequestSource(), getImpactedSwitches(), getImpactedHosts(), getApproval(), getImpact(), status, hostActionPlan, zoneId); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java index ba72f812e34..0ebf4cbc2d2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java @@ -49,6 +49,7 @@ public class ChangeRequestMaintainer extends ControllerMaintainer { var currentChangeRequests = pruneOldChangeRequests(); var changeRequests = changeRequestClient.getChangeRequests(currentChangeRequests); + logger.fine(() -> "Found requests: " + changeRequests); storeChangeRequests(changeRequests); if (system.equals(SystemName.main)) { approveChanges(changeRequests); @@ -63,6 +64,7 @@ public class ChangeRequestMaintainer extends ControllerMaintainer { .filter(changeRequest -> changeRequest.getApproval() == ChangeRequest.Approval.REQUESTED) .collect(Collectors.toList()); + logger.fine(() -> "Approving " + unapprovedRequests); changeRequestClient.approveChangeRequests(unapprovedRequests); } @@ -79,7 +81,9 @@ public class ChangeRequestMaintainer extends ControllerMaintainer { optionalZone.ifPresent(zone -> { var vcmr = existingChangeRequests .getOrDefault(changeRequest.getId(), new VespaChangeRequest(changeRequest, zone)) - .withSource(changeRequest.getChangeRequestSource()); + .withSource(changeRequest.getChangeRequestSource()) + .withApproval(changeRequest.getApproval()); + logger.fine(() -> "Storing " + vcmr); curator.writeChangeRequest(vcmr); }); }); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java index 99f2927e1fc..015da1faae8 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ControllerMaintenance.java @@ -155,7 +155,7 @@ public class ControllerMaintenance extends AbstractComponent { this.archiveUriUpdater = duration(5, MINUTES); this.archiveAccessMaintainer = duration(10, MINUTES); this.tenantRoleMaintainer = duration(5, MINUTES); - this.changeRequestMaintainer = duration(12, HOURS); + this.changeRequestMaintainer = duration(1, HOURS); this.vcmrMaintainer = duration(1, HOURS); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainer.java index 036331a7454..a8de70a56a2 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainer.java @@ -63,10 +63,10 @@ public class VCMRMaintainer extends ControllerMaintainer { var status = getStatus(nextActions, changeRequest); try (var lock = curator.lockChangeRequests()) { - curator.writeChangeRequest( - changeRequest - .withActionPlan(nextActions) - .withStatus(status)); + // Read the vcmr again, in case the source status has been updated + curator.readChangeRequest(changeRequest.getId()) + .ifPresent(vcmr -> curator.writeChangeRequest(vcmr.withActionPlan(nextActions) + .withStatus(status))); } }); @@ -128,11 +128,13 @@ public class VCMRMaintainer extends ControllerMaintainer { .orElse(new HostAction(node.hostname().value(), State.NONE, Instant.now())); if (changeRequest.getChangeRequestSource().isClosed()) { + logger.fine(() -> changeRequest.getChangeRequestSource().getId() + " is closed, recycling " + node.hostname()); recycleNode(changeRequest.getZoneId(), node, hostAction); return hostAction.withState(State.COMPLETE); } if (isPostponed(changeRequest, hostAction)) { + logger.fine(() -> changeRequest.getChangeRequestSource().getId() + " is postponed, recycling " + node.hostname()); recycleNode(changeRequest.getZoneId(), node, hostAction); return hostAction.withState(State.PENDING_RETIREMENT); } @@ -159,10 +161,12 @@ public class VCMRMaintainer extends ControllerMaintainer { } if (hasRetired(node, hostAction)) { + logger.fine(() -> node.hostname() + " has retired"); return hostAction.withState(State.RETIRED); } - if (pendingRetirement(node)) { + if (pendingRetirement(node, hostAction)) { + logger.fine(() -> node.hostname() + " is pending retirement"); return hostAction.withState(State.PENDING_RETIREMENT); } @@ -205,8 +209,8 @@ public class VCMRMaintainer extends ControllerMaintainer { /** * TODO: For now, we choose to retire any active host */ - private boolean pendingRetirement(Node node) { - return node.state() == Node.State.active; + private boolean pendingRetirement(Node node, HostAction action) { + return action.getState() == State.NONE && node.state() == Node.State.active; } private Map<ZoneId, List<Node>> nodesByZone() { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainerTest.java index 117002e6074..290e08ca47b 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainerTest.java @@ -5,6 +5,7 @@ import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequest; import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestSource; +import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestSource.Status; import com.yahoo.vespa.hosted.controller.api.integration.vcmr.MockChangeRequestClient; import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest; import org.junit.Test; @@ -26,9 +27,8 @@ public class ChangeRequestMaintainerTest { @Test public void only_approve_requests_pending_approval() { - var time = ZonedDateTime.now(); - var changeRequest1 = newChangeRequest("id1", ChangeRequest.Approval.APPROVED, time); - var changeRequest2 = newChangeRequest("id2", ChangeRequest.Approval.REQUESTED, time); + var changeRequest1 = newChangeRequest("id1", ChangeRequest.Approval.APPROVED); + var changeRequest2 = newChangeRequest("id2", ChangeRequest.Approval.REQUESTED); var upcomingChangeRequests = List.of( changeRequest1, changeRequest2 @@ -49,11 +49,27 @@ public class ChangeRequestMaintainerTest { } @Test + public void updates_status_time_and_approval() { + var time = ZonedDateTime.now(); + var persistedChangeRequest = persistedChangeRequest("some-id", time.minusDays(5), Status.WAITING_FOR_APPROVAL); + tester.curator().writeChangeRequest(persistedChangeRequest); + + var updatedChangeRequest = newChangeRequest("some-id", ChangeRequest.Approval.APPROVED, time, Status.CANCELED); + changeRequestClient.setUpcomingChangeRequests(List.of(updatedChangeRequest)); + changeRequestMaintainer.maintain(); + + persistedChangeRequest = tester.curator().readChangeRequest("some-id").get(); + assertEquals(Status.CANCELED, persistedChangeRequest.getChangeRequestSource().getStatus()); + assertEquals(ChangeRequest.Approval.APPROVED, persistedChangeRequest.getApproval()); + assertEquals(time, persistedChangeRequest.getChangeRequestSource().getPlannedStartTime()); + } + + @Test public void deletes_old_change_requests() { var now = ZonedDateTime.now(); var before = now.minus(Duration.ofDays(8)); - var newChangeRequest = persistedChangeRequest("new", now); - var oldChangeRequest = persistedChangeRequest("old", before); + var newChangeRequest = persistedChangeRequest("new", now, Status.CLOSED); + var oldChangeRequest = persistedChangeRequest("old", before, Status.CLOSED); tester.curator().writeChangeRequest(newChangeRequest); tester.curator().writeChangeRequest(oldChangeRequest); @@ -65,7 +81,11 @@ public class ChangeRequestMaintainerTest { assertEquals(newChangeRequest, persistedChangeRequests.get(0)); } - private ChangeRequest newChangeRequest(String id, ChangeRequest.Approval approval, ZonedDateTime time) { + private ChangeRequest newChangeRequest(String id, ChangeRequest.Approval approval) { + return newChangeRequest(id, approval, ZonedDateTime.now(), Status.CLOSED); + } + + private ChangeRequest newChangeRequest(String id, ChangeRequest.Approval approval, ZonedDateTime time, Status status) { return new ChangeRequest.Builder() .id(id) .approval(approval) @@ -78,14 +98,14 @@ public class ChangeRequestMaintainerTest { .id("some-id") .url("some-url") .system("some-system") - .status(ChangeRequestSource.Status.CLOSED) + .status(status) .build()) .build(); } - private VespaChangeRequest persistedChangeRequest(String id, ZonedDateTime time) { + private VespaChangeRequest persistedChangeRequest(String id, ZonedDateTime time, Status status) { return new VespaChangeRequest( - newChangeRequest(id, ChangeRequest.Approval.APPROVED, time), + newChangeRequest(id, ChangeRequest.Approval.REQUESTED, time, status), ZoneId.from("prod.us-east-3") ); } |