Merge pull request #17135 from vespa-engine/mpolden/use-virtual-id

Always use virtual zone ID in OsUpgrader

4 files changed, 40 insertions, 12 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java
index 92ebf4ee4b6..79bea294472 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgrader.java
@@ -47,9 +47,9 @@ public class OsUpgrader extends InfrastructureUpgrader<OsVersionTarget> {
                                                      .map(totalBudget -> zoneBudgetOf(totalBudget, zone));
         log.info(String.format("Upgrading OS of %s to version %s in %s in cloud %s%s", application.id(),
                                target.osVersion().version().toFullString(),
-                               zone.getId(), zone.getCloudName(),
+                               zone.getVirtualId(), zone.getCloudName(),
                                zoneUpgradeBudget.map(d -> " with time budget " + d).orElse("")));
-        controller().serviceRegistry().configServer().nodeRepository().upgradeOs(zone.getId(), application.nodeType(),
+        controller().serviceRegistry().configServer().nodeRepository().upgradeOs(zone.getVirtualId(), application.nodeType(),
                                                                                  target.osVersion().version(),
                                                                                  zoneUpgradeBudget);
     }
@@ -78,7 +78,7 @@ public class OsUpgrader extends InfrastructureUpgrader<OsVersionTarget> {
     protected boolean changeTargetTo(OsVersionTarget target, SystemApplication application, ZoneApi zone) {
         if (!application.shouldUpgradeOs()) return false;
         return controller().serviceRegistry().configServer().nodeRepository()
-                           .targetVersionsOf(zone.getId())
+                           .targetVersionsOf(zone.getVirtualId())
                            .osVersion(application.nodeType())
                            .map(currentTarget -> target.osVersion().version().isAfter(currentTarget))
                            .orElse(true);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java
index 226852f1f3d..8fd5f07b9ea 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/OsVersionStatus.java
@@ -65,14 +65,14 @@ public class OsVersionStatus {
             for (var zone : zonesToUpgrade(controller)) {
                 if (!application.shouldUpgradeOs()) continue;
                 var targetOsVersion = controller.serviceRegistry().configServer().nodeRepository()
-                                                .targetVersionsOf(zone.getId())
+                                                .targetVersionsOf(zone.getVirtualId())
                                                 .osVersion(application.nodeType())
                                                 .orElse(Version.emptyVersion);
 
-                for (var node : controller.serviceRegistry().configServer().nodeRepository().list(zone.getId(), application.id())) {
+                for (var node : controller.serviceRegistry().configServer().nodeRepository().list(zone.getVirtualId(), application.id())) {
                     if (!OsUpgrader.canUpgrade(node)) continue;
                     var suspendedAt = node.suspendedSince();
-                    var nodeVersion = new NodeVersion(node.hostname(), zone.getId(), node.currentOsVersion(),
+                    var nodeVersion = new NodeVersion(node.hostname(), zone.getVirtualId(), node.currentOsVersion(),
                                                       targetOsVersion, suspendedAt);
                     var osVersion = new OsVersion(nodeVersion.currentVersion(), zone.getCloudName());
                     osVersions.putIfAbsent(osVersion, new ArrayList<>());
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java
index af0e3d5807d..3d87f2d7190 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java
@@ -14,18 +14,24 @@ import java.util.Objects;
  * @author hakonhall
  */
 public class ZoneApiMock implements ZoneApi {
+
     private final SystemName systemName;
     private final ZoneId id;
+    private final ZoneId virtualId;
     private final CloudName cloudName;
     private final String cloudNativeRegionName;
 
     public static Builder newBuilder() { return new Builder(); }
 
-    private ZoneApiMock(SystemName systemName, ZoneId id, CloudName cloudName, String cloudNativeRegionName) {
+    private ZoneApiMock(SystemName systemName, ZoneId id, ZoneId virtualId, CloudName cloudName, String cloudNativeRegionName) {
         this.systemName = systemName;
         this.id = id;
+        this.virtualId = virtualId;
         this.cloudName = cloudName;
         this.cloudNativeRegionName = cloudNativeRegionName;
+        if (virtualId != null && virtualId.equals(id)) {
+            throw new IllegalArgumentException("Virtual ID cannot be equal to zone ID: " + id);
+        }
     }
 
     public static ZoneApiMock fromId(String id) {
@@ -47,6 +53,11 @@ public class ZoneApiMock implements ZoneApi {
     public ZoneId getId() { return id; }
 
     @Override
+    public ZoneId getVirtualId() {
+        return virtualId == null ? getId() : virtualId;
+    }
+
+    @Override
     public CloudName getCloudName() { return cloudName; }
 
     @Override
@@ -66,8 +77,11 @@ public class ZoneApiMock implements ZoneApi {
     }
 
     public static class Builder {
+
         private final SystemName systemName = SystemName.defaultSystem();
+
         private ZoneId id = ZoneId.defaultId();
+        private ZoneId virtualId ;
         private CloudName cloudName = CloudName.defaultName();
         private String cloudNativeRegionName = id.region().value();
 
@@ -78,6 +92,15 @@ public class ZoneApiMock implements ZoneApi {
 
         public Builder withId(String id) { return with(ZoneId.from(id)); }
 
+        public Builder withVirtualId(ZoneId virtualId) {
+            this.virtualId = virtualId;
+            return this;
+        }
+
+        public Builder withVirtualId(String virtualId) {
+            return withVirtualId(ZoneId.from(virtualId));
+        }
+
         public Builder with(CloudName cloudName) {
             this.cloudName = cloudName;
             return this;
@@ -91,7 +114,8 @@ public class ZoneApiMock implements ZoneApi {
         }
 
         public ZoneApiMock build() {
-            return new ZoneApiMock(systemName, id, cloudName, cloudNativeRegionName);
+            return new ZoneApiMock(systemName, id, virtualId, cloudName, cloudNativeRegionName);
         }
     }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java
index ccccf4f24d9..1bc633122a0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgraderTest.java
@@ -37,7 +37,7 @@ public class OsUpgraderTest {
     public void upgrade_os() {
         CloudName cloud1 = CloudName.from("c1");
         CloudName cloud2 = CloudName.from("c2");
-        ZoneApi zone0 = zone("prod.controller", cloud1);
+        ZoneApi zone0 = zone("prod.us-north-42", "prod.controller", cloud1);
         ZoneApi zone1 = zone("prod.eu-west-1", cloud1);
         ZoneApi zone2 = zone("prod.us-west-1", cloud1);
         ZoneApi zone3 = zone("prod.us-central-1", cloud1);
@@ -54,7 +54,7 @@ public class OsUpgraderTest {
         // Bootstrap system
         tester.configServer().bootstrap(List.of(zone1.getId(), zone2.getId(), zone3.getId(), zone4.getId(), zone5.getId()),
                                         List.of(SystemApplication.tenantHost));
-        tester.configServer().addNodes(List.of(zone0.getId()), List.of(SystemApplication.controllerHost));
+        tester.configServer().addNodes(List.of(zone0.getVirtualId()), List.of(SystemApplication.controllerHost));
 
         // Add system application that exists in a real system, but isn't eligible for OS upgrades
         tester.configServer().addNodes(List.of(zone1.getId(), zone2.getId(), zone3.getId(), zone4.getId(), zone5.getId()),
@@ -73,8 +73,8 @@ public class OsUpgraderTest {
 
         // zone 0: controllers upgrade first
         osUpgrader.maintain();
-        assertWanted(version1, SystemApplication.controllerHost, zone0.getId());
-        completeUpgrade(version1, SystemApplication.controllerHost, zone0.getId());
+        assertWanted(version1, SystemApplication.controllerHost, zone0.getVirtualId());
+        completeUpgrade(version1, SystemApplication.controllerHost, zone0.getVirtualId());
         statusUpdater.maintain();
         assertEquals(3, nodesOn(version1).size());
 
@@ -304,4 +304,8 @@ public class OsUpgraderTest {
         return ZoneApiMock.newBuilder().withId(id).with(cloud).build();
     }
 
+    private static ZoneApi zone(String id, String virtualId, CloudName cloud) {
+        return ZoneApiMock.newBuilder().withId(id).withVirtualId(virtualId).with(cloud).build();
+    }
+
 }