Merge pull request #23519 from vespa-engine/simplify-setup-of-ExecutionFactory

Make ExecutionFactory a child of SearchHandler for correct chains config [run-systemtest]

5 files changed, 66 insertions, 62 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java
index 4d45398de90..6a9d6b63623 100755
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java
@@ -70,8 +70,6 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeSet;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import static com.yahoo.vespa.model.container.component.chain.ProcessingHandler.PROCESSING_HANDLER_CLASS;
 
@@ -130,10 +128,6 @@ public abstract class ContainerCluster<CONTAINER extends Container>
 
     public static final BindingPattern VIP_HANDLER_BINDING = SystemBindingPattern.fromHttpPath("/status.html");
 
-    public static final Set<Path> SEARCH_AND_DOCPROC_BUNDLES = Stream.of(
-                    PlatformBundles.SEARCH_AND_DOCPROC_BUNDLE, "container-search-gui", "docprocs", "linguistics-components")
-            .map(PlatformBundles::absoluteBundlePath).collect(Collectors.toSet());
-
     private final String name;
 
     protected List<CONTAINER> containers = new ArrayList<>();
@@ -404,18 +398,6 @@ public abstract class ContainerCluster<CONTAINER extends Container>
         return Collections.unmodifiableCollection(allComponents);
     }
 
