Delete old and closed change requests. Get status of all persisted change requests

5 files changed, 65 insertions, 8 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 e8ff768927f..f8f54567bea 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
@@ -8,7 +8,8 @@ import java.util.List;
  */
 public interface ChangeRequestClient {
 
-    List<ChangeRequest> getUpcomingChangeRequests();
+    /** Get upcoming change requests and updated status of previously stored requests */
+    List<ChangeRequest> getChangeRequests(List<ChangeRequest> changeRequests);
 
     void approveChangeRequests(List<ChangeRequest> changeRequests);
 
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 e85c0afcb0e..10175f36991 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
@@ -13,7 +13,7 @@ public class MockChangeRequestClient implements ChangeRequestClient {
     private List<ChangeRequest> approvedChangeRequests = new ArrayList<>();
 
     @Override
-    public List<ChangeRequest> getUpcomingChangeRequests() {
+    public List<ChangeRequest> getChangeRequests(List<ChangeRequest> changeRequests) {
         return upcomingChangeRequests;
     }
 
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 8f30a932116..ba72f812e34 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
@@ -8,10 +8,13 @@ import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.NodeRepository;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequest;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestClient;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestSource;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.VespaChangeRequest;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
 import java.time.Duration;
+import java.time.ZonedDateTime;
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -43,7 +46,8 @@ public class ChangeRequestMaintainer extends ControllerMaintainer {
 
     @Override
     protected boolean maintain() {
-        var changeRequests = changeRequestClient.getUpcomingChangeRequests();
+        var currentChangeRequests = pruneOldChangeRequests();
+        var changeRequests = changeRequestClient.getChangeRequests(currentChangeRequests);
 
         storeChangeRequests(changeRequests);
         if (system.equals(SystemName.main)) {
@@ -82,6 +86,22 @@ public class ChangeRequestMaintainer extends ControllerMaintainer {
         }
     }
 
+    // Deletes closed change requests older than 7 days, returns the current list of requests
+    private List<ChangeRequest> pruneOldChangeRequests() {
+        List<ChangeRequest> currentChangeRequests = new ArrayList<>();
+
+        try (var lock = curator.lockChangeRequests()) {
+            for (var changeRequest : curator.readChangeRequests()) {
+                if (shouldDeleteChangeRequest(changeRequest.getChangeRequestSource())) {
+                    curator.deleteChangeRequest(changeRequest);
+                } else {
+                    currentChangeRequests.add(changeRequest);
+                }
+            }
+        }
+        return currentChangeRequests;
+    }
+
     private Map<ZoneId, List<String>> hostsByZone() {
         return controller().zoneRegistry()
                 .zones()
@@ -104,4 +124,11 @@ public class ChangeRequestMaintainer extends ControllerMaintainer {
                 .map(Map.Entry::getKey)
                 .findFirst();
     }
+
+    private boolean shouldDeleteChangeRequest(ChangeRequestSource source) {
+        return source.isClosed() &&
+                source.getPlannedStartTime()
+                        .plus(Duration.ofDays(7))
+                        .isBefore(ZonedDateTime.now());
+    }
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
index aea0eebd374..bf1ee4b23ce 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
@@ -585,6 +585,10 @@ public class CuratorDb {
         curator.set(changeRequestPath(changeRequest.getId()), asJson(ChangeRequestSerializer.toSlime(changeRequest)));
     }
 
+    public void deleteChangeRequest(VespaChangeRequest changeRequest) {
+        curator.delete(changeRequestPath(changeRequest.getId()));
+    }
+
     // -------------- Paths ---------------------------------------------------
 
     private Path lockPath(TenantName tenant) {
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 183130a8ec8..117002e6074 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,8 +26,9 @@ public class ChangeRequestMaintainerTest {
 
     @Test
     public void only_approve_requests_pending_approval() {
-        var changeRequest1 = newChangeRequest("id1", ChangeRequest.Approval.APPROVED);
-        var changeRequest2 = newChangeRequest("id2", ChangeRequest.Approval.REQUESTED);
+        var time = ZonedDateTime.now();
+        var changeRequest1 = newChangeRequest("id1", ChangeRequest.Approval.APPROVED, time);
+        var changeRequest2 = newChangeRequest("id2", ChangeRequest.Approval.REQUESTED, time);
         var upcomingChangeRequests = List.of(
                 changeRequest1,
                 changeRequest2
@@ -47,7 +48,24 @@ public class ChangeRequestMaintainerTest {
         assertEquals(expectedChangeRequest, writtenChangeRequests.get(0));
     }
 
-    private ChangeRequest newChangeRequest(String id, ChangeRequest.Approval approval) {
+    @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);
+
+        tester.curator().writeChangeRequest(newChangeRequest);
+        tester.curator().writeChangeRequest(oldChangeRequest);
+
+        changeRequestMaintainer.maintain();
+
+        var persistedChangeRequests = tester.curator().readChangeRequests();
+        assertEquals(1, persistedChangeRequests.size());
+        assertEquals(newChangeRequest, persistedChangeRequests.get(0));
+    }
+
+    private ChangeRequest newChangeRequest(String id, ChangeRequest.Approval approval, ZonedDateTime time) {
         return new ChangeRequest.Builder()
                 .id(id)
                 .approval(approval)
@@ -55,8 +73,8 @@ public class ChangeRequestMaintainerTest {
                 .impactedSwitches(List.of())
                 .impactedHosts(List.of("node-1-tenant-host-prod.us-east-3"))
                 .changeRequestSource(new ChangeRequestSource.Builder()
-                        .plannedStartTime(ZonedDateTime.now())
-                        .plannedEndTime(ZonedDateTime.now())
+                        .plannedStartTime(time)
+                        .plannedEndTime(time)
                         .id("some-id")
                         .url("some-url")
                         .system("some-system")
@@ -64,4 +82,11 @@ public class ChangeRequestMaintainerTest {
                         .build())
                 .build();
     }
+
+    private VespaChangeRequest persistedChangeRequest(String id, ZonedDateTime time) {
+        return new VespaChangeRequest(
+                newChangeRequest(id, ChangeRequest.Approval.APPROVED, time),
+                ZoneId.from("prod.us-east-3")
+        );
+    }
 }