Merge pull request #27457 from vespa-engine/hmusum/reduce-use-of-ApplicationInstanceReference

Use ApplicationId instead of ApplicationInstanceReference where possible

9 files changed, 79 insertions, 42 deletions

diff --git a/orchestrator/pom.xml b/orchestrator/pom.xml
index bd9c7ed977a..6ae24d2782e 100644
--- a/orchestrator/pom.xml
+++ b/orchestrator/pom.xml
@@ -138,6 +138,11 @@
       <scope>test</scope>
     </dependency>
     <dependency>
+      <groupId>org.junit.jupiter</groupId>
+      <artifactId>junit-jupiter-params</artifactId>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
       <groupId>org.junit.vintage</groupId>
       <artifactId>junit-vintage-engine</artifactId>
       <scope>test</scope>
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
index 49fab7522ba..99c2fa11825 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java
@@ -380,7 +380,7 @@ public class OrchestratorImpl implements Orchestrator {
         OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock);
         ApplicationInstanceReference reference = OrchestratorUtil.toApplicationInstanceReference(appId, serviceMonitor);
 
-        ApplicationInstance application = serviceMonitor.getApplication(reference)
+        ApplicationInstance application = serviceMonitor.getApplication(appId)
                 .orElseThrow(ApplicationIdNotFoundException::new);
 
         try (ApplicationLock lock = statusService.lockApplication(context, reference)) {
@@ -412,7 +412,7 @@ public class OrchestratorImpl implements Orchestrator {
     @Override
     public boolean isQuiescent(ApplicationId id) {
         try {
-            ApplicationInstance application = serviceMonitor.getApplication(OrchestratorUtil.toApplicationInstanceReference(id, serviceMonitor))
+            ApplicationInstance application = serviceMonitor.getApplication(id)
                                                             .orElseThrow(ApplicationIdNotFoundException::new);
 
             List<ServiceCluster> contentClusters = application.serviceClusters().stream()
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java
index 3093a2a9828..c232cd95a2f 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java
@@ -14,13 +14,10 @@ import com.yahoo.vespa.applicationmodel.ServiceInstance;
 import com.yahoo.vespa.applicationmodel.TenantId;
 import com.yahoo.vespa.service.monitor.ServiceMonitor;
 
-import java.util.Collection;
 import java.util.List;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import static java.util.stream.Collectors.toSet;
 
@@ -91,7 +88,6 @@ public class OrchestratorUtil {
     }
 
     public static ApplicationId toApplicationId(ApplicationInstanceReference appRef) {
-
         String appNameStr = appRef.asString();
         String[] appNameParts = appNameStr.split(":");
 
@@ -99,8 +95,8 @@ public class OrchestratorUtil {
         // Assume here that first two are tenant and application name.
         if (appNameParts.length == 2) {
             return ApplicationId.from(TenantName.from(appNameParts[0]),
-                    ApplicationName.from(appNameParts[1]),
-                    InstanceName.defaultName());
+                                      ApplicationName.from(appNameParts[1]),
+                                      InstanceName.defaultName());
         }
 
         // Other normal application should have 5 parts.
@@ -109,8 +105,8 @@ public class OrchestratorUtil {
         }
 
         return ApplicationId.from(TenantName.from(appNameParts[0]),
-                ApplicationName.from(appNameParts[1]),
-                InstanceName.from(appNameParts[4]));
+                                  ApplicationName.from(appNameParts[1]),
+                                  InstanceName.from(appNameParts[4]));
     }
 
 }
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandler.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandler.java
index 0964a3f7dd5..3b3e3ce7eea 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandler.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandler.java
@@ -7,6 +7,7 @@ import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
 import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
 import com.yahoo.component.annotation.Inject;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.Zone;
 import com.yahoo.container.jdisc.ThreadedHttpRequestHandler;
 import com.yahoo.jrt.slobrok.api.Mirror;
 import com.yahoo.restapi.RestApi;
@@ -27,6 +28,7 @@ import com.yahoo.vespa.orchestrator.status.HostInfos;
 import com.yahoo.vespa.orchestrator.status.StatusService;
 import com.yahoo.vespa.service.manager.MonitorManager;
 import com.yahoo.vespa.service.manager.UnionMonitorManager;
+import com.yahoo.vespa.service.model.ApplicationInstanceGenerator;
 import com.yahoo.vespa.service.monitor.ServiceMonitor;
 import com.yahoo.vespa.service.monitor.SlobrokApi;
 
@@ -54,18 +56,21 @@ public class InstanceRequestHandler extends RestApiRequestHandler<InstanceReques
     private final SlobrokApi slobrokApi;
     private final MonitorManager rootManager;
     private final ServiceMonitor serviceMonitor;
+    private final Zone zone;
 
     @Inject
     public InstanceRequestHandler(ThreadedHttpRequestHandler.Context context,
                                   ServiceMonitor serviceMonitor,
                                   StatusService statusService,
                                   SlobrokApi slobrokApi,
-                                  UnionMonitorManager rootManager) {
+                                  UnionMonitorManager rootManager,
+                                  Zone zone) {
         super(context, InstanceRequestHandler::createRestApiDefinition);
         this.statusService = statusService;
         this.slobrokApi = slobrokApi;
         this.rootManager = rootManager;
         this.serviceMonitor = serviceMonitor;
+        this.zone = zone;
     }
 
     private static RestApi createRestApiDefinition(InstanceRequestHandler self) {
@@ -89,16 +94,19 @@ public class InstanceRequestHandler extends RestApiRequestHandler<InstanceReques
                 .build();
     }
 
-    private List<ApplicationInstanceReference> getAllInstances(RestApi.RequestContext context) {
-        return serviceMonitor.getAllApplicationInstanceReferences().stream().sorted().toList();
+    private List<ApplicationId> getAllInstances(RestApi.RequestContext context) {
+        return serviceMonitor.getAllApplicationInstanceReferences().stream()
+                .map(OrchestratorUtil::toApplicationId)
+                .sorted()
+                .toList();
     }
 
     private InstanceStatusResponse getInstance(RestApi.RequestContext context) {
         String instanceIdString = context.pathParameters().getStringOrThrow("instanceId");
-        ApplicationInstanceReference instanceId = parseInstanceId(instanceIdString);
+        ApplicationInstanceReference instanceId = getApplicationInstanceReference(instanceIdString);
 
         ApplicationInstance applicationInstance
-                = serviceMonitor.getApplication(instanceId)
+                = serviceMonitor.getApplication(OrchestratorUtil.toApplicationId(instanceId))
                 .orElseThrow(RestApiException.NotFound::new);
 
         HostInfos hostInfos = statusService.getHostInfosByApplicationResolver().apply(applicationInstance.reference());
@@ -113,6 +121,19 @@ public class InstanceRequestHandler extends RestApiRequestHandler<InstanceReques
         return InstanceStatusResponse.create(applicationInstance, hostStatusMap);
     }
 
+    // Gets ApplicationInstanceReference when string might be an ApplicationId (tenant:application:instance) or
+    // an ApplicationInstanceReference (tenant:application:instance:environment:region:instance).
+    // TODO: Accept only strings on the form tenant:application:instance when all users have been
+    // updated and return ApplicationId instead of ApplicationInstanceReference.
+    private ApplicationInstanceReference getApplicationInstanceReference(String instanceIdString) {
+        try {
+            ApplicationId applicationId = ApplicationId.fromSerializedForm(instanceIdString);
+            return ApplicationInstanceGenerator.toApplicationInstanceReference(applicationId, zone);
+        } catch (IllegalArgumentException e) {
+            return parseInstanceId(instanceIdString);
+        }
+    }
+
     private WireHostInfo hostInfoToWire(HostInfo hostInfo) {
         String hostStatusString = hostInfo.status().asString();
         String suspendedSinceUtcOrNull = hostInfo.suspendedSince().map(Instant::toString).orElse(null);
@@ -122,7 +143,7 @@ public class InstanceRequestHandler extends RestApiRequestHandler<InstanceReques
     private List<SlobrokEntryResponse> getSlobrokEntries(RestApi.RequestContext context) {
         String instanceId = context.pathParameters().getStringOrThrow("instanceId");
         String pattern = context.queryParameters().getString("pattern").orElse(null);
-        ApplicationInstanceReference reference = parseInstanceId(instanceId);
+        ApplicationInstanceReference reference = getApplicationInstanceReference(instanceId);
         ApplicationId applicationId = OrchestratorUtil.toApplicationId(reference);
 
         if (pattern == null) {
@@ -140,7 +161,7 @@ public class InstanceRequestHandler extends RestApiRequestHandler<InstanceReques
         String clusterIdString = context.queryParameters().getStringOrThrow("clusterId");
         String serviceTypeString = context.queryParameters().getStringOrThrow("serviceType");
         String configIdString = context.queryParameters().getStringOrThrow("configId");
-        ApplicationInstanceReference reference = parseInstanceId(instanceId);
+        ApplicationInstanceReference reference = getApplicationInstanceReference(instanceId);
         ApplicationId applicationId = OrchestratorUtil.toApplicationId(reference);
 
         ClusterId clusterId = new ClusterId(clusterIdString);
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/DummyServiceMonitor.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/DummyServiceMonitor.java
index 853dc17a3fc..3c272204185 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/DummyServiceMonitor.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/DummyServiceMonitor.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.orchestrator;
 
 
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.vespa.applicationmodel.ApplicationInstance;
 import com.yahoo.vespa.applicationmodel.ApplicationInstanceId;
 import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference;
@@ -25,6 +26,8 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static com.yahoo.vespa.orchestrator.OrchestratorUtil.toApplicationInstanceReference;
+
 /**
  * A hardcoded set of applications with one storage cluster with two nodes each.
  *
@@ -141,6 +144,19 @@ public class DummyServiceMonitor implements ServiceMonitor, AntiServiceMonitor {
     }
 
     @Override
+    public Optional<ApplicationInstance> getApplication(ApplicationId applicationId) {
+        return apps.stream()
+                .filter(instance -> {
+                    try {
+                        return instance.reference().equals(toApplicationInstanceReference(applicationId, this));
+                    } catch (ApplicationIdNotFoundException e) {
+                        throw new RuntimeException(e);
+                    }
+                })
+                .findFirst();
+    }
+
+    @Override
     public Optional<ApplicationInstance> getApplication(HostName hostname) {
         for (ApplicationInstance app : apps) {
             for (ServiceCluster cluster : app.serviceClusters()) {
@@ -153,14 +169,6 @@ public class DummyServiceMonitor implements ServiceMonitor, AntiServiceMonitor {
     }
 
     @Override
-    public Optional<ApplicationInstance> getApplication(ApplicationInstanceReference reference) {
-        for (ApplicationInstance app : apps) {
-            if (app.reference().equals(reference)) return Optional.of(app);
-        }
-        return Optional.empty();
-    }
-
-    @Override
     public CriticalRegion disallowDuperModelLockAcquisition(String regionDescription) {
         return () -> {};
     }
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
index 70a8381c9ac..d27e0bae88a 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
@@ -413,7 +413,7 @@ public class OrchestratorImplTest {
         HostName hostName = new HostName("my.host");
         HostName ccHost = new HostName("cc.host");
         TenantId tenantId = new TenantId("tenant");
-        ApplicationInstanceId applicationInstanceId = new ApplicationInstanceId("app:env:region:instance");
+        ApplicationInstanceId applicationInstanceId = new ApplicationInstanceId("app:prod:default:instance");
         ApplicationInstanceReference reference = new ApplicationInstanceReference(tenantId, applicationInstanceId);
         ApplicationId id = ApplicationId.from("tenant", "app", "instance");
 
@@ -442,7 +442,7 @@ public class OrchestratorImplTest {
                                                                                              ccHost,
                                                                                              ServiceStatus.UP)))));
 
-        ServiceMonitor serviceMonitor = () -> new ServiceModel(Map.of(reference, applicationInstance));
+        ServiceMonitor serviceMonitor = () -> new ServiceModel(Map.of(reference, applicationInstance), zone);
 
         ClusterControllerClientFactory clusterControllerClientFactory = mock(ClusterControllerClientFactory.class);
         ClusterControllerClient fooClient = mock(ClusterControllerClient.class);
@@ -508,7 +508,7 @@ public class OrchestratorImplTest {
                                                hostName,
                                                ServiceStatus.NOT_CHECKED)))));
 
-        ServiceMonitor serviceMonitor = () -> new ServiceModel(Map.of(reference, applicationInstance));
+        ServiceMonitor serviceMonitor = () -> new ServiceModel(Map.of(reference, applicationInstance), zone);
 
         orchestrator = new OrchestratorImpl(new HostedVespaPolicy(new HostedVespaClusterPolicy(flagSource, zone),
                                                                   clusterControllerClientFactory,
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
index f2e2972ae9f..d93a8353fa3 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
@@ -74,7 +74,7 @@ class ModelTestUtils {
     private final Map<ApplicationInstanceReference, ApplicationInstance> applications = new HashMap<>();
     private final ClusterControllerClientFactory clusterControllerClientFactory = new ClusterControllerClientFactoryMock();
     private final Map<HostName, HostStatus> hostStatusMap = new HashMap<>();
-    private final ServiceMonitor serviceMonitor = () -> new ServiceModel(applications);
+    private final ServiceMonitor serviceMonitor = () -> new ServiceModel(applications, Zone.defaultZone());
     private final StatusService statusService = new ZkStatusService(
             new MockCurator(),
             mock(Metric.class),
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostRequestHandlerTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostRequestHandlerTest.java
index b9dab4b3aeb..f3f2bb18400 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostRequestHandlerTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostRequestHandlerTest.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.orchestrator.resources;
 
 import com.yahoo.concurrent.UncheckedTimeoutException;
+import com.yahoo.config.provision.Zone;
 import com.yahoo.container.jdisc.HttpRequestBuilder;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.jdisc.Metric;
@@ -91,7 +92,7 @@ class HostRequestHandlerTest {
     }
 
     private static final ServiceMonitor alwaysEmptyServiceMonitor = new ServiceMonitor() {
-        private final ServiceModel emptyServiceModel = new ServiceModel(Map.of());
+        private final ServiceModel emptyServiceModel = new ServiceModel(Map.of(), Zone.defaultZone());
 
         @Override
         public ServiceModel getServiceModelSnapshot() {
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandlerTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandlerTest.java
index a72afe8ba46..a120b2c2500 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandlerTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/InstanceRequestHandlerTest.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.orchestrator.resources;
 
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.Zone;
 import com.yahoo.container.jdisc.HttpRequest;
 import com.yahoo.container.jdisc.HttpRequestBuilder;
 import com.yahoo.container.jdisc.HttpResponse;
@@ -17,6 +18,9 @@ import com.yahoo.vespa.orchestrator.restapi.wire.SlobrokEntryResponse;
 import com.yahoo.vespa.service.manager.UnionMonitorManager;
 import com.yahoo.vespa.service.monitor.SlobrokApi;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
 import java.util.Arrays;
 import java.util.List;
 
@@ -31,18 +35,20 @@ import static org.mockito.Mockito.when;
  */
 class InstanceRequestHandlerTest {
 
-    private static final String APPLICATION_INSTANCE_REFERENCE = "tenant:app:prod:us-west-1:instance";
+    private static final String APPLICATION_INSTANCE_REFERENCE_LONG = "tenant:app:prod:us-west-1:instance";
+    private static final String APPLICATION_INSTANCE_REFERENCE = "tenant:app:instance";
     private static final ApplicationId APPLICATION_ID = ApplicationId.from(
             "tenant", "app", "instance");
     private static final List<Mirror.Entry> ENTRIES = Arrays.asList(
             new Mirror.Entry("name1", "tcp/spec:1"),
             new Mirror.Entry("name2", "tcp/spec:2"));
     private static final ClusterId CLUSTER_ID = new ClusterId("cluster-id");
+    private static final Zone zone = Zone.defaultZone();
 
     private final SlobrokApi slobrokApi = mock(SlobrokApi.class);
     private final UnionMonitorManager rootManager = mock(UnionMonitorManager.class);
     private final RestApiTestDriver testDriver =
-            RestApiTestDriver.newBuilder(ctx -> new InstanceRequestHandler(ctx, null, null, slobrokApi, rootManager))
+            RestApiTestDriver.newBuilder(ctx -> new InstanceRequestHandler(ctx, null, null, slobrokApi, rootManager, zone))
                     .build();
 
     @Test
@@ -55,16 +61,16 @@ class InstanceRequestHandlerTest {
         testGetSlobrokEntriesWith(null, InstanceRequestHandler.DEFAULT_SLOBROK_PATTERN);
     }
 
-    @Test
-    void testGetServiceStatusInfo() {
+    @ParameterizedTest
+    @ValueSource(strings = { APPLICATION_INSTANCE_REFERENCE, APPLICATION_INSTANCE_REFERENCE_LONG })
+    void testGetServiceStatusInfo(String reference) {
         ServiceType serviceType = new ServiceType("serviceType");
         ConfigId configId = new ConfigId("configId");
         ServiceStatus serviceStatus = ServiceStatus.UP;
         when(rootManager.getStatus(APPLICATION_ID, CLUSTER_ID, serviceType, configId))
                 .thenReturn(new ServiceStatusInfo(serviceStatus));
 
-
-        String uriPath = String.format("/orchestrator/v1/instances/%s/serviceStatusInfo", APPLICATION_INSTANCE_REFERENCE);
+        String uriPath = String.format("/orchestrator/v1/instances/%s/serviceStatusInfo", reference);
         HttpRequest request = HttpRequestBuilder.create(GET, uriPath)
                 .withQueryParameter("clusterId", CLUSTER_ID.s())
                 .withQueryParameter("serviceType", serviceType.s())
@@ -79,9 +85,10 @@ class InstanceRequestHandlerTest {
         assertEquals(serviceStatus, actualServiceStatus);
     }
 
-    @Test
-    void testBadRequest() {
-        String uriPath = String.format("/orchestrator/v1/instances/%s/serviceStatusInfo", APPLICATION_INSTANCE_REFERENCE);
+    @ParameterizedTest
+    @ValueSource(strings = { APPLICATION_INSTANCE_REFERENCE, APPLICATION_INSTANCE_REFERENCE_LONG })
+    void testBadRequest(String reference) {
+        String uriPath = String.format("/orchestrator/v1/instances/%s/serviceStatusInfo", reference);
         HttpRequest request = HttpRequestBuilder.create(GET, uriPath)
                 .withQueryParameter("clusterId", CLUSTER_ID.s())
                 .build();
@@ -89,12 +96,11 @@ class InstanceRequestHandlerTest {
         assertEquals(400, response.getStatus());
     }
 
-    private void testGetSlobrokEntriesWith(String pattern, String expectedLookupPattern)
-            throws Exception{
+    private void testGetSlobrokEntriesWith(String pattern, String expectedLookupPattern) throws Exception {
         when(slobrokApi.lookup(APPLICATION_ID, expectedLookupPattern))
                 .thenReturn(ENTRIES);
 
-        String uriPath = String.format("/orchestrator/v1/instances/%s/slobrok", APPLICATION_INSTANCE_REFERENCE);
+        String uriPath = String.format("/orchestrator/v1/instances/%s/slobrok", APPLICATION_INSTANCE_REFERENCE_LONG);
         var builder = HttpRequestBuilder.create(GET, uriPath);
         if (pattern != null) {
             builder.withQueryParameter("pattern", pattern);