-    /*
-    Add all search/docproc/feed related platform bundles.
-    This is only required for configured containers as the platform bundle set is not allowed to change between config generations.
-    For standalone container platform bundles can be added on features enabled as an update of application package requires restart.
-    */
-    public void addAllPlatformBundles() {
-        ContainerDocumentApi.addVespaClientContainerBundle(this);
-        addSearchAndDocprocBundles();
-    }
-
-    public void addSearchAndDocprocBundles() { SEARCH_AND_DOCPROC_BUNDLES.forEach(this::addPlatformBundle); }
-
     private void recursivelyFindAllComponents(Collection<Component<?, ?>> allComponents, AbstractConfigProducer<?> current) {
         for (AbstractConfigProducer<?> child: current.getChildren().values()) {
             if (child instanceof Component)
@@ -476,9 +458,22 @@ public abstract class ContainerCluster<CONTAINER extends Container>
      * Adds the Vespa bundles that are necessary for all container types.
      */
     public void addCommonVespaBundles() {
-        PlatformBundles.commonVespaBundles().forEach(this::addPlatformBundle);
+        PlatformBundles.commonVespaBundles.forEach(this::addPlatformBundle);
     }
 
+    /*
+    Add all search/docproc/feed related platform bundles.
+    This is only required for application configured containers as the platform bundle set is not allowed to change
+    between config generations. For standalone container platform bundles can be added on features enabled as an
+    update of application package requires restart.
+    */
+    public void addAllPlatformBundles() {
+        ContainerDocumentApi.addVespaClientContainerBundle(this);
+        addSearchAndDocprocBundles();
+    }
+
+    public void addSearchAndDocprocBundles() { PlatformBundles.SEARCH_AND_DOCPROC_BUNDLES.forEach(this::addPlatformBundle); }
+
     /**
      * Adds a bundle present at a known location at the target container nodes.
      * Note that the set of platform bundles cannot change during the jdisc container's lifetime.
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/PlatformBundles.java b/config-model/src/main/java/com/yahoo/vespa/model/container/PlatformBundles.java
index 6a1e647e9be..f8691dcde53 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/PlatformBundles.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/PlatformBundles.java
@@ -6,12 +6,13 @@ import com.yahoo.vespa.defaults.Defaults;
 
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.Collections;
-import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
+ * NOTE: Stable ordering of bundles in config is handled by {@link ContainerCluster#addPlatformBundle(Path)}
+ *
  * @author gjoranv
  * @author Ulf Lilleengen
  */
@@ -31,11 +32,18 @@ public class PlatformBundles {
     public static final Path LIBRARY_PATH = Paths.get(Defaults.getDefaults().underVespaHome("lib/jars"));
     public static final String SEARCH_AND_DOCPROC_BUNDLE = BundleInstantiationSpecification.CONTAINER_SEARCH_AND_DOCPROC;
 
-    public static Set<Path> commonVespaBundles() {
-        var bundles = new LinkedHashSet<Path>();
-        commonVespaBundles.stream().map(PlatformBundles::absoluteBundlePath).forEach(bundles::add);
-        return Collections.unmodifiableSet(bundles);
-    }
+    // Bundles that must be loaded for all container types.
+    public static final Set<Path> commonVespaBundles = Stream.of(
+            "zkfacade",
+            "zookeeper-server"  // TODO: not necessary in metrics-proxy.
+    ).map(PlatformBundles::absoluteBundlePath).collect(Collectors.toSet());
+
+    public static final Set<Path> SEARCH_AND_DOCPROC_BUNDLES = Stream.of(
+            PlatformBundles.SEARCH_AND_DOCPROC_BUNDLE,
+            "container-search-gui",
+            "docprocs",
+            "linguistics-components"
+    ).map(PlatformBundles::absoluteBundlePath).collect(Collectors.toSet());
 
     public static Path absoluteBundlePath(String fileName) {
         return absoluteBundlePath(fileName, JarSuffix.JAR_WITH_DEPS);
@@ -50,12 +58,6 @@ public class PlatformBundles {
         return searchAndDocprocComponents.contains(className);
     }
 
-    // Bundles that must be loaded for all container types.
-    private static final List<String> commonVespaBundles = List.of(
-            "zkfacade",
-            "zookeeper-server"  // TODO: not necessary in metrics-proxy.
-    );
-
     // This is a hack to allow users to declare components from the search-and-docproc bundle without naming the bundle.
     private static final Set<String> searchAndDocprocComponents = Set.of(
             "com.yahoo.docproc.AbstractConcreteDocumentFactory",
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index 8f02476b2cc..423d82687a3 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -30,7 +30,6 @@ import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.Zone;
-import com.yahoo.container.bundle.BundleInstantiationSpecification;
 import com.yahoo.container.logging.FileConnectionLog;
 import com.yahoo.osgi.provider.model.ComponentModel;
 import com.yahoo.schema.OnnxModel;
@@ -63,13 +62,13 @@ import com.yahoo.vespa.model.container.PlatformBundles;
 import com.yahoo.vespa.model.container.SecretStore;
 import com.yahoo.vespa.model.container.component.AccessLogComponent;
 import com.yahoo.vespa.model.container.component.BindingPattern;
+import com.yahoo.vespa.model.container.component.Component;
 import com.yahoo.vespa.model.container.component.ConnectionLogComponent;
 import com.yahoo.vespa.model.container.component.FileStatusHandlerComponent;
 import com.yahoo.vespa.model.container.component.Handler;
 import com.yahoo.vespa.model.container.component.SimpleComponent;
 import com.yahoo.vespa.model.container.component.SystemBindingPattern;
 import com.yahoo.vespa.model.container.component.UserBindingPattern;
-import com.yahoo.vespa.model.container.component.chain.ProcessingHandler;
 import com.yahoo.vespa.model.container.docproc.ContainerDocproc;
 import com.yahoo.vespa.model.container.docproc.DocprocChains;
 import com.yahoo.vespa.model.container.http.AccessControl;
@@ -89,6 +88,7 @@ import com.yahoo.vespa.model.container.xml.embedder.EmbedderConfig;
 import com.yahoo.vespa.model.content.StorageGroup;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
+
 import java.net.URI;
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
@@ -886,16 +886,13 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
     }
 
     private void addSearchHandler(ApplicationContainerCluster cluster, Element searchElement) {
-        // Magic spell is needed to receive the chains config :-|
-        cluster.addComponent(new ProcessingHandler<>(
-                cluster.getSearch().getChains(),
-                BundleInstantiationSpecification.fromSearchAndDocproc("com.yahoo.search.searchchain.ExecutionFactory")));
-
-        cluster.addComponent(
-                new SearchHandler(
-                        cluster,
-                        serverBindings(searchElement, SearchHandler.DEFAULT_BINDING),
-                        ContainerThreadpool.UserOptions.fromXml(searchElement).orElse(null)));
+        SearchHandler searchHandler = new SearchHandler(cluster,
+                                                        serverBindings(searchElement, SearchHandler.DEFAULT_BINDING),
+                                                        ContainerThreadpool.UserOptions.fromXml(searchElement).orElse(null));
+        cluster.addComponent(searchHandler);
+
+        // Add as child to SearchHandler to get the correct chains config.
+        searchHandler.addComponent(Component.fromClassAndBundle(SearchHandler.EXECUTION_FACTORY_CLASS, PlatformBundles.SEARCH_AND_DOCPROC_BUNDLE));
     }
 
     private void addGUIHandler(ApplicationContainerCluster cluster) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/SearchHandler.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/SearchHandler.java
index 7e1b3be9240..54cd061d2c5 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/SearchHandler.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/SearchHandler.java
@@ -5,7 +5,6 @@ import com.yahoo.container.bundle.BundleInstantiationSpecification;
 import com.yahoo.container.handler.threadpool.ContainerThreadpoolConfig;
 import com.yahoo.vespa.model.container.ApplicationContainerCluster;
 import com.yahoo.vespa.model.container.ContainerThreadpool;
-import com.yahoo.vespa.model.container.PlatformBundles;
 import com.yahoo.vespa.model.container.component.BindingPattern;
 import com.yahoo.vespa.model.container.component.SystemBindingPattern;
 import com.yahoo.vespa.model.container.component.chain.ProcessingHandler;
@@ -23,6 +22,8 @@ import static com.yahoo.container.bundle.BundleInstantiationSpecification.fromSe
 class SearchHandler extends ProcessingHandler<SearchChains> {
 
     static final String HANDLER_CLASS = com.yahoo.search.handler.SearchHandler.class.getName();
+    static final String EXECUTION_FACTORY_CLASS = com.yahoo.search.searchchain.ExecutionFactory.class.getName();
+
     static final BundleInstantiationSpecification HANDLER_SPEC = fromSearchAndDocproc(HANDLER_CLASS);
     static final BindingPattern DEFAULT_BINDING = SystemBindingPattern.fromHttpPath("/search/*");
 
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/SearchBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/SearchBuilderTest.java
index c41373b9f85..e6e580b9baa 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/SearchBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/SearchBuilderTest.java
@@ -9,6 +9,7 @@ import com.yahoo.container.jdisc.JdiscBindingsConfig;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.container.ApplicationContainerCluster;
 import com.yahoo.vespa.model.container.ContainerCluster;
+import com.yahoo.vespa.model.container.component.Component;
 import com.yahoo.vespa.model.container.component.Handler;
 import com.yahoo.vespa.model.container.search.GUIHandler;
 import com.yahoo.vespa.model.test.utils.ApplicationPackageUtils;
@@ -39,13 +40,7 @@ public class SearchBuilderTest extends ContainerModelBuilderTestBase {
 
     @Test
     public void gui_search_handler_is_always_included_when_search_is_specified() {
-        Element clusterElem = DomBuilderTest.parse(
-                "<container id='default' version='1.0'>",
-                "  <search />",
-                nodesXml,
-                "</container>");
-
-        createModel(root, clusterElem);
+        createBasicSearchModel();
 
         String discBindingsConfig = root.getConfig(JdiscBindingsConfig.class, "default").toString();
         assertTrue(discBindingsConfig.contains(GUIHandler.BINDING_PATH));
@@ -190,7 +185,7 @@ public class SearchBuilderTest extends ContainerModelBuilderTestBase {
 
         VespaModel model = getVespaModelWithMusic(hosts, services);
 
-        ContainerCluster cluster = model.getContainerClusters().get("container");
+        ApplicationContainerCluster cluster = model.getContainerClusters().get("container");
         assertFalse(cluster.getSearchChains().localProviders().isEmpty());
     }
 
@@ -216,19 +211,13 @@ public class SearchBuilderTest extends ContainerModelBuilderTestBase {
 
         VespaModel model = getVespaModelWithMusic(hosts, services);
 
-        ContainerCluster cluster = model.getContainerClusters().get("container");
+        ApplicationContainerCluster cluster = model.getContainerClusters().get("container");
         assertFalse(cluster.getSearchChains().localProviders().isEmpty());
     }
 
     @Test
     public void search_handler_has_dedicated_threadpool() {
-        Element clusterElem = DomBuilderTest.parse(
-                "<container id='default' version='1.0'>",
-                "  <search />",
-                nodesXml,
-                "</container>");
-
-        createModel(root, clusterElem);
+        createBasicSearchModel();
 
         Handler searchHandler = getHandler("default", SearchHandler.HANDLER_CLASS);
         assertTrue(searchHandler.getInjectedComponentIds().contains("threadpool@search-handler"));
@@ -261,10 +250,30 @@ public class SearchBuilderTest extends ContainerModelBuilderTestBase {
         assertEquals(10, config.queueSize());
     }
 
+    @Test
+    public void ExecutionFactory_gets_same_chains_config_as_SearchHandler() {
+        createBasicSearchModel();
+        Component<?,?> executionFactory = ((SearchHandler) getComponent("default",SearchHandler.HANDLER_CLASS))
+                .getChildren().get(SearchHandler.EXECUTION_FACTORY_CLASS);
+
+        ChainsConfig executionFactoryChainsConfig = root.getConfig(ChainsConfig.class, executionFactory.getConfigId());
+        assertEquals(chainsConfig(), executionFactoryChainsConfig);
+    }
+
     private VespaModel getVespaModelWithMusic(String hosts, String services) {
         return new VespaModelCreatorWithMockPkg(hosts, services, ApplicationPackageUtils.generateSchemas("music")).create();
     }
 
+    private void createBasicSearchModel() {
+        Element clusterElem = DomBuilderTest.parse(
+                "<container id='default' version='1.0'>",
+                "  <search />",
+                nodesXml,
+                "</container>");
+
+        createModel(root, clusterElem);
+    }
+
     private String hostsXml() {
         return "" +
                     "<hosts>  " +