Ensure unique ports and reals

6 files changed, 55 insertions, 34 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 e59e17815bf..c07666cee63 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
@@ -1,12 +1,13 @@
 // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.lb;
 
-import com.google.common.collect.Ordering;
+import com.google.common.collect.ImmutableSortedSet;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.vespa.hosted.provision.maintenance.LoadBalancerExpirer;
 
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 
 /**
  * Represents a load balancer for an application.
@@ -17,15 +18,20 @@ public class LoadBalancer {
 
     private final LoadBalancerId id;
     private final HostName hostname;
-    private final List<Integer> ports;
-    private final List<Real> reals;
+    private final Set<Integer> ports;
+    private final Set<Real> reals;
     private final boolean inactive;
 
+    // TODO: Remove this when no longer used by internal code
     public LoadBalancer(LoadBalancerId id, HostName hostname, List<Integer> ports, List<Real> reals, boolean inactive) {
+        this(id, hostname,  ImmutableSortedSet.copyOf(ports), ImmutableSortedSet.copyOf(reals), inactive);
+    }
+
+    public LoadBalancer(LoadBalancerId id, HostName hostname, Set<Integer> ports, Set<Real> reals, boolean inactive) {
         this.id = Objects.requireNonNull(id, "id must be non-null");
         this.hostname = Objects.requireNonNull(hostname, "hostname must be non-null");
-        this.ports = Ordering.natural().immutableSortedCopy(requirePorts(ports));
-        this.reals = Ordering.natural().immutableSortedCopy(Objects.requireNonNull(reals, "targets must be non-null"));
+        this.ports = ImmutableSortedSet.copyOf(requirePorts(ports));
+        this.reals = ImmutableSortedSet.copyOf(Objects.requireNonNull(reals, "targets must be non-null"));
         this.inactive = inactive;
     }
 
@@ -40,12 +46,12 @@ public class LoadBalancer {
     }
 
     /** Listening port(s) of this load balancer */
