Merge pull request #23971 from vespa-engine/simplify-application-bundle-installing

Always return the set of bundles to uninstall upon completeGen [run-systemtest]

4 files changed, 36 insertions, 45 deletions

diff --git a/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java b/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java
index c1cf18582d5..88e66c45ca3 100644
--- a/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java
+++ b/container-core/src/main/java/com/yahoo/container/core/config/ApplicationBundleLoader.java
@@ -7,7 +7,6 @@ import com.yahoo.osgi.Osgi;
 import org.osgi.framework.Bundle;
 
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
@@ -50,51 +49,49 @@ public class ApplicationBundleLoader {
     }
 
     /**
-     * Installs the given set of bundles and returns the set of bundles that is no longer used
-     * by the application, and should therefore be scheduled for uninstall.
-     *
-     * TODO: return void, and instead return the bundles to remove from completeGeneration()
+     * Installs the given set of bundles and updates state for which bundles and file references
+     * that are active or should be uninstalled in case of success or failure.
      */
-    public synchronized Set<Bundle> useBundles(List<FileReference> newFileReferences) {
+    public synchronized void useBundles(List<FileReference> newFileReferences) {
         if (! readyForNewBundles)
             throw new IllegalStateException("Bundles must be committed or reverted before using new bundles.");
 
-        obsoleteBundles = removeObsoleteBundles(newFileReferences);
-        Set<Bundle> bundlesToUninstall = new LinkedHashSet<>(obsoleteBundles.values());
-        log.info("Bundles to schedule for uninstall: " + bundlesToUninstall);
-
-        osgi.allowDuplicateBundles(bundlesToUninstall);
+        obsoleteBundles = removeObsoleteReferences(newFileReferences);
+        osgi.allowDuplicateBundles(obsoleteBundles.values());
 
         bundlesFromNewGeneration = installBundles(newFileReferences);
         BundleStarter.startBundles(activeBundles.values());
         log.info(installedBundlesMessage());
 
         readyForNewBundles = false;
-
-        return bundlesToUninstall;
     }
 
-    public synchronized Collection<Bundle> completeGeneration(GenerationStatus status) {
-        Collection<Bundle> ret = List.of();
+    public synchronized Set<Bundle> completeGeneration(GenerationStatus status) {
+        Set<Bundle> ret = Set.of();
         if (readyForNewBundles) return ret;
 
+        readyForNewBundles = true;
         if (status == GenerationStatus.SUCCESS) {
-            commitBundles();
+            return commitBundles();
         } else {
-            ret = revertToPreviousGeneration();
+            return revertToPreviousGeneration();
         }
-        readyForNewBundles = true;
-        return ret;
     }
 
     /**
      * Commit to the current set of bundles. Must be called after the component graph creation proved successful,
      * to prevent uninstalling bundles unintentionally upon a future call to {@link #revertToPreviousGeneration()}.
+     * Returns the set of bundles that is no longer used by the application, and should therefore be scheduled
+     * for uninstall.
      */
-    private void commitBundles() {
+    private Set<Bundle> commitBundles() {
+        var bundlesToUninstall = new LinkedHashSet<>(obsoleteBundles.values());
+        log.info("Bundles to be uninstalled from previous generation: " + bundlesToUninstall);
+
         bundlesFromNewGeneration = Map.of();
         obsoleteBundles = Map.of();
         readyForNewBundles = true;
+        return bundlesToUninstall;
     }
 
     /**
@@ -102,12 +99,12 @@ public class ApplicationBundleLoader {
      * exclusively belongs to the latest (failed) application generation. Uninstalling must
      * be done by the Deconstructor as they may still be needed by components from the failed gen.
      */
-    private Collection<Bundle> revertToPreviousGeneration() {
+    private Set<Bundle> revertToPreviousGeneration() {
         log.info("Reverting to previous generation with bundles: " + obsoleteBundles);
         log.info("Bundles from latest generation will be removed: " + bundlesFromNewGeneration);
         activeBundles.putAll(obsoleteBundles);
         bundlesFromNewGeneration.forEach(activeBundles::remove);
-        Collection<Bundle> ret = bundlesFromNewGeneration.values();
+        var ret = new LinkedHashSet<>(bundlesFromNewGeneration.values());
 
         // For correct operation of the CollisionHook (more specifically its FindHook implementation), the set of
         // allowed duplicates must reflect the next set of bundles to uninstall, which is now the bundles from the
@@ -128,7 +125,7 @@ public class ApplicationBundleLoader {
      *
      * Returns the map of bundles that are not needed by the new application generation.
      */
-    private Map<FileReference, Bundle> removeObsoleteBundles(List<FileReference> newReferences) {
+    private Map<FileReference, Bundle> removeObsoleteReferences(List<FileReference> newReferences) {
         Map<FileReference, Bundle> obsoleteReferences = new LinkedHashMap<>(activeBundles);
         newReferences.forEach(obsoleteReferences::remove);
 
diff --git a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
index f573de1f4ac..e2f97c9ad6b 100644
--- a/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
+++ b/container-core/src/main/java/com/yahoo/container/core/config/HandlersConfigurerDi.java
@@ -98,13 +98,13 @@ public class HandlersConfigurerDi {
         }
 
         @Override
-        public Set<Bundle> useApplicationBundles(Collection<FileReference> bundles, long generation) {
+        public void useApplicationBundles(Collection<FileReference> bundles, long generation) {
             log.info("Installing bundles for application generation " + generation);
-            return applicationBundleLoader.useBundles(new ArrayList<>(bundles));
+            applicationBundleLoader.useBundles(new ArrayList<>(bundles));
         }
 
         @Override
-        public Collection<Bundle> completeBundleGeneration(GenerationStatus status) {
+        public Set<Bundle> completeBundleGeneration(GenerationStatus status) {
             return applicationBundleLoader.completeGeneration(status);
         }
     }
diff --git a/container-core/src/main/java/com/yahoo/container/di/Container.java b/container-core/src/main/java/com/yahoo/container/di/Container.java
index c34e393ac02..5913c5e79d1 100644
--- a/container-core/src/main/java/com/yahoo/container/di/Container.java
+++ b/container-core/src/main/java/com/yahoo/container/di/Container.java
@@ -71,9 +71,8 @@ public class Container {
     public ComponentGraphResult waitForNextGraphGeneration(ComponentGraph oldGraph, Injector fallbackInjector, boolean isInitializing) {
         try {
             ComponentGraph newGraph;
-            Collection<Bundle> obsoleteBundles = new HashSet<>();
             try {
-                newGraph = waitForNewConfigGenAndCreateGraph(oldGraph, fallbackInjector, isInitializing, obsoleteBundles);
+                newGraph = waitForNewConfigGenAndCreateGraph(oldGraph, fallbackInjector, isInitializing);
                 newGraph.reuseNodes(oldGraph);
             } catch (Throwable t) {
                 log.warning("Failed to set up component graph - uninstalling latest bundles. Bootstrap generation: " + getBootstrapGeneration());
@@ -89,9 +88,8 @@ public class Container {
                 deconstructFailedGraph(oldGraph, newGraph, newBundlesFromFailedGen);
                 throw e;
             }
-            // TODO: take obsoleteBundles as return value here!
-            osgi.completeBundleGeneration(Osgi.GenerationStatus.SUCCESS);
-            Runnable cleanupTask = createPreviousGraphDeconstructionTask(oldGraph, newGraph, obsoleteBundles);
+            Collection<Bundle> unusedBundlesFromPreviousGen = osgi.completeBundleGeneration(Osgi.GenerationStatus.SUCCESS);
+            Runnable cleanupTask = createPreviousGraphDeconstructionTask(oldGraph, newGraph, unusedBundlesFromPreviousGen);
             return new ComponentGraphResult(newGraph, cleanupTask);
         } catch (Throwable t) {
             invalidateGeneration(oldGraph.generation(), t);
@@ -108,7 +106,7 @@ public class Container {
     }
 
     private ComponentGraph waitForNewConfigGenAndCreateGraph(
-            ComponentGraph graph, Injector fallbackInjector, boolean isInitializing, Collection<Bundle> obsoleteBundles) // NOTE: Return value
+            ComponentGraph graph, Injector fallbackInjector, boolean isInitializing)
     {
         ConfigSnapshot snapshot;
         while (true) {
@@ -132,8 +130,7 @@ public class Container {
                 } else {
                     throwIfPlatformBundlesChanged(snapshot);
                 }
-                Collection<Bundle> bundlesToRemove = installApplicationBundles(snapshot.configs());
-                obsoleteBundles.addAll(bundlesToRemove);
+                installApplicationBundles(snapshot.configs());
 
                 graph = createComponentGraph(snapshot.configs(), getBootstrapGeneration(), fallbackInjector);
 
@@ -203,9 +200,9 @@ public class Container {
         return () -> destructor.deconstruct(oldGraph.generation(), obsoleteComponents, obsoleteBundles);
     }
 
-    private Set<Bundle> installApplicationBundles(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs) {
+    private void installApplicationBundles(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs) {
         ApplicationBundlesConfig applicationBundlesConfig = getConfig(applicationBundlesConfigKey, configsIncludingBootstrapConfigs);
-        return osgi.useApplicationBundles(applicationBundlesConfig.bundles(), getBootstrapGeneration());
+        osgi.useApplicationBundles(applicationBundlesConfig.bundles(), getBootstrapGeneration());
     }
 
     private ComponentGraph createComponentGraph(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs,
diff --git a/container-core/src/main/java/com/yahoo/container/di/Osgi.java b/container-core/src/main/java/com/yahoo/container/di/Osgi.java
index c13ebca6f62..d245767120c 100644
--- a/container-core/src/main/java/com/yahoo/container/di/Osgi.java
+++ b/container-core/src/main/java/com/yahoo/container/di/Osgi.java
@@ -30,28 +30,25 @@ public interface Osgi {
     }
 
     /**
-     * TODO: return void and let all obsolete bundles be returned by completeBundleGeneration
-     *
-     * Returns the set of bundles that is not needed by the new application generation,
-     * and therefore should be scheduled for uninstalling.
+     * Installs the new set of application bundles.
      *
      * @param bundles The bundles for the new application.
      * @param generation The generation number of the new application.
-     * @return the set of bundles that is not needed by the new application generation,
      */
-    default Set<Bundle> useApplicationBundles(Collection<FileReference> bundles, long generation) {
-        return emptySet();
+    default void useApplicationBundles(Collection<FileReference> bundles, long generation) {
     }
 
     /**
+     * If the current generation is a success, the set of bundles that is not needed by the new application
+     * generation, and therefore should be scheduled for uninstalling, is returned.
      * If the current generation is a failure, all state related to application bundles is reverted to
      * the previous generation. The set of bundles that was exclusively needed by the new generation,
      * and therefore should be scheduled for uninstalling, is returned.
      *
      * @param status The success or failure of the new generation
-     * @return The set of bundles that are no longer needed by the latest good generation.
+     * @return The set of bundles that are no longer needed by the new or latest good generation.
      */
-    default Collection<Bundle> completeBundleGeneration(GenerationStatus status) {
+    default Set<Bundle> completeBundleGeneration(GenerationStatus status) {
         return emptySet();
     }