Log and forward duper model completion

7 files changed, 62 insertions, 57 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 9bfba36a2d3..ca4a1e7f9c5 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
@@ -37,7 +37,15 @@ public class DuperModel {
         listeners.add(listener);
     }
 
-    void setComplete() { this.isComplete = true; }
+    void setComplete() {
+        if (!isComplete) {
+            logger.log(LogLevel.INFO, "Bootstrap done - duper model is complete");
+            isComplete = true;
+
+            listeners.forEach(DuperModelListener::bootstrapComplete);
+        }
+    }
+
     public boolean isComplete() { return isComplete; }
 
     public int numberOfApplications() {
@@ -75,7 +83,7 @@ public class DuperModel {
         }
         logger.log(LogLevel.INFO, logPrefix + applicationInfo.getApplicationId());
 
-        Set<HostName> oldHostnames = hostnamesById.get(applicationInfo.getApplicationId());
+        Set<HostName> oldHostnames = hostnamesById.remove(applicationInfo.getApplicationId());
         if (oldHostnames != null) {
             oldHostnames.forEach(applicationsByHostname::remove);
         }
@@ -98,8 +106,7 @@ public class DuperModel {
         }
 
         ApplicationInfo application = applicationsById.remove(applicationId);
-
-        if (application != null || hostnames != null) {
+        if (application != null) {
             logger.log(LogLevel.INFO, "Removed application " + applicationId);
             listeners.forEach(listener -> listener.applicationRemoved(applicationId));
         }
diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ApplicationInstanceGenerator.java b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ApplicationInstanceGenerator.java
index db15c2789d5..412c47bee95 100644
--- a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ApplicationInstanceGenerator.java
+++ b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ApplicationInstanceGenerator.java
@@ -57,14 +57,6 @@ public class ApplicationInstanceGenerator {
         return makeApplicationInstanceLimitedToHosts(serviceStatusProvider, hostname -> true);
     }
 
-    public static ServiceId getServiceId(ApplicationInfo applicationInfo, ServiceInfo serviceInfo) {
-        return new ServiceId(
-                applicationInfo.getApplicationId(),
-                getClusterId(serviceInfo),
-                toServiceType(serviceInfo),
-                toConfigId(serviceInfo));
-    }
-
     public ApplicationInstanceReference toApplicationInstanceReference() {
         TenantId tenantId = new TenantId(applicationInfo.getApplicationId().tenant().toString());
         ApplicationInstanceId applicationInstanceId = toApplicationInstanceId(applicationInfo.getApplicationId(), zone);
@@ -107,25 +99,6 @@ public class ApplicationInstanceGenerator {
                 InstanceName.from(appNameParts[4]));
     }
 
-    private static ApplicationInstanceId toApplicationInstanceId(ApplicationId applicationId, Zone zone) {
-        if (applicationId.equals(configServerApplicationId)) {
-            // Removing this historical discrepancy would break orchestration during rollout.
-            // An alternative may be to use a feature flag and flip it between releases,
-            // once that's available.
-            return new ApplicationInstanceId(applicationId.application().value());
-        } else {
-            return new ApplicationInstanceId(String.format("%s:%s:%s:%s",
-                    applicationId.application().value(),
-                    zone.environment().value(),
-                    zone.region().value(),
-                    applicationId.instance().value()));
-        }
-    }
-
-    private static ClusterId getClusterId(ServiceInfo serviceInfo) {
-        return new ClusterId(serviceInfo.getProperty(CLUSTER_ID_PROPERTY_NAME).orElse(""));
-    }
-
     private ApplicationInstance makeApplicationInstanceLimitedToHosts(ServiceStatusProvider serviceStatusProvider,
                                                                       Predicate<HostName> includeHostPredicate) {
         Map<ServiceClusterKey, Set<ServiceInstance>> groupedServiceInstances = new HashMap<>();
@@ -187,6 +160,33 @@ public class ApplicationInstanceGenerator {
         return new ServiceInstance(configId, hostName, status);
     }
 
+    private static ApplicationInstanceId toApplicationInstanceId(ApplicationId applicationId, Zone zone) {
+        if (applicationId.equals(configServerApplicationId)) {
+            // Removing this historical discrepancy would break orchestration during rollout.
+            // An alternative may be to use a feature flag and flip it between releases,
+            // once that's available.
+            return new ApplicationInstanceId(applicationId.application().value());
+        } else {
+            return new ApplicationInstanceId(String.format("%s:%s:%s:%s",
+                    applicationId.application().value(),
+                    zone.environment().value(),
+                    zone.region().value(),
+                    applicationId.instance().value()));
+        }
+    }
+
+    public static ServiceId getServiceId(ApplicationInfo applicationInfo, ServiceInfo serviceInfo) {
+        return new ServiceId(
+                applicationInfo.getApplicationId(),
+                getClusterId(serviceInfo),
+                toServiceType(serviceInfo),
+                toConfigId(serviceInfo));
+    }
+
+    private static ClusterId getClusterId(ServiceInfo serviceInfo) {
+        return new ClusterId(serviceInfo.getProperty(CLUSTER_ID_PROPERTY_NAME).orElse(""));
+    }
+
     private static ServiceClusterKey toServiceClusterKey(ServiceInfo serviceInfo) {
         ClusterId clusterId = getClusterId(serviceInfo);
         ServiceType serviceType = toServiceType(serviceInfo);
diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ModelGenerator.java b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ModelGenerator.java
index cce14c12195..d9378da957e 100644
--- a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ModelGenerator.java
+++ b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ModelGenerator.java
@@ -20,6 +20,7 @@ import java.util.stream.Collectors;
  */
 public class ModelGenerator {
     public static final String CLUSTER_ID_PROPERTY_NAME = "clustername";
+
     private final Zone zone;
 
     public ModelGenerator(Zone zone) {
@@ -48,12 +49,17 @@ public class ModelGenerator {
                 .collect(Collectors.toSet());
     }
 
-    public ApplicationInstance toApplication(ApplicationInfo applicationInfo,
-                                             ServiceStatusProvider serviceStatusProvider) {
+    public ApplicationInstance toApplicationInstance(ApplicationInfo applicationInfo,
+                                                     ServiceStatusProvider serviceStatusProvider) {
         var generator = new ApplicationInstanceGenerator(applicationInfo, zone);
         return generator.makeApplicationInstance(serviceStatusProvider);
     }
 
+    /**
+     * Make an application instance that contains all services and clusters present on the host,
+     * but lacking other services and hosts. This is an optimization over
+     * {@link #toApplicationInstance(ApplicationInfo, ServiceStatusProvider)}.
+     */
     public ApplicationInstance toApplicationNarrowedToHost(ApplicationInfo applicationInfo,
                                                            HostName hostname,
                                                            ServiceStatusProvider serviceStatusProvider) {
diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelProvider.java b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelProvider.java
index 35c72ba03fb..45ee38ab560 100644
--- a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelProvider.java
+++ b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelProvider.java
@@ -45,36 +45,34 @@ public class ServiceModelProvider implements ServiceMonitor {
     @Override
     public ServiceModel getServiceModelSnapshot() {
         try (LatencyMeasurement measurement = metrics.startServiceModelSnapshotLatencyMeasurement()) {
-            return modelGenerator.toServiceModel(applicationInfos(), serviceStatusProvider);
+            return modelGenerator.toServiceModel(duperModelManager.getApplicationInfos(), serviceStatusProvider);
         }
     }
 
     @Override
     public Set<ApplicationInstanceReference> getAllApplicationInstanceReferences() {
-        return modelGenerator.toApplicationInstanceReferenceSet(applicationInfos());
+        return modelGenerator.toApplicationInstanceReferenceSet(duperModelManager.getApplicationInfos());
     }
 
     @Override
     public Optional<ApplicationInstance> getApplication(HostName hostname) {
-        Optional<ApplicationInfo> applicationInfo =
-                duperModelManager.getApplicationInfo(toConfigProvisionHostName(hostname));
+        Optional<ApplicationInfo> applicationInfo = getApplicationInfo(hostname);
         if (applicationInfo.isEmpty()) {
             return Optional.empty();
         }
 
-        return Optional.of(modelGenerator.toApplication(applicationInfo.get(), serviceStatusProvider));
+        return Optional.of(modelGenerator.toApplicationInstance(applicationInfo.get(), serviceStatusProvider));
     }
 
     @Override
     public Optional<ApplicationInstance> getApplication(ApplicationInstanceReference reference) {
         return getApplicationInfo(reference)
-                .map(applicationInfo -> modelGenerator.toApplication(applicationInfo, serviceStatusProvider));
+                .map(applicationInfo -> modelGenerator.toApplicationInstance(applicationInfo, serviceStatusProvider));
     }
 
     @Override
     public Optional<ApplicationInstance> getApplicationNarrowedTo(HostName hostname) {
-        Optional<ApplicationInfo> applicationInfo =
-                duperModelManager.getApplicationInfo(toConfigProvisionHostName(hostname));
+        Optional<ApplicationInfo> applicationInfo = getApplicationInfo(hostname);
         if (applicationInfo.isEmpty()) {
             return Optional.empty();
         }
@@ -93,12 +91,9 @@ public class ServiceModelProvider implements ServiceMonitor {
         return duperModelManager.getApplicationInfo(applicationId);
     }
 
-    private List<ApplicationInfo> applicationInfos() {
-        return duperModelManager.getApplicationInfos();
-    }
-
-    /** The duper model uses HostName from config.provision, which is more natural than applicationmodel. */
-    private com.yahoo.config.provision.HostName toConfigProvisionHostName(HostName hostname) {
-        return com.yahoo.config.provision.HostName.from(hostname.s());
+    private Optional<ApplicationInfo> getApplicationInfo(HostName hostname) {
+        // The duper model uses HostName from config.provision, which is more natural than applicationmodel.
+        var configProvisionHostname = com.yahoo.config.provision.HostName.from(hostname.s());
+        return duperModelManager.getApplicationInfo(configProvisionHostname);
     }
 }
diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java b/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java
index f3d02992cdc..deafaaace0e 100644
--- a/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java
+++ b/service-monitor/src/main/java/com/yahoo/vespa/service/monitor/ServiceMonitor.java
@@ -22,7 +22,8 @@ public interface ServiceMonitor {
     /**
      * Returns a ServiceModel which contains the current liveness status (up, down or unknown) of all instances
      * of all services of all clusters of all applications in a zone.
-     * Deprecated: Please use the more specific methods below.
+     *
+     * <p>Please use the more specific methods below to avoid the cost of this method.</p>
      */
     ServiceModel getServiceModelSnapshot();
 
@@ -39,7 +40,7 @@ public interface ServiceMonitor {
     }
 
     default Optional<ApplicationInstance> getApplicationNarrowedTo(HostName hostname) {
-        return Optional.ofNullable(getServiceModelSnapshot().getApplicationsByHostName().get(hostname));
+        return getApplication(hostname);
     }
 
     default Map<HostName, List<ServiceInstance>> getServicesByHostname() {
diff --git a/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java b/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java
index 67508f14e5a..3448ce46ff9 100644
--- a/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java
+++ b/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java
@@ -71,7 +71,6 @@ public class DuperModelManagerTest {
         verify(duperModel, times(0)).add(any());
         manager.infraApplicationActivated(id, proxyHostHosts);
         verify(duperModel, times(1)).add(any());
-        when(duperModel.contains(id)).thenReturn(true);
 
         verify(duperModel, times(0)).remove(any());
         manager.infraApplicationRemoved(id);
@@ -98,7 +97,6 @@ public class DuperModelManagerTest {
         List<HostName> hostnames1 = Stream.of("node11", "node12").map(HostName::from).collect(Collectors.toList());
         manager.infraApplicationActivated(firstId, hostnames1);
         verify(duperModel, times(1)).add(any());
-        when(duperModel.contains(firstId)).thenReturn(true);
 
         // Adding the second config server like application will be ignored
         List<HostName> hostnames2 = Stream.of("node21", "node22").map(HostName::from).collect(Collectors.toList());
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 0cc108bae9e..69b6d3d59f3 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
@@ -16,8 +16,6 @@ import java.util.Optional;
 import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -60,11 +58,11 @@ public class DuperModelTest {
     }
 
     @Test
-    public void test() {
+    public void testListeners() {
         assertEquals(0, duperModel.numberOfApplications());
 
         duperModel.add(application1);
-        assertTrue(duperModel.contains(id1));
+        assertEquals(Optional.of(application1), duperModel.getApplicationInfo(id1));
         assertEquals(Arrays.asList(application1), duperModel.getApplicationInfos());
         assertEquals(1, duperModel.numberOfApplications());
 
@@ -82,7 +80,7 @@ public class DuperModelTest {
         verifyNoMoreInteractions(listener1);
 
         duperModel.remove(id1);
-        assertFalse(duperModel.contains(id1));
+        assertEquals(Optional.empty(), duperModel.getApplicationInfo(id1));
         verify(listener1, times(1)).applicationRemoved(id1);
         verifyNoMoreInteractions(listener1);
         assertEquals(Arrays.asList(application2), duperModel.getApplicationInfos());