-    public List<Integer> ports() {
+    public Set<Integer> ports() {
         return ports;
     }
 
     /** Real servers behind this load balancer */
-    public List<Real> reals() {
+    public Set<Real> reals() {
         return reals;
     }
 
@@ -62,7 +68,7 @@ public class LoadBalancer {
         return new LoadBalancer(id, hostname, ports, reals, true);
     }
 
-    private static List<Integer> requirePorts(List<Integer> ports) {
+    private static Set<Integer> requirePorts(Set<Integer> ports) {
         Objects.requireNonNull(ports, "ports must be non-null");
         if (ports.isEmpty()) {
             throw new IllegalArgumentException("ports must be non-empty");
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 b5f59414c65..048f3758ac5 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
@@ -4,7 +4,9 @@ package com.yahoo.vespa.hosted.provision.lb;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * A managed load balance service.
@@ -14,7 +16,16 @@ import java.util.List;
 public interface LoadBalancerService {
 
     /** Create a load balancer for given application cluster. Implementations are expected to be idempotent */
-    LoadBalancer create(ApplicationId application, ClusterSpec.Id cluster, List<Real> reals);
+    // TODO: Remove once removed from all implementations
+    default LoadBalancer create(ApplicationId application, ClusterSpec.Id cluster, List<Real> reals) {
+        return create(application, cluster, new HashSet<>(reals));
+    }
+
+    /** Create a load balancer for given application cluster. Implementations are expected to be idempotent */
+    // TODO: Remove default implementation once implemented everywhere
+    default LoadBalancer create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals) {
+        throw new UnsupportedOperationException();
+    }
 
     /** Permanently remove load balancer with given ID */
     void remove(LoadBalancerId 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 a5a0d8cb2f8..c3ec3ea9402 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
@@ -7,8 +7,8 @@ import com.yahoo.config.provision.HostName;
 
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * @author mpolden
@@ -27,11 +27,11 @@ public class LoadBalancerServiceMock implements LoadBalancerService {
     }
 
     @Override
-    public LoadBalancer create(ApplicationId application, ClusterSpec.Id cluster, List<Real> reals) {
+    public LoadBalancer create(ApplicationId application, ClusterSpec.Id cluster, Set<Real> reals) {
         LoadBalancer loadBalancer = new LoadBalancer(
                 new LoadBalancerId(application, cluster),
                 HostName.from("lb-" + application.toShortString() + "-" + cluster.value()),
-                Collections.singletonList(4443),
+                Collections.singleton(4443),
                 reals,
                 false);
         loadBalancers.put(loadBalancer.id(), loadBalancer);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java
index ba29fcf2920..53f825f960b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializer.java
@@ -12,8 +12,8 @@ import com.yahoo.vespa.hosted.provision.lb.Real;
 
 import java.io.IOException;
 import java.io.UncheckedIOException;
-import java.util.ArrayList;
-import java.util.List;
+import java.util.LinkedHashSet;
+import java.util.Set;
 
 /**
  * Serializer for load balancers.
@@ -57,7 +57,7 @@ public class LoadBalancerSerializer {
     public static LoadBalancer fromJson(byte[] data) {
         Cursor object = SlimeUtils.jsonToSlime(data).get();
 
-        List<Real> reals = new ArrayList<>();
+        Set<Real> reals = new LinkedHashSet<>();
         object.field(realsField).traverse((ArrayTraverser) (i, realObject) -> {
             reals.add(new Real(HostName.from(realObject.field(hostnameField).asString()),
                                realObject.field(ipAddressField).asString(),
@@ -65,7 +65,7 @@ public class LoadBalancerSerializer {
 
         });
 
-        List<Integer> ports = new ArrayList<>();
+        Set<Integer> ports = new LinkedHashSet<>();
         object.field(portsField).traverse((ArrayTraverser) (i, port) -> ports.add((int) port.asLong()));
 
         return new LoadBalancer(LoadBalancerId.fromSerializedForm(object.field(idField).asString()),
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java
index 5fcbab64429..ce8475a3a07 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/LoadBalancerSerializerTest.java
@@ -1,6 +1,7 @@
 // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.persistence;
 
+import com.google.common.collect.ImmutableSet;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostName;
@@ -9,8 +10,6 @@ import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId;
 import com.yahoo.vespa.hosted.provision.lb.Real;
 import org.junit.Test;
 
-import java.util.Arrays;
-
 import static org.junit.Assert.assertEquals;
 
 /**
@@ -25,13 +24,13 @@ public class LoadBalancerSerializerTest {
                                                                                            "default"),
                                                                         ClusterSpec.Id.from("qrs")),
                                                      HostName.from("lb-host"),
-                                                     Arrays.asList(4080, 4443),
-                                                     Arrays.asList(new Real(HostName.from("real-1"),
-                                                                            "127.0.0.1",
-                                                                            4080),
-                                                                   new Real(HostName.from("real-2"),
-                                                                            "127.0.0.2",
-                                                                            4080)),
+                                                     ImmutableSet.of(4080, 4443),
+                                                     ImmutableSet.of(new Real(HostName.from("real-1"),
+                                                                              "127.0.0.1",
+                                                                              4080),
+                                                                     new Real(HostName.from("real-2"),
+                                                                              "127.0.0.2",
+                                                                              4080)),
                                                      false);
 
         LoadBalancer serialized = LoadBalancerSerializer.fromJson(LoadBalancerSerializer.toJson(loadBalancer));
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 e340abfa5c1..ed7b4292eb3 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
@@ -1,6 +1,7 @@
 // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.provisioning;
 
+import com.google.common.collect.Iterators;
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
@@ -60,13 +61,13 @@ public class LoadBalancerProvisionerTest {
         List<LoadBalancer> loadBalancers = loadBalancerProvisioner.get(app1);
         assertEquals(1, loadBalancers.size());
 
-        assertEquals(loadBalancers.get(0).id().application(), app1);
-        assertEquals(loadBalancers.get(0).id().cluster(), containerCluster1);
-        assertEquals(loadBalancers.get(0).ports(), Collections.singletonList(4443));
-        assertEquals(loadBalancers.get(0).reals().get(0).ipAddress(), "127.0.0.1");
-        assertEquals(loadBalancers.get(0).reals().get(0).port(), 4080);
-        assertEquals(loadBalancers.get(0).reals().get(1).ipAddress(), "127.0.0.2");
-        assertEquals(loadBalancers.get(0).reals().get(1).port(), 4080);
+        assertEquals(app1, loadBalancers.get(0).id().application());
+        assertEquals(containerCluster1, loadBalancers.get(0).id().cluster());
+        assertEquals(Collections.singleton(4443), loadBalancers.get(0).ports());
+        assertEquals("127.0.0.1", get(loadBalancers.get(0).reals(), 0).ipAddress());
+        assertEquals(4080, get(loadBalancers.get(0).reals(), 0).port());
+        assertEquals("127.0.0.2", get(loadBalancers.get(0).reals(), 1).ipAddress());
+        assertEquals(4080, get(loadBalancers.get(0).reals(), 1).port());
 
         // A container is failed
         List<Node> containers = tester.getNodes(app1).type(ClusterSpec.Type.container).asList();
@@ -81,8 +82,8 @@ public class LoadBalancerProvisionerTest {
         containers = tester.getNodes(app1).type(ClusterSpec.Type.container).asList();
         LoadBalancer loadBalancer = loadBalancerProvisioner.get(app1).get(0);
         assertEquals(2, loadBalancer.reals().size());
-        assertEquals(containers.get(0).hostname(), loadBalancer.reals().get(0).hostname().value());
-        assertEquals(containers.get(1).hostname(), loadBalancer.reals().get(1).hostname().value());
+        assertEquals(containers.get(0).hostname(), get(loadBalancer.reals(), 0).hostname().value());
+        assertEquals(containers.get(1).hostname(), get(loadBalancer.reals(), 1).hostname().value());
 
         // Add another container cluster
         ClusterSpec.Id containerCluster2 = ClusterSpec.Id.from("qrs2");
@@ -131,4 +132,8 @@ public class LoadBalancerProvisionerTest {
         return allNodes;
     }
 
+    private static <T> T get(Set<T> set, int position) {
+        return Iterators.get(set.iterator(), position, null);
+    }
+
 }