Merge pull request #26739 from vespa-engine/olaa/dont-rewrite-cmr-report

Automatically approve CMR impacting proxy node

2 files changed, 27 insertions, 8 deletions

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 9271f870390..d4c4b4efda7 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
@@ -139,7 +139,10 @@ public class VcmrMaintainer extends ControllerMaintainer {
                         return Stream.empty();
                     }
                     var spareCapacity = hasSpareCapacity(zone, nodes);
-                    return nodes.stream().map(node -> nextAction(zone, node, changeRequest, spareCapacity));
+                    var impactedProxyCount = nodes.stream()
+                            .filter(node -> node.type() == NodeType.proxy)
+                            .count();
+                    return nodes.stream().map(node -> nextAction(zone, node, changeRequest, spareCapacity, impactedProxyCount));
                 }).toList();
 
     }
@@ -162,7 +165,7 @@ public class VcmrMaintainer extends ControllerMaintainer {
                 .findFirst();
     }
 
-    private HostAction nextAction(ZoneId zoneId, Node node, VespaChangeRequest changeRequest, boolean spareCapacity) {
+    private HostAction nextAction(ZoneId zoneId, Node node, VespaChangeRequest changeRequest, boolean spareCapacity, long impactedProxyCount) {
         var hostAction = getPreviousAction(node, changeRequest)
                 .orElse(new HostAction(node.hostname().value(), State.NONE, Instant.now()));
 
@@ -176,7 +179,8 @@ public class VcmrMaintainer extends ControllerMaintainer {
         if (isLowImpact(changeRequest))
             return hostAction;
 
-        addReport(zoneId, changeRequest, node);
+        if (shouldAddReport(node, changeRequest.getChangeRequestSource().getId(), hostAction))
+            addReport(zoneId, changeRequest, node);
 
         if (isOutOfSync(node, hostAction))
             return hostAction.withState(State.OUT_OF_SYNC);
@@ -187,7 +191,13 @@ public class VcmrMaintainer extends ControllerMaintainer {
             return hostAction.withState(State.PENDING_RETIREMENT);
         }
 
-        if (node.type() != NodeType.host || !spareCapacity) {
+        if (!spareCapacity) {
+            return hostAction.withState(State.REQUIRES_OPERATOR_ACTION);
+        }
+
+        if (node.type() != NodeType.host) {
+            if (node.type() == NodeType.proxy && impactedProxyCount == 1)
+                return hostAction.withState(State.READY);
             return hostAction.withState(State.REQUIRES_OPERATOR_ACTION);
         }
 
@@ -267,6 +277,16 @@ public class VcmrMaintainer extends ControllerMaintainer {
                 && node.state() == Node.State.active;
     }
 
+    private boolean shouldAddReport(Node node, String vcmrId, HostAction previousAction) {
+        var vcmrReport = VcmrReport.fromReports(node.reports());
+        var hasReport = vcmrReport.getVcmrs().stream().map(VcmrReport.Vcmr::id).anyMatch(id -> id.equals(vcmrId));
+        // Don't add report if none exists and this is not initial assessment
+        // Presumably removed manually by operator.
+        if (!hasReport && previousAction.getState() != State.NONE)
+            return false;
+        return true;
+    }
+
     // Determines if node state is unexpected based on previous action taken
     private boolean isOutOfSync(Node node, HostAction action) {
         return action.getState() == State.RETIRED && node.state() != Node.State.parked ||
@@ -343,8 +363,7 @@ public class VcmrMaintainer extends ControllerMaintainer {
     private void addReport(ZoneId zoneId, VespaChangeRequest changeRequest, Node node) {
         var report = VcmrReport.fromReports(node.reports());
 
-        var source = changeRequest.getChangeRequestSource();
-        if (report.addVcmr(source.getId(), source.getPlannedStartTime(), source.getPlannedEndTime())) {
+        if (report.addVcmr(changeRequest.getChangeRequestSource())) {
             updateReport(zoneId, node, report);
         }
     }
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 52bd8e9c618..39bf61df9ed 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
@@ -58,7 +58,7 @@ public class VcmrMaintainerTest {
     @Test
     void recycle_hosts_after_completion() {
         var vcmrReport = new VcmrReport();
-        vcmrReport.addVcmr("id123", ZonedDateTime.now(), ZonedDateTime.now());
+        vcmrReport.addVcmr(new ChangeRequestSource("aws", "id123", "url", ChangeRequestSource.Status.WAITING_FOR_APPROVAL , ZonedDateTime.now(), ZonedDateTime.now()));
         var parkedNode = createNode(host1, NodeType.host, Node.State.parked, true);
         var failedNode = createNode(host2, NodeType.host, Node.State.failed, false);
         var reports = vcmrReport.toNodeReports();
@@ -181,7 +181,7 @@ public class VcmrMaintainerTest {
         activeNode = nodeRepo.list(zoneId, NodeFilter.all().hostnames(host2)).get(0);
         var report = VcmrReport.fromReports(activeNode.reports());
         var reportAdded = report.getVcmrs().stream()
-                .filter(vcmr -> vcmr.getId().equals(changeRequestId))
+                .filter(vcmr -> vcmr.id().equals(changeRequestId))
                 .count() == 1;
         assertTrue(reportAdded);
     }