Merge pull request #28400 from vespa-engine/mpolden/lb-fix

Never clear load balancer instance

4 files changed, 23 insertions, 18 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
index 74f2225adb5..a5aa7f42bc4 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/lb/LoadBalancer.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.provision.lb;
 
 import com.yahoo.vespa.applicationmodel.InfrastructureApplication;
 import com.yahoo.vespa.hosted.provision.maintenance.LoadBalancerExpirer;
-import com.yahoo.vespa.service.duper.ConfigServerApplication;
 
 import java.time.Instant;
 import java.util.Objects;
@@ -69,8 +68,8 @@ public class LoadBalancer {
     }
 
     /** Returns a copy of this with instance set to given instance */
-    public LoadBalancer with(Optional<LoadBalancerInstance> instance) {
-        return new LoadBalancer(id, instance, state, changedAt);
+    public LoadBalancer with(LoadBalancerInstance instance) {
+        return new LoadBalancer(id, Optional.of(instance), state, changedAt);
     }
 
     public enum State {
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 baa2e596b36..895fdd522e7 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
@@ -116,7 +116,7 @@ public class LoadBalancerExpirer extends NodeRepositoryMaintainer {
                                                                   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());
+                db.writeLoadBalancer(lb.with(instance), lb.state());
             } catch (Exception e) {
                 failed.add(lb.id());
                 lastException.set(e);
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 5506fdf8ea3..c414c70f315 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
@@ -207,14 +207,11 @@ public class LoadBalancerProvisioner {
                 throw new LoadBalancerServiceException("Could not (re)configure " + id + " due to change in load balancer visibility. The operation will be retried on next deployment");
             }
             LoadBalancerInstance instance = provisionInstance(id, loadBalancer, zoneEndpoint, cloudAccount);
-            newLoadBalancer = newLoadBalancer.with(Optional.of(instance));
-        }
-        catch (LoadBalancerServiceException e) {
+            newLoadBalancer = newLoadBalancer.with(instance);
+        } catch (LoadBalancerServiceException e) {
             log.log(Level.WARNING, "Failed to provision load balancer", e);
-            newLoadBalancer = newLoadBalancer.with(Optional.empty());
             throw e;
-        }
-        finally {
+        } finally {
             db.writeLoadBalancer(newLoadBalancer, fromState);
         }
     }
@@ -233,21 +230,19 @@ public class LoadBalancerProvisioner {
 
         try {
             LoadBalancerInstance instance = configureInstance(id, nodes, loadBalancer.get(), settings, loadBalancer.get().instance().get().cloudAccount());
-            db.writeLoadBalancers(List.of(loadBalancer.get().with(Optional.of(instance)).with(State.active, now)),
-                                  loadBalancer.get().state(), transaction.nested());
-        }
-        catch (LoadBalancerServiceException e) {
-            db.writeLoadBalancers(List.of(loadBalancer.get().with(Optional.empty())),
+            db.writeLoadBalancers(List.of(loadBalancer.get().with(instance).with(State.active, now)),
                                   loadBalancer.get().state(), transaction.nested());
+        } catch (LoadBalancerServiceException e) {
+            db.writeLoadBalancers(List.of(loadBalancer.get()), loadBalancer.get().state(), transaction.nested());
             throw e;
         }
     }
 
     /** Provision a load balancer instance, if necessary */
     private LoadBalancerInstance provisionInstance(LoadBalancerId id,
-                                                             Optional<LoadBalancer> currentLoadBalancer,
-                                                             ZoneEndpoint zoneEndpoint,
-                                                             CloudAccount cloudAccount) {
+                                                   Optional<LoadBalancer> currentLoadBalancer,
+                                                   ZoneEndpoint zoneEndpoint,
+                                                   CloudAccount cloudAccount) {
         Set<Real> reals = currentLoadBalancer.flatMap(LoadBalancer::instance)
                                              .map(LoadBalancerInstance::reals)
                                              .orElse(Set.of()); // Targeted reals are changed on activation.
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
index 53054ba3f24..2b36bacf1b1 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
@@ -300,6 +300,17 @@ public class LoadBalancerProvisionerTest {
         loadBalancers = lbs.get();
         assertSame(LoadBalancer.State.active, loadBalancers.get(0).state());
         assertTrue("Load balancer has instance", loadBalancers.get(0).instance().isPresent());
+
+        // Reconfiguration of load balancer fails on next prepare, but instance is preserved
+        tester.loadBalancerService().throwOnCreate(true);
+        ZoneEndpoint settings = new ZoneEndpoint(true, true, List.of(new AllowedUrn(AccessType.awsPrivateLink, "alice"), new AllowedUrn(AccessType.gcpServiceConnect, "bob")));
+        try {
+            prepare(app1, clusterRequest(ClusterSpec.Type.container, cluster, Optional.empty(), settings));
+            fail("Expected exception");
+        } catch (LoadBalancerServiceException ignored) {
+        }
+        assertSame(LoadBalancer.State.active, loadBalancers.get(0).state());
+        assertTrue("Load balancer has instance", loadBalancers.get(0).instance().isPresent());
     }
 
     @Test