diff options
author | Jon Bratseth <bratseth@gmail.com> | 2022-12-29 16:45:48 +0100 |
---|---|---|
committer | Jon Bratseth <bratseth@gmail.com> | 2022-12-29 16:45:48 +0100 |
commit | 2279ce96234d01cf039a319ce390a96e431437cc (patch) | |
tree | d4cee03eb393fe56b33511fe505999dfa181d72a /node-repository | |
parent | 6391f836b21b2d22510b70d554863d432536a59b (diff) |
Get rid of Advice.present
Diffstat (limited to 'node-repository')
5 files changed, 25 insertions, 31 deletions
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java index c816abc060c..0d643dc070b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java @@ -124,13 +124,11 @@ public class Autoscaler { public static class Advice { - private final boolean present; private final Optional<ClusterResources> target; private final AutoscalingStatus reason; - private Advice(Optional<ClusterResources> target, boolean present, AutoscalingStatus reason) { + private Advice(Optional<ClusterResources> target, AutoscalingStatus reason) { this.target = target; - this.present = present; this.reason = Objects.requireNonNull(reason); } @@ -140,33 +138,26 @@ public class Autoscaler { */ public Optional<ClusterResources> target() { return target; } - /** True if this does not provide any advice */ - public boolean isEmpty() { return ! present; } - - /** True if this provides advice (which may be to keep the current allocation) */ - public boolean isPresent() { return present; } - /** The reason for this advice */ public AutoscalingStatus reason() { return reason; } private static Advice none(Status status, String description) { - return new Advice(Optional.empty(), false, new AutoscalingStatus(status, description)); + return new Advice(Optional.empty(), new AutoscalingStatus(status, description)); } private static Advice dontScale(Status status, String description) { - return new Advice(Optional.empty(), true, new AutoscalingStatus(status, description)); + return new Advice(Optional.empty(), new AutoscalingStatus(status, description)); } private static Advice scaleTo(ClusterResources target) { - return new Advice(Optional.of(target), true, + return new Advice(Optional.of(target), new AutoscalingStatus(AutoscalingStatus.Status.rescaling, "Rescaling initiated due to load changes")); } @Override public String toString() { - return "autoscaling advice: " + - (present ? (target.isPresent() ? "Scale to " + target.get() : "Don't scale") : "None"); + return "autoscaling advice: " + (target.isPresent() ? "Scale to " + target.get() : "Don't scale"); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java index 65f6eecf790..7eedad98697 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java @@ -87,7 +87,7 @@ public class AutoscalingMaintainer extends NodeRepositoryMaintainer { applications().put(application.get().with(updatedCluster), lock); var current = new AllocatableClusterResources(clusterNodes, nodeRepository()).advertisedResources(); - if (advice.isPresent() && advice.target().isPresent() && !current.equals(advice.target().get())) { + if (advice.target().isPresent() && !current.equals(advice.target().get())) { // 2. Also autoscale try (MaintenanceDeployment deployment = new MaintenanceDeployment(applicationId, deployer, metric, nodeRepository())) { if (deployment.isValid()) { @@ -101,7 +101,7 @@ public class AutoscalingMaintainer extends NodeRepositoryMaintainer { private boolean anyChanges(Autoscaler.Advice advice, Cluster cluster, Cluster updatedCluster, NodeList clusterNodes) { if (updatedCluster != cluster) return true; - if (advice.isPresent() && !cluster.target().resources().equals(advice.target())) return true; + if (!cluster.target().resources().equals(advice.target())) return true; if ( ! advice.reason().equals(cluster.target().status())) return true; if (advice.target().isPresent() && !advice.target().get().equals(new AllocatableClusterResources(clusterNodes, nodeRepository()).advertisedResources())) return true; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java index 0201ffae103..248329966cc 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java @@ -64,11 +64,14 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer { Optional<Cluster> cluster = application.cluster(clusterId); if (cluster.isEmpty()) return true; var suggestion = autoscaler.suggest(application, cluster.get(), clusterNodes); - if (suggestion.isEmpty()) return true; + if (suggestion.reason().status() == AutoscalingStatus.Status.waiting) return true; + + // empty suggested resources == keep the current allocation + var suggestedResources = suggestion.target().orElse(clusterNodes.not().retired().toResources()); + if ( ! shouldUpdateSuggestion(cluster.get().suggested(), suggestedResources)) return true; + // Wait only a short time for the lock to avoid interfering with change deployments try (Mutex lock = nodeRepository().applications().lock(applicationId, Duration.ofSeconds(1))) { - // empty suggested resources == keep the current allocation, so we record that - var suggestedResources = suggestion.target().orElse(clusterNodes.not().retired().toResources()); applications().get(applicationId).ifPresent(a -> updateSuggestion(suggestedResources, clusterId, a, lock)); return true; } @@ -77,20 +80,21 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer { } } + private boolean shouldUpdateSuggestion(Autoscaling currentSuggestion, ClusterResources suggestedResources) { + return currentSuggestion.resources().isEmpty() + || currentSuggestion.at().isBefore(nodeRepository().clock().instant().minus(Duration.ofDays(7))) + || isHigher(suggestedResources, currentSuggestion.resources().get()); + } + private void updateSuggestion(ClusterResources suggestion, ClusterSpec.Id clusterId, Application application, Mutex lock) { Optional<Cluster> cluster = application.cluster(clusterId); if (cluster.isEmpty()) return; - var at = nodeRepository().clock().instant(); - var currentSuggestion = cluster.get().suggested(); - if (currentSuggestion.resources().isEmpty() - || currentSuggestion.at().isBefore(at.minus(Duration.ofDays(7))) - || isHigher(suggestion, currentSuggestion.resources().get())) - applications().put(application.with(cluster.get().withSuggested(new Autoscaling(suggestion, - AutoscalingStatus.empty(), - at))), lock); + applications().put(application.with(cluster.get().withSuggested(new Autoscaling(suggestion, + AutoscalingStatus.empty(), + nodeRepository().clock().instant()))), lock); } private boolean isHigher(ClusterResources r1, ClusterResources r2) { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingIntegrationTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingIntegrationTest.java index c65ebae9b3b..158c5116e19 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingIntegrationTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingIntegrationTest.java @@ -44,7 +44,6 @@ public class AutoscalingIntegrationTest { } var scaledResources = autoscaler.suggest(fixture.application(), fixture.cluster(), fixture.nodes()); - assertTrue(scaledResources.isPresent()); } private static class MockHttpClient implements MetricsV2MetricsFetcher.AsyncHttpClient { diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java index abe25b05955..7b5f573980c 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java @@ -12,6 +12,7 @@ import static com.yahoo.config.provision.NodeResources.DiskSpeed.slow; import com.yahoo.config.provision.NodeResources.StorageType; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.Zone; +import com.yahoo.vespa.hosted.provision.applications.AutoscalingStatus; import com.yahoo.vespa.hosted.provision.provisioning.CapacityPolicies; import org.junit.Test; @@ -36,7 +37,7 @@ public class AutoscalingTest { fixture.autoscale()); fixture.deploy(Capacity.from(scaledResources)); - assertTrue("Cluster in flux -> No further change", fixture.autoscale().isEmpty()); + assertEquals("Cluster in flux -> No further change", AutoscalingStatus.Status.waiting, fixture.autoscale().reason().status()); fixture.deactivateRetired(Capacity.from(scaledResources)); @@ -357,10 +358,9 @@ public class AutoscalingTest { ClusterResources min = new ClusterResources( 2, 1, new NodeResources(1, 1, 1, 1)); var fixture = AutoscalingTester.fixture().awsProdSetup(true).capacity(Capacity.from(min, min)).build(); - // deploy fixture.tester().clock().advance(Duration.ofDays(1)); fixture.loader().applyCpuLoad(0.25, 120); - assertTrue(fixture.autoscale().isEmpty()); + assertEquals(AutoscalingStatus.Status.unavailable, fixture.autoscale().reason().status()); } @Test |