Always require upgrade budget for OS upgrades in controller

8 files changed, 26 insertions, 36 deletions

diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
index 19a84ec93f0..1b43fe2aaa5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MetricsReporterTest.java
@@ -24,7 +24,6 @@ import org.junit.Test;
 import java.time.Duration;
 import java.util.Comparator;
 import java.util.List;
-import java.util.Optional;
 import java.util.Set;
 import java.util.function.UnaryOperator;
 import java.util.stream.Collectors;
@@ -353,7 +352,7 @@ public class MetricsReporterTest {
 
         // All nodes upgrade to initial OS version
         var version0 = Version.fromString("8.0");
-        tester.controller().upgradeOsIn(cloud, version0, Optional.empty(), false);
+        tester.controller().upgradeOsIn(cloud, version0, Duration.ZERO, false);
         osUpgrader.maintain();
         tester.configServer().setOsVersion(version0, SystemApplication.tenantHost.id(), zone);
         tester.configServer().setOsVersion(version0, SystemApplication.configServerHost.id(), zone);
@@ -367,7 +366,7 @@ public class MetricsReporterTest {
             var currentVersion = i == 0 ? version0 : targets.get(i - 1);
             var nextVersion = targets.get(i);
             // System starts upgrading to next OS version
-            tester.controller().upgradeOsIn(cloud, nextVersion, Optional.empty(), false);
+            tester.controller().upgradeOsIn(cloud, nextVersion, Duration.ZERO, false);
             runAll(osUpgrader, statusUpdater, reporter);
             assertOsChangeDuration(Duration.ZERO, hosts);
             assertOsNodeCount(hosts.size(), currentVersion);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgradeSchedulerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgradeSchedulerTest.java
index 4aed9b0bffe..76781d964a1 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgradeSchedulerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsUpgradeSchedulerTest.java
@@ -11,7 +11,6 @@ import org.junit.Test;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.List;
-import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
@@ -38,7 +37,7 @@ public class OsUpgradeSchedulerTest {
 
         // Target is set
         Version version0 = Version.fromString("7.0.0.20210123190005");
-        tester.controller().upgradeOsIn(cloud, version0, Optional.of(Duration.ofDays(1)), false);
+        tester.controller().upgradeOsIn(cloud, version0, Duration.ofDays(1), false);
 
         // Target remains unchanged as it hasn't expired yet
         for (var interval : List.of(Duration.ZERO, Duration.ofDays(15))) {
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 42057e7880c..3e2fd4ec0b9 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
@@ -17,13 +17,11 @@ import org.junit.Test;
 import java.time.Duration;
 import java.util.Collection;
 import java.util.List;
-import java.util.Optional;
 import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 
 /**
  * @author mpolden
@@ -67,8 +65,8 @@ public class OsUpgraderTest {
 
         // New OS version released
         Version version1 = Version.fromString("7.1");
-        tester.controller().upgradeOsIn(cloud1, Version.fromString("7.0"), Optional.empty(), false);
-        tester.controller().upgradeOsIn(cloud1, version1, Optional.empty(), false);
+        tester.controller().upgradeOsIn(cloud1, Version.fromString("7.0"), Duration.ZERO, false);
+        tester.controller().upgradeOsIn(cloud1, version1, Duration.ZERO, false);
         assertEquals(1, tester.controller().osVersionTargets().size()); // Only allows one version per cloud
         statusUpdater.maintain();
 
@@ -136,16 +134,10 @@ public class OsUpgraderTest {
         tester.configServer().bootstrap(List.of(zone1.getId(), zone2.getId(), zone3.getId(), zone4.getId()),
                                         nodeTypes);
 
-        // Upgrade without budget fails
-        Version version = Version.fromString("7.1");
-        try {
-            tester.controller().upgradeOsIn(cloud, version, Optional.empty(), false);
-            fail("Expected exception");
-        } catch (IllegalArgumentException ignored) {}
-
         // Upgrade with budget
-        tester.controller().upgradeOsIn(cloud, version, Optional.of(Duration.ofHours(12)), false);
-        assertEquals(Duration.ofHours(12), tester.controller().osVersionTarget(cloud).get().upgradeBudget().get());
+        Version version = Version.fromString("7.1");
+        tester.controller().upgradeOsIn(cloud, version, Duration.ofHours(12), false);
+        assertEquals(Duration.ofHours(12), tester.controller().osVersionTarget(cloud).get().upgradeBudget());
         statusUpdater.maintain();
         osUpgrader.maintain();
 
@@ -195,8 +187,8 @@ public class OsUpgraderTest {
 
         // New OS version released
         Version version = Version.fromString("7.1");
-        tester.controller().upgradeOsIn(cloud, Version.fromString("7.0"), Optional.empty(), false);
-        tester.controller().upgradeOsIn(cloud, version, Optional.empty(), false);
+        tester.controller().upgradeOsIn(cloud, Version.fromString("7.0"), Duration.ZERO, false);
+        tester.controller().upgradeOsIn(cloud, version, Duration.ZERO, false);
         statusUpdater.maintain();
 
         // zone 1 upgrades
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java
index e9a1adcfe88..95f6af06230 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OsVersionStatusUpdaterTest.java
@@ -11,7 +11,6 @@ import com.yahoo.vespa.hosted.controller.versions.OsVersionStatus;
 import org.junit.Test;
 
 import java.time.Duration;
-import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -41,7 +40,7 @@ public class OsVersionStatusUpdaterTest {
         // Setting a new target adds it to current status
         Version version1 = Version.fromString("7.1");
         CloudName cloud = CloudName.defaultName();
-        tester.controller().upgradeOsIn(cloud, version1, Optional.empty(), false);
+        tester.controller().upgradeOsIn(cloud, version1, Duration.ZERO, false);
         statusUpdater.maintain();
 
         var osVersions = tester.controller().osVersionStatus().versions();
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java
index dea09998952..6a3632ec2c9 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/OsVersionTargetSerializerTest.java
@@ -9,7 +9,6 @@ import com.yahoo.vespa.hosted.controller.versions.OsVersionTarget;
 import org.junit.Test;
 
 import java.time.Duration;
-import java.util.Optional;
 import java.util.Set;
 
 import static org.junit.Assert.assertEquals;
@@ -23,8 +22,8 @@ public class OsVersionTargetSerializerTest {
     public void serialization() {
         OsVersionTargetSerializer serializer = new OsVersionTargetSerializer(new OsVersionSerializer());
         Set<OsVersionTarget> targets = ImmutableSet.of(
-                new OsVersionTarget(new OsVersion(Version.fromString("7.1"), CloudName.defaultName()), Optional.empty()),
-                new OsVersionTarget(new OsVersion(Version.fromString("7.1"), CloudName.from("foo")), Optional.of(Duration.ofDays(1)))
+                new OsVersionTarget(new OsVersion(Version.fromString("7.1"), CloudName.defaultName()), Duration.ZERO),
+                new OsVersionTarget(new OsVersion(Version.fromString("7.1"), CloudName.from("foo")), Duration.ofDays(1))
         );
         Set<OsVersionTarget> serialized = serializer.fromSlime(serializer.toSlime(targets));
         assertEquals(targets, serialized);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java
index 857530df1fe..7364723f5f0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/OsApiTest.java
@@ -69,8 +69,8 @@ public class OsApiTest extends ControllerContainerTest {
         // All nodes are initially on empty version
         upgradeAndUpdateStatus();
         // Upgrade OS to a different version in each cloud
-        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.5.2\", \"cloud\": \"cloud1\"}", Request.Method.PATCH),
-                       "{\"message\":\"Set target OS version for cloud 'cloud1' to 7.5.2\"}", 200);
+        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.5.2\", \"cloud\": \"cloud1\", \"upgradeBudget\": \"PT0S\"}", Request.Method.PATCH),
+                       "{\"message\":\"Set target OS version for cloud 'cloud1' to 7.5.2 with upgrade budget PT0S\"}", 200);
         assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"8.2.1\", \"cloud\": \"cloud2\", \"upgradeBudget\": \"PT24H\"}", Request.Method.PATCH),
                        "{\"message\":\"Set target OS version for cloud 'cloud2' to 8.2.1 with upgrade budget PT24H\"}", 200);
 
@@ -85,27 +85,27 @@ public class OsApiTest extends ControllerContainerTest {
         assertFile(new Request("http://localhost:8080/os/v1/"), "versions-all-upgraded.json");
 
         // Downgrade with force is permitted
-        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.5.1\", \"cloud\": \"cloud1\", \"force\": true}", Request.Method.PATCH),
-                       "{\"message\":\"Set target OS version for cloud 'cloud1' to 7.5.1\"}", 200);
+        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.5.1\", \"cloud\": \"cloud1\", \"force\": true, \"upgradeBudget\": \"PT0S\"}", Request.Method.PATCH),
+                       "{\"message\":\"Set target OS version for cloud 'cloud1' to 7.5.1 with upgrade budget PT0S\"}", 200);
 
-        // Error: Missing field 'cloud' or 'version'
+        // Error: Missing fields
         assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.6\"}", Request.Method.PATCH),
-                       "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Fields 'version' and 'cloud' are required\"}", 400);
+                       "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Fields 'version', 'cloud' and 'upgradeBudget' are required\"}", 400);
         assertResponse(new Request("http://localhost:8080/os/v1/", "{\"cloud\": \"cloud1\"}", Request.Method.PATCH),
-                       "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Fields 'version' and 'cloud' are required\"}", 400);
+                       "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Fields 'version', 'cloud' and 'upgradeBudget' are required\"}", 400);
 
         // Error: Invalid versions
-        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": null, \"cloud\": \"cloud1\"}", Request.Method.PATCH),
+        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": null, \"cloud\": \"cloud1\", \"upgradeBudget\": \"PT0S\"}", Request.Method.PATCH),
                        "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Invalid version '0.0.0'\"}", 400);
-        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"foo\", \"cloud\": \"cloud1\"}", Request.Method.PATCH),
+        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"foo\", \"cloud\": \"cloud1\", \"upgradeBudget\": \"PT0S\"}", Request.Method.PATCH),
                        "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Invalid version 'foo': For input string: \\\"foo\\\"\"}", 400);
 
         // Error: Invalid cloud
-        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.6\", \"cloud\": \"foo\"}", Request.Method.PATCH),
+        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.6\", \"cloud\": \"foo\", \"upgradeBudget\": \"PT0S\"}", Request.Method.PATCH),
                        "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Cloud 'foo' does not exist in this system\"}", 400);
 
         // Error: Downgrade OS
-        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.4.1\", \"cloud\": \"cloud1\"}", Request.Method.PATCH),
+        assertResponse(new Request("http://localhost:8080/os/v1/", "{\"version\": \"7.4.1\", \"cloud\": \"cloud1\", \"upgradeBudget\": \"PT0S\"}", Request.Method.PATCH),
                        "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Cannot downgrade cloud 'cloud1' to version 7.4.1\"}", 400);
 
         // Request firmware checks in all zones.
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-all-upgraded.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-all-upgraded.json
index 4350b70f09f..e1b70c780c5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-all-upgraded.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-all-upgraded.json
@@ -3,6 +3,7 @@
     {
       "version": "7.5.2",
       "targetVersion": true,
+      "upgradeBudget": "PT0S",
       "cloud": "cloud1",
       "nodes": [
         {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-partially-upgraded.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-partially-upgraded.json
index dcdecaaa2cc..a9186a9d733 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-partially-upgraded.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/os/responses/versions-partially-upgraded.json
@@ -55,6 +55,7 @@
     {
       "version": "7.5.2",
       "targetVersion": true,
+      "upgradeBudget": "PT0S",
       "cloud": "cloud1",
       "nodes": [
         {