Let implementations short-circuit LB (re)configuration

7 files changed, 29 insertions, 29 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java
index f166e73da77..e228d31384c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerInstance.java
@@ -110,9 +110,18 @@ public class LoadBalancerInstance {
         return ports;
     }
 
-    /** Prepends the given service IDs, possibly replacing those we have in this. */
+    /** Updates this with new data, from a reconfiguration. */
+    public LoadBalancerInstance with(Set<Real> reals, ZoneEndpoint settings, Optional<PrivateServiceId> serviceId) {
+        List<PrivateServiceId> ids = new ArrayList<>(serviceIds);
+        serviceId.filter(id -> ! ids.contains(id)).ifPresent(ids::add);
+        return new LoadBalancerInstance(hostname, ipAddress, dnsZone, ports, networks,
+                                        reals, settings, ids,
+                                        cloudAccount);
+    }
+
+    /** Prepends the given service IDs, possibly changing the order of those we have in this. */
     public LoadBalancerInstance withServiceIds(List<PrivateServiceId> serviceIds) {
-        List<PrivateServiceId> ids = new ArrayList<>(serviceIds());
+        List<PrivateServiceId> ids = new ArrayList<>(serviceIds);
         for (PrivateServiceId id : this.serviceIds) if ( ! ids.contains(id)) ids.add(id);
         return new LoadBalancerInstance(hostname, ipAddress, dnsZone, ports, networks, reals, settings, ids, cloudAccount);
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java
index 82f679bbe34..ceedbcf89c2 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerService.java
@@ -22,12 +22,13 @@ public interface LoadBalancerService {
     /**
      * Configures load balancers for the given specification. Implementations are expected to be idempotent
      *
+     * @param instance    Load balancer instance to reconfigure
      * @param spec        Load balancer specification
      * @param force       Whether reconfiguration should be forced (e.g. allow configuring an empty set of reals on a
      *                    pre-existing load balancer, or removing an unused private endpoint service load balancer).
      * @return The (re)configured load balancer instance
      */
-    LoadBalancerInstance configure(LoadBalancerSpec spec, boolean force);
+    LoadBalancerInstance configure(LoadBalancerInstance instance, LoadBalancerSpec spec, boolean force);
 
     /** Permanently remove given load balancer */
     void remove(LoadBalancer loadBalancer);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java
index d35c11783dd..1f354dc7081 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancerServiceMock.java
@@ -68,25 +68,17 @@ public class LoadBalancerServiceMock implements LoadBalancerService {
     }
 
     @Override
-    public LoadBalancerInstance configure(LoadBalancerSpec spec, boolean force) {
-        if (throwOnCreate) throw new IllegalStateException("Did not expect a new load balancer to be created");
+    public LoadBalancerInstance configure(LoadBalancerInstance instance, LoadBalancerSpec spec, boolean force) {
         var id = new LoadBalancerId(spec.application(), spec.cluster());
         var oldInstance = Objects.requireNonNull(instances.get(id), "expected existing load balancer " + id);
         if (!force && !oldInstance.reals().isEmpty() && spec.reals().isEmpty()) {
             throw new IllegalArgumentException("Refusing to remove all reals from load balancer " + id);
         }
-        var instance = new LoadBalancerInstance(
-                Optional.of(DomainName.of("lb-" + spec.application().toShortString() + "-" + spec.cluster().value())),
-                Optional.empty(),
-                Optional.of(new DnsZone("zone-id-1")),
-                Collections.singleton(4443),
-                ImmutableSet.of("10.2.3.0/24", "10.4.5.0/24"),
-                spec.reals(),
-                spec.settings(),
-                spec.settings().isPrivateEndpoint() ? List.of(PrivateServiceId.of("service")) : List.of(),
-                spec.cloudAccount());
-        instances.put(id, instance);
-        return instance;
+        var updated = instance.with(spec.reals(),
+                                    spec.settings(),
+                                    spec.settings().isPrivateEndpoint() ? Optional.of(PrivateServiceId.of("service")) : Optional.empty());
+        instances.put(id, updated);
+        return updated;
     }
 
     @Override
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java
index 432b56c7b48..22367f72666 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerService.java
@@ -33,8 +33,8 @@ public class SharedLoadBalancerService implements LoadBalancerService {
     }
 
     @Override
-    public LoadBalancerInstance configure(LoadBalancerSpec spec, boolean force) {
-        return create(spec);
+    public LoadBalancerInstance configure(LoadBalancerInstance instance, LoadBalancerSpec spec, boolean force) {
+        return instance.with(spec.reals(), spec.settings(), Optional.empty());
     }
 
     private LoadBalancerInstance create(LoadBalancerSpec spec) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
index b1beb615bc3..f864ab18920 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
@@ -112,7 +112,8 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
             try {
                 attempts.add(1);
                 LOG.log(Level.INFO, () -> "Removing reals from inactive load balancer " + lb.id() + ": " + Sets.difference(lb.instance().get().reals(), reals));
-                LoadBalancerInstance instance = service.configure(new LoadBalancerSpec(lb.id().application(), lb.id().cluster(), reals,
+                LoadBalancerInstance instance = service.configure(lb.instance().get(),
+                                                                  new LoadBalancerSpec(lb.id().application(), lb.id().cluster(), reals,
                                                                                        lb.instance().get().settings(), lb.instance().get().cloudAccount()),
                                                                   true);
                 db.writeLoadBalancer(lb.with(Optional.of(instance)), lb.state());
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
index 494d50f6a45..4f83ad69893 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
@@ -259,7 +259,7 @@ public class LoadBalancerProvisioner {
         return Optional.empty(); // Will cause activation to fail, but lets us proceed with more preparations.
     }
 
-    /** Provision or reconfigure a load balancer instance, if necessary */
+    /** Reconfigure a load balancer instance, if necessary */
     private Optional<LoadBalancerInstance> configureInstance(LoadBalancerId id, NodeList nodes,
                                                              LoadBalancer currentLoadBalancer,
                                                              ZoneEndpoint zoneEndpoint,
@@ -268,14 +268,11 @@ public class LoadBalancerProvisioner {
                                                                  id.application().serializedForm())
                                                            .value();
         Set<Real> reals = shouldDeactivateRouting ? Set.of() : realsOf(nodes);
-        if (isUpToDate(currentLoadBalancer, reals, zoneEndpoint))
-            return currentLoadBalancer.instance();
-
         log.log(Level.INFO, () -> "Configuring instance for " + id + ", targeting: " + reals);
         try {
-            return Optional.of(service.configure(new LoadBalancerSpec(id.application(), id.cluster(), reals, zoneEndpoint, cloudAccount),
-                                                 shouldDeactivateRouting || currentLoadBalancer.state() != LoadBalancer.State.active)
-                                      .withServiceIds(currentLoadBalancer.instance().map(LoadBalancerInstance::serviceIds).orElse(List.of())));
+            return Optional.of(service.configure(currentLoadBalancer.instance().orElseThrow(() -> new IllegalArgumentException("expected existing instance for " + id)),
+                                                 new LoadBalancerSpec(id.application(), id.cluster(), reals, zoneEndpoint, cloudAccount),
+                                                 shouldDeactivateRouting || currentLoadBalancer.state() != LoadBalancer.State.active));
         } catch (Exception e) {
             log.log(Level.WARNING, e, () -> "Could not (re)configure " + id + ", targeting: " +
                                             reals + ". The operation will be retried on next deployment");
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java
index ccfe96d462c..1a6058bed39 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/lb/SharedLoadBalancerServiceTest.java
@@ -30,8 +30,8 @@ public class SharedLoadBalancerServiceTest {
     public void test_create_lb() {
         LoadBalancerSpec spec = new LoadBalancerSpec(applicationId, clusterId, reals,
                                                      ZoneEndpoint.defaultEndpoint, CloudAccount.empty);
-        loadBalancerService.provision(spec);
-        var lb = loadBalancerService.configure(spec, false);
+
+        var lb = loadBalancerService.configure(loadBalancerService.provision(spec), spec, false);
 
         assertEquals(Optional.of(HostName.of("vip.example.com")), lb.hostname());
         assertEquals(Optional.empty(), lb.dnsZone());