Only approve VCMRs manageable by automation

6 files changed, 54 insertions, 52 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/ChangeRequestClient.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/ChangeRequestClient.java
index f8f54567bea..4fa195f1b05 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/ChangeRequestClient.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/ChangeRequestClient.java
@@ -11,6 +11,6 @@ public interface ChangeRequestClient {
     /** Get upcoming change requests and updated status of previously stored requests */
     List<ChangeRequest> getChangeRequests(List<ChangeRequest> changeRequests);
 
-    void approveChangeRequests(List<ChangeRequest> changeRequests);
+    void approveChangeRequest(ChangeRequest changeRequest);
 
 }
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/MockChangeRequestClient.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/MockChangeRequestClient.java
index 10175f36991..e64b2ee3368 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/MockChangeRequestClient.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/vcmr/MockChangeRequestClient.java
@@ -18,8 +18,8 @@ public class MockChangeRequestClient implements ChangeRequestClient {
     }
 
     @Override
-    public void approveChangeRequests(List<ChangeRequest> changeRequests) {
-        approvedChangeRequests.addAll(changeRequests);
+    public void approveChangeRequest(ChangeRequest changeRequest) {
+        approvedChangeRequests.add(changeRequest);
     }
 
     public void setUpcomingChangeRequests(List<ChangeRequest> changeRequests) {
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 0ebf4cbc2d2..1f360c477b9 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
@@ -31,14 +31,12 @@ public class ChangeRequestMaintainer extends ControllerMaintainer {
 
     private final Logger logger = Logger.getLogger(ChangeRequestMaintainer.class.getName());
     private final ChangeRequestClient changeRequestClient;
-    private final SystemName system;
     private final CuratorDb curator;
     private final NodeRepository nodeRepository;
 
     public ChangeRequestMaintainer(Controller controller, Duration interval) {
         super(controller, interval, null, SystemName.allOf(Predicate.not(SystemName::isPublic)));
         this.changeRequestClient = controller.serviceRegistry().changeRequestClient();
-        this.system = controller.system();
         this.curator = controller.curator();
         this.nodeRepository = controller.serviceRegistry().configServer().nodeRepository();
     }
@@ -51,23 +49,10 @@ public class ChangeRequestMaintainer extends ControllerMaintainer {
 
         logger.fine(() -> "Found requests: " + changeRequests);
         storeChangeRequests(changeRequests);
-        if (system.equals(SystemName.main)) {
-            approveChanges(changeRequests);
-        }
 
         return true;
     }
 
-    private void approveChanges(List<ChangeRequest> changeRequests) {
-        var unapprovedRequests = changeRequests
-                .stream()
-                .filter(changeRequest -> changeRequest.getApproval() == ChangeRequest.Approval.REQUESTED)
-                .collect(Collectors.toList());
-
-        logger.fine(() -> "Approving " + unapprovedRequests);
-        changeRequestClient.approveChangeRequests(unapprovedRequests);
-    }
-
     private void storeChangeRequests(List<ChangeRequest> changeRequests) {
         var existingChangeRequests = curator.readChangeRequests()
                 .stream()
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 a8de70a56a2..f525dde9e03 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
@@ -10,7 +10,9 @@ import com.yahoo.vespa.hosted.controller.api.integration.configserver.Node;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.NodeRepository;
 import com.yahoo.vespa.hosted.controller.api.integration.noderepository.NodeRepositoryNode;
 import com.yahoo.vespa.hosted.controller.api.integration.noderepository.NodeState;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequest;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequest.Impact;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestClient;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.HostAction;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.HostAction.State;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest;
@@ -41,11 +43,15 @@ public class VCMRMaintainer extends ControllerMaintainer {
     private final Duration ALLOWED_POSTPONEMENT_TIME = Duration.ofDays(7);
     private final CuratorDb curator;
     private final NodeRepository nodeRepository;
+    private final ChangeRequestClient changeRequestClient;
+    private final SystemName system;
 
     public VCMRMaintainer(Controller controller, Duration interval) {
         super(controller, interval, null, SystemName.allOf(Predicate.not(SystemName::isPublic)));
         this.curator = controller.curator();
         this.nodeRepository = controller.serviceRegistry().configServer().nodeRepository();
+        this.changeRequestClient = controller.serviceRegistry().changeRequestClient();
+        this.system = controller.system();
     }
 
     @Override
@@ -65,11 +71,14 @@ public class VCMRMaintainer extends ControllerMaintainer {
             try (var lock = curator.lockChangeRequests()) {
                 // 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)));
+                        .ifPresent(vcmr -> {
+                            var updatedVcmr = vcmr.withActionPlan(nextActions)
+                                    .withStatus(status);
+                            curator.writeChangeRequest(updatedVcmr);
+                            approveChangeRequest(updatedVcmr);
+                        });
             }
         });
-
         return true;
     }
 
@@ -133,6 +142,9 @@ public class VCMRMaintainer extends ControllerMaintainer {
             return hostAction.withState(State.COMPLETE);
         }
 
+        if (isLowImpact(changeRequest))
+            return hostAction;
+
         if (isPostponed(changeRequest, hostAction)) {
             logger.fine(() -> changeRequest.getChangeRequestSource().getId() + " is postponed, recycling " + node.hostname());
             recycleNode(changeRequest.getZoneId(), node, hostAction);
@@ -233,9 +245,12 @@ public class VCMRMaintainer extends ControllerMaintainer {
                         .orElse(false);
     }
     private Predicate<VespaChangeRequest> shouldUpdate() {
-        return changeRequest -> changeRequest.getStatus() != Status.COMPLETED &&
-                 List.of(Impact.HIGH, Impact.VERY_HIGH)
-                         .contains(changeRequest.getImpact());
+        return changeRequest -> changeRequest.getStatus() != Status.COMPLETED;
+    }
+
+    private boolean isLowImpact(VespaChangeRequest changeRequest) {
+        return !List.of(Impact.HIGH, Impact.VERY_HIGH)
+                .contains(changeRequest.getImpact());
     }
 
     private boolean hasSpareCapacity(ZoneId zoneId, List<Node> nodes) {
@@ -253,4 +268,16 @@ public class VCMRMaintainer extends ControllerMaintainer {
         newNode.setWantToRetire(wantToRetire);
         nodeRepository.patchNode(zoneId, node.hostname().value(), newNode);
     }
+
+    private void approveChangeRequest(VespaChangeRequest changeRequest) {
+        if (!system.equals(SystemName.main))
+            return;
+        if (changeRequest.getStatus() == Status.REQUIRES_OPERATOR_ACTION)
+            return;
+        if (changeRequest.getApproval() != ChangeRequest.Approval.REQUESTED)
+            return;
+
+        logger.info("Approving " + changeRequest.getChangeRequestSource().getId());
+        changeRequestClient.approveChangeRequest(changeRequest);
+    }
 }
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 290e08ca47b..15a2cf3063d 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
@@ -26,29 +26,6 @@ public class ChangeRequestMaintainerTest {
     private final ChangeRequestMaintainer changeRequestMaintainer = new ChangeRequestMaintainer(tester.controller(), Duration.ofMinutes(1));
 
     @Test
-    public void only_approve_requests_pending_approval() {
-        var changeRequest1 = newChangeRequest("id1", ChangeRequest.Approval.APPROVED);
-        var changeRequest2 = newChangeRequest("id2", ChangeRequest.Approval.REQUESTED);
-        var upcomingChangeRequests = List.of(
-                changeRequest1,
-                changeRequest2
-        );
-
-        changeRequestClient.setUpcomingChangeRequests(upcomingChangeRequests);
-        changeRequestMaintainer.maintain();
-
-        var approvedChangeRequests = changeRequestClient.getApprovedChangeRequests();
-
-        assertEquals(1, approvedChangeRequests.size());
-        assertEquals("id2", approvedChangeRequests.get(0).getId());
-        var writtenChangeRequests = tester.curator().readChangeRequests();
-        assertEquals(2, writtenChangeRequests.size());
-
-        var expectedChangeRequest = new VespaChangeRequest(changeRequest1, ZoneId.from("prod.us-east-3"));
-        assertEquals(expectedChangeRequest, writtenChangeRequests.get(0));
-    }
-
-    @Test
     public void updates_status_time_and_approval() {
         var time = ZonedDateTime.now();
         var persistedChangeRequest = persistedChangeRequest("some-id", time.minusDays(5), Status.WAITING_FOR_APPROVAL);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainerTest.java
index d5c35f806f4..49c8423b82b 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/VCMRMaintainerTest.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.vcmr.HostAction.State;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest.Status;
 import com.yahoo.vespa.hosted.controller.integration.NodeRepositoryMock;
+import org.junit.Before;
 import org.junit.Test;
 
 import java.time.Duration;
@@ -26,14 +27,21 @@ import static org.junit.Assert.*;
  */
 public class VCMRMaintainerTest {
 
-    private final ControllerTester tester = new ControllerTester();
-    private final VCMRMaintainer maintainer = new VCMRMaintainer(tester.controller(), Duration.ofMinutes(1));
-    private final NodeRepositoryMock nodeRepo = tester.serviceRegistry().configServer().nodeRepository();
+    private ControllerTester tester;
+    private VCMRMaintainer maintainer;
+    private NodeRepositoryMock nodeRepo;
     private final ZoneId zoneId = ZoneId.from("prod.us-east-3");
     private final HostName host1 = HostName.from("host1");
     private final HostName host2 = HostName.from("host2");
     private final String changeRequestId = "id123";
 
+    @Before
+    public void setup() {
+        tester = new ControllerTester();
+        maintainer = new VCMRMaintainer(tester.controller(), Duration.ofMinutes(1));
+        nodeRepo = tester.serviceRegistry().configServer().nodeRepository();
+    }
+
     @Test
     public void recycle_hosts_after_completion() {
         var parkedNode = createNode(host1, NodeType.host, Node.State.parked, true);
@@ -88,7 +96,6 @@ public class VCMRMaintainerTest {
 
         activeNode = nodeRepo.list(zoneId, List.of(activeNode.hostname())).get(0);
         assertTrue(activeNode.wantToRetire());
-
     }
 
     @Test
@@ -107,6 +114,9 @@ public class VCMRMaintainerTest {
         assertEquals(State.REQUIRES_OPERATOR_ACTION, parkedNodeAction.getState());
         assertEquals(State.REQUIRES_OPERATOR_ACTION, failedNodeAction.getState());
         assertEquals(Status.REQUIRES_OPERATOR_ACTION, writtenChangeRequest.getStatus());
+
+        var approvedChangeRequests = tester.serviceRegistry().changeRequestClient().getApprovedChangeRequests();
+        assertTrue(approvedChangeRequests.isEmpty());
     }
 
     @Test
@@ -137,6 +147,9 @@ public class VCMRMaintainerTest {
         var tenantHostAction = writtenChangeRequest.getHostActionPlan().get(0);
         assertEquals(State.PENDING_RETIREMENT, tenantHostAction.getState());
         assertEquals(Status.PENDING_ACTION, writtenChangeRequest.getStatus());
+
+        var approvedChangeRequests = tester.serviceRegistry().changeRequestClient().getApprovedChangeRequests();
+        assertEquals(1, approvedChangeRequests.size());
     }
 
     @Test
@@ -195,7 +208,7 @@ public class VCMRMaintainerTest {
                 source,
                 List.of("switch1"),
                 List.of("host1", "host2"),
-                ChangeRequest.Approval.APPROVED,
+                ChangeRequest.Approval.REQUESTED,
                 ChangeRequest.Impact.VERY_HIGH,
                 VespaChangeRequest.Status.IN_PROGRESS,
                 actionPlan,