Also remove hostnames from hostnamesById when removing hosts via application activate

I have seen a few "Host not found" messages in CD the last 2 days, and found
this bug in how the duper model indices are maintained on application
activation.

2 files changed, 75 insertions, 20 deletions

diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModel.java b/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModel.java
index 77ccc8b79d0..75a309ab202 100644
--- a/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModel.java
+++ b/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModel.java
@@ -27,7 +27,7 @@ public class DuperModel {
 
     private final Map<ApplicationId, ApplicationInfo> applicationsById = new HashMap<>();
     private final Map<HostName, ApplicationId> idsByHostname = new HashMap<>();
-    private final Map<ApplicationId, Set<HostName>> hostnamesById = new HashMap<>();
+    private final Map<ApplicationId, HashSet<HostName>> hostnamesById = new HashMap<>();
 
     private final List<DuperModelListener> listeners = new ArrayList<>();
     private boolean isComplete = false;
@@ -75,6 +75,16 @@ public class DuperModel {
         return List.copyOf(applicationsById.values());
     }
 
+    /** Note: Returns an empty set for unknown application. */
+    public Set<HostName> getHostnames(ApplicationId applicationId) {
+        HashSet<HostName> set = hostnamesById.get(applicationId);
+        return set == null ? Set.of() : Set.copyOf(set);
+    }
+
+    public Optional<ApplicationId> getApplicationId(HostName hostname) {
+        return Optional.ofNullable(idsByHostname.get(hostname));
+    }
+
     public void add(ApplicationInfo applicationInfo) {
         ApplicationId id = applicationInfo.getApplicationId();
         ApplicationInfo oldApplicationInfo = applicationsById.put(id, applicationInfo);
@@ -87,15 +97,35 @@ public class DuperModel {
         }
         logger.log(LogLevel.INFO, logPrefix + id.toFullString());
 
-        Set<HostName> hostnames = hostnamesById.computeIfAbsent(id, k -> new HashSet<>());
-        Set<HostName> removedHosts = new HashSet<>(hostnames);
+        updateHostnameVsIdMaps(applicationInfo, id);
+
+        listeners.forEach(listener -> listener.applicationActivated(applicationInfo));
+    }
+
+    public void remove(ApplicationId applicationId) {
+        Set<HostName> hostnames = hostnamesById.remove(applicationId);
+        if (hostnames != null) {
+            hostnames.forEach(idsByHostname::remove);
+        }
+
+        ApplicationInfo application = applicationsById.remove(applicationId);
+        if (application != null) {
+            logger.log(LogLevel.INFO, "Removed application " + applicationId.toFullString());
+            listeners.forEach(listener -> listener.applicationRemoved(applicationId));
+        }
+    }
+
+    /** Update hostnamesById and idsByHostname based on a new applicationInfo. */
+    private void updateHostnameVsIdMaps(ApplicationInfo applicationInfo, ApplicationId id) {
+        Set<HostName> removedHosts = new HashSet<>(hostnamesById.computeIfAbsent(id, k -> new HashSet<>()));
 
         applicationInfo.getModel().getHosts().stream()
                 .map(HostInfo::getHostname)
                 .map(HostName::from)
                 .forEach(hostname -> {
                     if (!removedHosts.remove(hostname)) {
-                        hostnames.add(hostname);
+                        // hostname has been added
+                        hostnamesById.get(id).add(hostname);
                         ApplicationId previousId = idsByHostname.put(hostname, id);
 
                         if (previousId != null && !previousId.equals(id)) {
@@ -113,21 +143,10 @@ public class DuperModel {
                     }
                 });
 
-        removedHosts.forEach(idsByHostname::remove);
-
-        listeners.forEach(listener -> listener.applicationActivated(applicationInfo));
-    }
-
-    public void remove(ApplicationId applicationId) {
-        Set<HostName> hostnames = hostnamesById.remove(applicationId);
-        if (hostnames != null) {
-            hostnames.forEach(idsByHostname::remove);
-        }
-
-        ApplicationInfo application = applicationsById.remove(applicationId);
-        if (application != null) {
-            logger.log(LogLevel.INFO, "Removed application " + applicationId.toFullString());
-            listeners.forEach(listener -> listener.applicationRemoved(applicationId));
-        }
+        removedHosts.forEach(hostname -> {
+            // hostname has been removed
+            idsByHostname.remove(hostname);
+            hostnamesById.get(id).remove(hostname);
+        });
     }
 }
diff --git a/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelTest.java b/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelTest.java
index 69b6d3d59f3..2e38283a091 100644
--- a/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelTest.java
+++ b/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelTest.java
@@ -13,9 +13,13 @@ import org.junit.Test;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -115,4 +119,36 @@ public class DuperModelTest {
         assertEquals(Optional.of(application2), duperModel.getApplicationInfo(hostname1_1));
         assertEquals(Optional.empty(), duperModel.getApplicationInfo(hostname2_1));
     }
+
+    @Test
+    public void hostIndicesForOneApplication() {
+        assertEquals(0, duperModel.numberOfApplications());
+        assertEquals(0, duperModel.numberOfHosts());
+        assertEquals(Set.of(), duperModel.getHostnames(id1));
+
+        addAndVerifyApplication1("host1");
+        addAndVerifyApplication1("host1", "host2");
+        addAndVerifyApplication1("host2", "host3");
+        assertEquals(Optional.empty(), duperModel.getApplicationId(HostName.from("host1")));
+
+        duperModel.remove(id1);
+        assertEquals(0, duperModel.numberOfApplications());
+        assertEquals(0, duperModel.numberOfHosts());
+        assertEquals(Set.of(), duperModel.getHostnames(id1));
+    }
+
+    private void addAndVerifyApplication1(String... hostnameStrings) {
+        HostName[] hostnameArray = Stream.of(hostnameStrings).map(HostName::from).toArray(HostName[]::new);
+        setUpApplication(id1, application1, hostnameArray);
+        duperModel.add(application1);
+
+        assertEquals(1, duperModel.numberOfApplications());
+        Optional<ApplicationInfo> applicationInfo = duperModel.getApplicationInfo(id1);
+        assertTrue(applicationInfo.isPresent());
+        assertSame(application1, applicationInfo.get());
+
+        assertEquals(hostnameArray.length, duperModel.numberOfHosts());
+        assertEquals(Set.of(hostnameArray), duperModel.getHostnames(id1));
+        Stream.of(hostnameArray).forEach(hostname -> assertEquals(Optional.of(id1), duperModel.getApplicationId(hostname)));
+    }
 }
\ No newline at end of file