Merge pull request #23847 from vespa-engine/cleanup-after-failed-component-graph_rebased

Cleanup after failed component graph [run-systemtest]

11 files changed, 304 insertions, 171 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 f0ca5aee8a2..1e30b19a48d 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
@@ -6,13 +6,14 @@ 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;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 
 /**
  * Manages the set of installed and active/inactive bundles.
@@ -24,16 +25,15 @@ public class ApplicationBundleLoader {
 
     private static final Logger log = Logger.getLogger(ApplicationBundleLoader.class.getName());
 
-    /**
-     * Map of file refs of active bundles (not scheduled for uninstall) to the installed bundle.
-     *
-     * Used to:
-     * 1. Avoid installing already installed bundles. Just an optimization, installing the same bundle location is a NOP
-     * 2. Start bundles (all are started every time)
-     * 3. Calculate the set of bundles to uninstall
-     */
+    // The active bundles for the current application generation.
     private final Map<FileReference, Bundle> reference2Bundle = new LinkedHashMap<>();
 
+    // The bundles that are obsolete from the previous generation, but kept in case the generation is reverted.
+    private Map<FileReference, Bundle> obsoleteBundles = Map.of();
+
+    // The bundles that exclusively belong to the current application generation.
+    private Map<FileReference, Bundle> bundlesFromNewGeneration = Map.of();
+
     private final Osgi osgi;
     private final FileAcquirerBundleInstaller bundleInstaller;
 
@@ -48,54 +48,78 @@ public class ApplicationBundleLoader {
      */
     public synchronized Set<Bundle> useBundles(List<FileReference> newFileReferences) {
 
-        Set<FileReference> obsoleteReferences = getObsoleteFileReferences(newFileReferences);
-        Set<Bundle> bundlesToUninstall = getObsoleteBundles(obsoleteReferences);
+        obsoleteBundles = removeObsoleteBundles(newFileReferences);
+        Set<Bundle> bundlesToUninstall = new LinkedHashSet<>(obsoleteBundles.values());
         log.info("Bundles to schedule for uninstall: " + bundlesToUninstall);
 
         osgi.allowDuplicateBundles(bundlesToUninstall);
-        removeInactiveFileReferences(obsoleteReferences);
 
-        installBundles(newFileReferences);
+        bundlesFromNewGeneration = installBundles(newFileReferences);
         BundleStarter.startBundles(reference2Bundle.values());
         log.info(installedBundlesMessage());
 
         return bundlesToUninstall;
     }
 
-    private Set<FileReference> getObsoleteFileReferences(List<FileReference> newReferences) {
-        Set<FileReference> obsoleteReferences = new HashSet<>(reference2Bundle.keySet());
-        obsoleteReferences.removeAll(newReferences);
-        return obsoleteReferences;
+    /**
+     * Restores state from the previous application generation and returns the set of bundles that
+     * 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.
+     */
+    public synchronized Collection<Bundle> revertToPreviousGeneration() {
+        reference2Bundle.putAll(obsoleteBundles);
+        bundlesFromNewGeneration.forEach(reference2Bundle::remove);
+        Collection<Bundle> ret = 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
+        // failed generation.
+        osgi.allowDuplicateBundles(ret);
+
+        // Clear restore info in case this method is called multiple times, for some reason.
+        bundlesFromNewGeneration = Map.of();
+        obsoleteBundles = Map.of();
+
+        return ret;
     }
 
     /**
-     * Returns the bundles that will not be retained by the new application generation.
+     * Calculates the set of bundles that are not needed by the new application generation and
+     * removes them from the map of active bundles.
+     *
+     * Returns the map of bundles that are not needed by the new application generation.
      */
-    private Set<Bundle> getObsoleteBundles(Set<FileReference> obsoleteReferences) {
-        return obsoleteReferences.stream().map(reference2Bundle::get).collect(Collectors.toSet());
-    }
+    private Map<FileReference, Bundle> removeObsoleteBundles(List<FileReference> newReferences) {
+        Map<FileReference, Bundle> obsoleteReferences = new LinkedHashMap<>(reference2Bundle);
+        newReferences.forEach(obsoleteReferences::remove);
 
-    private void removeInactiveFileReferences(Set<FileReference> fileReferencesToRemove) {
-        fileReferencesToRemove.forEach(reference2Bundle::remove);
+        obsoleteReferences.forEach(reference2Bundle::remove);
+        return obsoleteReferences;
     }
 
-    private void installBundles(List<FileReference> references) {
+    /**
+     * Returns the set of new bundles that were installed.
+     */
+    private Map<FileReference, Bundle> installBundles(List<FileReference> references) {
         Set<FileReference> bundlesToInstall = new HashSet<>(references);
 
         // This is just an optimization, as installing a bundle with the same location id returns the already installed bundle.
         bundlesToInstall.removeAll(reference2Bundle.keySet());
 
-        if (!bundlesToInstall.isEmpty()) {
-            if (bundleInstaller.hasFileDistribution()) {
-                installWithFileDistribution(bundlesToInstall, bundleInstaller);
-            } else {
-                log.warning("Can't retrieve bundles since file distribution is disabled.");
-            }
+        if (bundlesToInstall.isEmpty()) return Map.of();
+
+        if (bundleInstaller.hasFileDistribution()) {
+            return installWithFileDistribution(bundlesToInstall, bundleInstaller);
+        } else {
+            log.warning("Can't retrieve bundles since file distribution is disabled.");
+            return Map.of();
         }
     }
 
-    private void installWithFileDistribution(Set<FileReference> bundlesToInstall,
-                                             FileAcquirerBundleInstaller bundleInstaller) {
+    private Map<FileReference, Bundle> installWithFileDistribution(Set<FileReference> bundlesToInstall,
+                                                    FileAcquirerBundleInstaller bundleInstaller) {
+        var newBundles = new LinkedHashMap<FileReference, Bundle>();
+
         for (FileReference reference : bundlesToInstall) {
             try {
                 log.info("Installing bundle with reference '" + reference.value() + "'");
@@ -108,11 +132,13 @@ public class ApplicationBundleLoader {
                     throw new RuntimeException("Bundle '" + bundles.get(0).getSymbolicName() + "' tried to pre-install bundles from disk.");
                 }
                 reference2Bundle.put(reference, bundles.get(0));
+                newBundles.put(reference, bundles.get(0));
             }
             catch(Exception e) {
                 throw new RuntimeException("Could not install bundle with reference '" + reference + "'", e);
             }
         }
+        return newBundles;
     }
 
     private String installedBundlesMessage() {
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 363ff26eea9..3fd8b8dc6a9 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
@@ -99,9 +99,15 @@ public class HandlersConfigurerDi {
 
         @Override
         public Set<Bundle> useApplicationBundles(Collection<FileReference> bundles) {
-            log.info("Installing bundles from the latest application");
+            log.info("Installing bundles from the latest application.");
             return applicationBundleLoader.useBundles(new ArrayList<>(bundles));
         }
+
+        @Override
+        public Collection<Bundle> revertApplicationBundles() {
+            log.info("Reverting to bundles from the previous generation.");
+            return applicationBundleLoader.revertToPreviousGeneration();
+        }
     }
 
     /**
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 1baf217da6b..db463edecc0 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
@@ -67,11 +67,7 @@ public class Container {
         this.retriever = new ConfigRetriever(bootstrapKeys, subscriberFactory);
     }
 
-    public Container(SubscriberFactory subscriberFactory, String configId, ComponentDeconstructor destructor) {
-        this(subscriberFactory, configId, destructor, new Osgi() {
-        });
-    }
-
+    // TODO: try to simplify by returning the result even when the graph failed, instead of throwing here.
     public ComponentGraphResult waitForNextGraphGeneration(ComponentGraph oldGraph, Injector fallbackInjector, boolean isInitializing) {
         try {
             Collection<Bundle> obsoleteBundles = new HashSet<>();
@@ -83,7 +79,9 @@ public class Container {
                 log.log(Level.WARNING, String.format(
                         "Failed to construct graph for generation '%d' - scheduling partial graph for deconstruction",
                         newGraph.generation()), e);
-                deconstructFailedGraph(oldGraph, newGraph);
+
+                Collection<Bundle> newBundlesFromFailedGen = osgi.revertApplicationBundles();
+                deconstructFailedGraph(oldGraph, newGraph, newBundlesFromFailedGen);
                 throw e;
             }
             Runnable cleanupTask = createPreviousGraphDeconstructionTask(oldGraph, newGraph, obsoleteBundles);
@@ -94,6 +92,14 @@ public class Container {
         }
     }
 
+    private void constructComponents(ComponentGraph graph) {
+        graph.nodes().forEach(n -> {
+            if (Thread.interrupted())
+                throw new UncheckedInterruptedException("Interrupted while constructing component graph", true);
+            n.constructInstance();
+        });
+    }
+
     private ComponentGraph waitForNewConfigGenAndCreateGraph(
             ComponentGraph graph, Injector fallbackInjector, boolean isInitializing, Collection<Bundle> obsoleteBundles) // NOTE: Return value
     {
@@ -122,7 +128,7 @@ public class Container {
                 Collection<Bundle> bundlesToRemove = installApplicationBundles(snapshot.configs());
                 obsoleteBundles.addAll(bundlesToRemove);
 
-                graph = createComponentsGraph(snapshot.configs(), getBootstrapGeneration(), fallbackInjector);
+                graph = createComponentGraph(snapshot.configs(), getBootstrapGeneration(), fallbackInjector);
 
                 // Continues loop
 
@@ -131,7 +137,7 @@ public class Container {
             }
         }
         log.log(FINE, () -> "Got components configs,\n" + configGenerationsString());
-        return createAndConfigureComponentsGraph(snapshot.configs(), fallbackInjector);
+        return createAndConfigureComponentGraph(snapshot.configs(), fallbackInjector);
     }
 
     private long getBootstrapGeneration() {
@@ -153,22 +159,14 @@ public class Container {
             throw new RuntimeException("Platform bundles are not allowed to change!\nOld: " + platformBundles + "\nNew: " + checkPlatformBundles);
     }
 
-    private ComponentGraph createAndConfigureComponentsGraph(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> componentsConfigs,
-                                                             Injector fallbackInjector) {
-        ComponentGraph componentGraph = createComponentsGraph(componentsConfigs, getComponentsGeneration(), fallbackInjector);
+    private ComponentGraph createAndConfigureComponentGraph(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> componentsConfigs,
+                                                            Injector fallbackInjector) {
+        ComponentGraph componentGraph = createComponentGraph(componentsConfigs, getComponentsGeneration(), fallbackInjector);
         componentGraph.setAvailableConfigs(componentsConfigs);
         return componentGraph;
     }
 
-    private void constructComponents(ComponentGraph graph) {
-        graph.nodes().forEach(n -> {
-            if (Thread.interrupted())
-                throw new UncheckedInterruptedException("Interrupted while constructing component graph", true);
-            n.constructInstance();
-        });
-    }
-
-    private void deconstructFailedGraph(ComponentGraph currentGraph, ComponentGraph failedGraph) {
+    private void deconstructFailedGraph(ComponentGraph currentGraph, ComponentGraph failedGraph, Collection<Bundle> bundlesFromFailedGraph) {
         Set<Object> currentComponents = Collections.newSetFromMap(new IdentityHashMap<>(currentGraph.size()));
         currentComponents.addAll(currentGraph.allConstructedComponentsAndProviders());
 
@@ -176,7 +174,7 @@ public class Container {
         for (Object component : failedGraph.allConstructedComponentsAndProviders()) {
             if (!currentComponents.contains(component)) unusedComponents.add(component);
         }
-        destructor.deconstruct(failedGraph.generation(), unusedComponents, List.of());
+        destructor.deconstruct(failedGraph.generation(), unusedComponents, bundlesFromFailedGraph);
     }
 
     private Runnable createPreviousGraphDeconstructionTask(ComponentGraph oldGraph,
@@ -199,8 +197,8 @@ public class Container {
         return osgi.useApplicationBundles(applicationBundlesConfig.bundles());
     }
 
-    private ComponentGraph createComponentsGraph(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs,
-                                                 long generation, Injector fallbackInjector) {
+    private ComponentGraph createComponentGraph(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs,
+                                                long generation, Injector fallbackInjector) {
         previousConfigGeneration = generation;
 
         ComponentGraph graph = new ComponentGraph(generation);
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 a3ebc909a6e..02f42fdb9b8 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
@@ -9,7 +9,6 @@ import org.osgi.framework.Bundle;
 
 import java.util.Collection;
 import java.util.Set;
-import java.util.stream.Collectors;
 
 import static java.util.Collections.emptySet;
 
@@ -17,6 +16,8 @@ import static java.util.Collections.emptySet;
  * This interface has default implementations of all methods, to allow using it
  * for testing, instead of mocking or a test implementation.
  *
+ * TODO: remove test code from this interface.
+ *
  * @author gjoranv
  * @author Tony Vaagenes
  * @author ollivir
@@ -24,7 +25,6 @@ import static java.util.Collections.emptySet;
 public interface Osgi {
 
     default void installPlatformBundles(Collection<String> bundlePaths) {
-        //System.out.println("installPlatformBundles " + bundlePaths);
     }
 
     /**
@@ -32,12 +32,21 @@ public interface Osgi {
      * and therefore should be scheduled for uninstalling.
      */
     default Set<Bundle> useApplicationBundles(Collection<FileReference> bundles) {
-        //System.out.println("useBundles " + bundles.stream().map(Object::toString).collect(Collectors.joining(", ")));
+        return emptySet();
+    }
+
+    /**
+     * To be used when a new application generation fails, e.g. during component construction.
+     * Reverts all state related to application bundles to the previous generation.
+     *
+     * Returns the set of bundles that was exclusively used by the new generation,
+     * and therefore should be scheduled for uninstalling.
+     */
+    default Collection<Bundle> revertApplicationBundles() {
         return emptySet();
     }
 
     default Class<?> resolveClass(BundleInstantiationSpecification spec) {
-        //System.out.println("resolving class " + spec.classId);
         try {
             return Class.forName(spec.classId.getName());
         } catch (ClassNotFoundException e) {
@@ -46,7 +55,6 @@ public interface Osgi {
     }
 
     default Bundle getBundle(ComponentSpecification spec) {
-        //System.out.println("resolving bundle " + spec);
         return new MockBundle();
     }
 }
diff --git a/container-core/src/main/java/com/yahoo/osgi/Osgi.java b/container-core/src/main/java/com/yahoo/osgi/Osgi.java
index c8fa901dd0a..10cd41cce08 100644
--- a/container-core/src/main/java/com/yahoo/osgi/Osgi.java
+++ b/container-core/src/main/java/com/yahoo/osgi/Osgi.java
@@ -22,6 +22,7 @@ public interface Osgi {
 
     List<Bundle> install(String absolutePath);
 
+    /** Sets the collection of bundles to allow duplicates for. */
     void allowDuplicateBundles(Collection<Bundle> bundles);
 
     default boolean hasFelixFramework() {
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java b/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java
index cf8cc6053cd..54ef2d41ac6 100644
--- a/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java
+++ b/container-core/src/test/java/com/yahoo/container/core/config/ApplicationBundleLoaderTest.java
@@ -1,18 +1,19 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.core.config;
 
-import com.yahoo.config.FileReference;
-import com.yahoo.filedistribution.fileacquirer.FileAcquirer;
-import com.yahoo.filedistribution.fileacquirer.MockFileAcquirer;
-import com.yahoo.osgi.Osgi;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.osgi.framework.Bundle;
 
+import java.util.Collection;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_1;
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_1_REF;
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_2;
+import static com.yahoo.container.core.config.BundleTestUtil.BUNDLE_2_REF;
+import static com.yahoo.container.core.config.BundleTestUtil.testBundles;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -21,20 +22,13 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
  */
 public class ApplicationBundleLoaderTest {
 
-    private static final FileReference BUNDLE_1_REF = new FileReference("bundle-1");
-    private static final Bundle BUNDLE_1 = new TestBundle(BUNDLE_1_REF.value());
-    private static final FileReference BUNDLE_2_REF = new FileReference("bundle-2");
-    private static final Bundle BUNDLE_2 = new TestBundle(BUNDLE_2_REF.value());
-
     private ApplicationBundleLoader bundleLoader;
     private TestOsgi osgi;
 
     @BeforeEach
     public void setup() {
         osgi = new TestOsgi(testBundles());
-        var bundleInstaller = new TestBundleInstaller(MockFileAcquirer.returnFile(null));
-
-        bundleLoader = new ApplicationBundleLoader(osgi, bundleInstaller);
+        bundleLoader = osgi.bundleLoader();
     }
 
     @Test
@@ -70,7 +64,6 @@ public class ApplicationBundleLoaderTest {
         assertEquals(BUNDLE_1.getSymbolicName(), osgi.getCurrentBundles().get(0).getSymbolicName());
         assertEquals(BUNDLE_2.getSymbolicName(), osgi.getCurrentBundles().get(1).getSymbolicName());
 
-
         // Both file references are active
         assertEquals(2, bundleLoader.getActiveFileReferences().size());
         assertEquals(BUNDLE_1_REF, bundleLoader.getActiveFileReferences().get(0));
@@ -101,22 +94,26 @@ public class ApplicationBundleLoaderTest {
     }
 
 
-    private static Map<String, Bundle> testBundles() {
-        return Map.of(BUNDLE_1_REF.value(), BUNDLE_1,
-                      BUNDLE_2_REF.value(), BUNDLE_2);
-    }
+    @Test
+    void previous_generation_can_be_restored_after_a_failed_reconfig() {
+        bundleLoader.useBundles(List.of(BUNDLE_1_REF));
+        Set<Bundle> obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_2_REF));
+        assertEquals(BUNDLE_1.getSymbolicName(), obsoleteBundles.iterator().next().getSymbolicName());
 
-    static class TestBundleInstaller extends FileAcquirerBundleInstaller {
+        // Revert to the previous generation, as will be done upon a failed reconfig.
+        Collection<Bundle> bundlesToUninstall = bundleLoader.revertToPreviousGeneration();
 
-        TestBundleInstaller(FileAcquirer fileAcquirer) {
-            super(fileAcquirer);
-        }
+        assertEquals(1, bundlesToUninstall.size());
+        assertEquals(BUNDLE_2.getSymbolicName(), bundlesToUninstall.iterator().next().getSymbolicName());
 
-        @Override
-        public List<Bundle> installBundles(FileReference reference, Osgi osgi) {
-            return osgi.install(reference.value());
-        }
+        // The bundle from the failed generation is not seen as current. It will still be installed,
+        // as uninstalling is handled by the Deconstructor, not included in this test setup.
+        assertEquals(1, osgi.getCurrentBundles().size());
+        assertEquals(BUNDLE_1.getSymbolicName(), osgi.getCurrentBundles().get(0).getSymbolicName());
 
+        // Only the bundle-1 file reference is active, bundle-2 is removed.
+        assertEquals(1, bundleLoader.getActiveFileReferences().size());
+        assertEquals(BUNDLE_1_REF, bundleLoader.getActiveFileReferences().get(0));
     }
 
 }
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/BundleTestUtil.java b/container-core/src/test/java/com/yahoo/container/core/config/BundleTestUtil.java
new file mode 100644
index 00000000000..fddbeaf42be
--- /dev/null
+++ b/container-core/src/test/java/com/yahoo/container/core/config/BundleTestUtil.java
@@ -0,0 +1,39 @@
+package com.yahoo.container.core.config;
+
+import com.yahoo.config.FileReference;
+import com.yahoo.filedistribution.fileacquirer.FileAcquirer;
+import com.yahoo.osgi.Osgi;
+import org.osgi.framework.Bundle;
+
+import java.util.List;
+import java.util.Map;
+
+/**
+ * @author gjoranv
+ */
+public class BundleTestUtil {
+
+    public static final FileReference BUNDLE_1_REF = new FileReference("bundle-1");
+    public static final Bundle BUNDLE_1 = new TestBundle(BUNDLE_1_REF.value());
+    public static final FileReference BUNDLE_2_REF = new FileReference("bundle-2");
+    public static final Bundle BUNDLE_2 = new TestBundle(BUNDLE_2_REF.value());
+
+    public static Map<String, Bundle> testBundles() {
+        return Map.of(BUNDLE_1_REF.value(), BUNDLE_1,
+                      BUNDLE_2_REF.value(), BUNDLE_2);
+    }
+
+    public static class TestBundleInstaller extends FileAcquirerBundleInstaller {
+
+        TestBundleInstaller(FileAcquirer fileAcquirer) {
+            super(fileAcquirer);
+        }
+
+        @Override
+        public List<Bundle> installBundles(FileReference reference, Osgi osgi) {
+            return osgi.install(reference.value());
+        }
+
+    }
+
+}
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java b/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java
index 4a607520b2a..17709844f99 100644
--- a/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java
+++ b/container-core/src/test/java/com/yahoo/container/core/config/TestBundle.java
@@ -16,7 +16,7 @@ import java.util.List;
 /**
  * @author gjoranv
  */
-class TestBundle extends MockBundle {
+public class TestBundle extends MockBundle {
 
     private static final BundleRevision revision = new TestBundleRevision();
 
@@ -24,7 +24,7 @@ class TestBundle extends MockBundle {
 
     boolean started = false;
 
-    TestBundle(String symbolicName) {
+    public TestBundle(String symbolicName) {
         this.symbolicName = symbolicName;
     }
 
diff --git a/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java b/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java
index 2e22af889ed..0df338c2144 100644
--- a/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java
+++ b/container-core/src/test/java/com/yahoo/container/core/config/TestOsgi.java
@@ -1,6 +1,8 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.core.config;
 
+import com.yahoo.config.FileReference;
+import com.yahoo.filedistribution.fileacquirer.MockFileAcquirer;
 import com.yahoo.osgi.MockOsgi;
 import org.osgi.framework.Bundle;
 
@@ -8,21 +10,28 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * @author gjoranv
  */
-class TestOsgi extends MockOsgi {
+public class TestOsgi extends MockOsgi implements com.yahoo.container.di.Osgi {
 
+    private final ApplicationBundleLoader bundleLoader;
     private final Map<String, Bundle> availableBundles;
 
     private final List<Bundle> installedBundles = new ArrayList<>();
     private final List<Bundle> allowedDuplicates = new ArrayList<>();
 
-    TestOsgi(Map<String, Bundle> availableBundles) {
+    public TestOsgi(Map<String, Bundle> availableBundles) {
         this.availableBundles = availableBundles;
+
+        var bundleInstaller = new BundleTestUtil.TestBundleInstaller(MockFileAcquirer.returnFile(null));
+        bundleLoader = new ApplicationBundleLoader(this, bundleInstaller);
     }
 
+    public ApplicationBundleLoader bundleLoader() { return bundleLoader; }
+
     @Override
     public List<Bundle> install(String fileReferenceValue) {
         if (! availableBundles.containsKey(fileReferenceValue))
@@ -51,7 +60,21 @@ class TestOsgi extends MockOsgi {
 
     @Override
     public void allowDuplicateBundles(Collection<Bundle> bundles) {
+        allowedDuplicates.clear();
         allowedDuplicates.addAll(bundles);
     }
 
+    @Override
+    public Set<Bundle> useApplicationBundles(Collection<FileReference> bundles) {
+        return bundleLoader.useBundles(new ArrayList<>(bundles));
+    }
+
+    @Override
+    public Collection<Bundle> revertApplicationBundles() {
+        return bundleLoader.revertToPreviousGeneration();
+    }
+
+    public void removeBundle(Bundle bundle) {
+        installedBundles.remove(bundle);
+    }
 }
diff --git a/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java b/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java
index 04091c25ad6..ccd60390129 100644
--- a/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java
+++ b/container-core/src/test/java/com/yahoo/container/di/ContainerTest.java
@@ -1,7 +1,6 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.container.di;
 
-import com.google.inject.Guice;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.config.di.IntConfig;
 import com.yahoo.config.test.TestConfig;
@@ -11,16 +10,20 @@ import com.yahoo.container.di.componentgraph.core.ComponentGraphTest.SimpleCompo
 import com.yahoo.container.di.componentgraph.core.ComponentNode.ComponentConstructorException;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
-import org.osgi.framework.Bundle;
 
-import java.util.Collection;
 import java.util.List;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * @author Tony Vaagenes
@@ -88,6 +91,28 @@ public class ContainerTest extends ContainerTestBase {
         container.shutdownConfigRetriever();
     }
 
+    @Test
+    void bundle_from_previous_generation_is_uninstalled_when_not_used_in_the_new_generation() {
+        ComponentEntry component1 = new ComponentEntry("component1", SimpleComponent.class);
+        ComponentEntry component2 = new ComponentEntry("component2", SimpleComponent.class);
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-1"), List.of(component1));
+        Container container = newContainer(dirConfigSource);
+        ComponentGraph graph = getNewComponentGraph(container);
+
+        // bundle-1 is installed
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-1", osgi.getBundles()[0].getSymbolicName());
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-2"), List.of(component2));
+        container.reloadConfig(2);
+        getNewComponentGraph(container, graph);
+
+        // bundle-1 is kept, bundle-2 has been uninstalled
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-2", osgi.getBundles()[0].getSymbolicName());
+    }
+
     //@Test TODO
     public void deconstructor_is_given_guice_components() {
     }
@@ -179,6 +204,34 @@ public class ContainerTest extends ContainerTestBase {
     }
 
     @Test
+    void bundle_from_failing_generation_is_uninstalled() {
+        ComponentEntry simpleComponentEntry = new ComponentEntry("simpleComponent", SimpleComponent.class);
+        ComponentEntry throwingComponentEntry = new ComponentEntry("throwingComponent", ComponentThrowingExceptionInConstructor.class);
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-1"), List.of(simpleComponentEntry));
+        Container container = newContainer(dirConfigSource);
+        ComponentGraph currentGraph = getNewComponentGraph(container);
+
+        // bundle-1 is installed
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-1", osgi.getBundles()[0].getSymbolicName());
+
+        writeBootstrapConfigsWithBundles(List.of("bundle-2"), List.of(throwingComponentEntry));
+        container.reloadConfig(2);
+        try {
+            currentGraph = getNewComponentGraph(container, currentGraph);
+            fail("Expected exception");
+        } catch (ComponentConstructorException ignored) {
+            // Expected, do nothing
+        }
+        assertEquals(1, currentGraph.generation());
+
+        // bundle-1 is kept, bundle-2 has been uninstalled
+        assertEquals(1, osgi.getBundles().length);
+        assertEquals("bundle-1", osgi.getBundles()[0].getSymbolicName());
+    }
+
+    @Test
     void getNewComponentGraph_hangs_waiting_for_valid_config_after_invalid_config() throws Exception {
         dirConfigSource.writeConfig("test", "stringVal \"original\"");
         writeBootstrapConfigs("myId", ComponentTakingConfig.class);
@@ -303,38 +356,6 @@ public class ContainerTest extends ContainerTestBase {
         }
     }
 
-    public static class TestDeconstructor implements ComponentDeconstructor {
-        @Override
-        public void deconstruct(long generation, List<Object> components, Collection<Bundle> bundles) {
-            components.forEach(component -> {
-                if (component instanceof DestructableComponent) {
-                    DestructableComponent vespaComponent = (DestructableComponent) component;
-                    vespaComponent.deconstruct();
-                }
-            });
-            if (! bundles.isEmpty()) throw new IllegalArgumentException("This test should not use bundles");
-        }
-    }
-
-    private static Container newContainer(DirConfigSource dirConfigSource,
-                                          ComponentDeconstructor deconstructor) {
-        return new Container(new CloudSubscriberFactory(dirConfigSource.configSource), dirConfigSource.configId(), deconstructor);
-    }
-
-    private static Container newContainer(DirConfigSource dirConfigSource) {
-        return newContainer(dirConfigSource, new TestDeconstructor());
-    }
-
-    ComponentGraph getNewComponentGraph(Container container, ComponentGraph oldGraph) {
-        Container.ComponentGraphResult result = container.waitForNextGraphGeneration(oldGraph, Guice.createInjector(), true);
-        result.oldComponentsCleanupTask().run();
-        return result.newGraph();
-    }
-
-    ComponentGraph getNewComponentGraph(Container container) {
-        return container.waitForNextGraphGeneration(new ComponentGraph(), Guice.createInjector(), true).newGraph();
-    }
-
     private ComponentTakingConfig createComponentTakingConfig(ComponentGraph componentGraph) {
         return componentGraph.getInstance(ComponentTakingConfig.class);
     }
diff --git a/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java b/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java
index 4383bd08f71..7442eb2068d 100644
--- a/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java
+++ b/container-core/src/test/java/com/yahoo/container/di/ContainerTestBase.java
@@ -2,9 +2,8 @@
 package com.yahoo.container.di;
 
 import com.google.inject.Guice;
-import com.yahoo.component.ComponentSpecification;
-import com.yahoo.config.FileReference;
-import com.yahoo.container.bundle.BundleInstantiationSpecification;
+import com.yahoo.container.core.config.BundleTestUtil;
+import com.yahoo.container.core.config.TestOsgi;
 import com.yahoo.container.di.ContainerTest.ComponentTakingConfig;
 import com.yahoo.container.di.componentgraph.core.ComponentGraph;
 import org.junit.jupiter.api.AfterEach;
@@ -14,9 +13,7 @@ import org.osgi.framework.Bundle;
 
 import java.io.File;
 import java.util.Collection;
-import java.util.Set;
-
-import static java.util.Collections.emptySet;
+import java.util.List;
 
 /**
  * @author Tony Vaagenes
@@ -27,6 +24,7 @@ public class ContainerTestBase {
 
     private ComponentGraph componentGraph;
     protected DirConfigSource dirConfigSource = null;
+    protected TestOsgi osgi;
 
     @TempDir
     File tmpDir;
@@ -34,6 +32,8 @@ public class ContainerTestBase {
     @BeforeEach
     public void setup() {
         dirConfigSource = new DirConfigSource(tmpDir, "ContainerTest-");
+        componentGraph = new ComponentGraph(0);
+        osgi = new TestOsgi(BundleTestUtil.testBundles());
     }
 
     @AfterEach
@@ -41,47 +41,43 @@ public class ContainerTestBase {
         dirConfigSource.cleanup();
     }
 
-    @BeforeEach
-    public void createGraph() {
-        componentGraph = new ComponentGraph(0);
+
+    protected Container newContainer(DirConfigSource dirConfigSource,
+                                            ComponentDeconstructor deconstructor) {
+        return new Container(new CloudSubscriberFactory(dirConfigSource.configSource),
+                             dirConfigSource.configId(),
+                             deconstructor,
+                             osgi);
     }
 
-    public void complete() {
-        try {
-            Container container = new Container(new CloudSubscriberFactory(dirConfigSource.configSource()), dirConfigSource.configId(),
-                    new ContainerTest.TestDeconstructor(), new Osgi() {
-                        @SuppressWarnings("unchecked")
-                        @Override
-                        public Class<Object> resolveClass(BundleInstantiationSpecification spec) {
-                            try {
-                                return (Class<Object>) Class.forName(spec.classId.getName());
-                            } catch (ClassNotFoundException e) {
-                                throw new RuntimeException(e);
-                            }
-                        }
-
-                        @Override
-                        public Set<Bundle> useApplicationBundles(Collection<FileReference> bundles) {
-                            return emptySet();
-                        }
-
-                        @Override
-                        public Bundle getBundle(ComponentSpecification spec) {
-                            throw new UnsupportedOperationException("getBundle not supported.");
-                        }
-                    });
-            Container.ComponentGraphResult result = container.waitForNextGraphGeneration(this.componentGraph, Guice.createInjector(), true);
-            result.oldComponentsCleanupTask().run();
-            this.componentGraph = result.newGraph();
-        } catch (Exception e) {
-            throw new RuntimeException(e);
-        }
+    protected Container newContainer(DirConfigSource dirConfigSource) {
+        return newContainer(dirConfigSource, new TestDeconstructor(osgi));
     }
 
+    ComponentGraph getNewComponentGraph(Container container, ComponentGraph oldGraph) {
+        Container.ComponentGraphResult result = container.waitForNextGraphGeneration(oldGraph, Guice.createInjector(), true);
+        result.oldComponentsCleanupTask().run();
+        return result.newGraph();
+    }
+
+    ComponentGraph getNewComponentGraph(Container container) {
+        return container.waitForNextGraphGeneration(new ComponentGraph(), Guice.createInjector(), true).newGraph();
+    }
+
+
     public <T> T getInstance(Class<T> componentClass) {
         return componentGraph.getInstance(componentClass);
     }
 
+    protected void writeBootstrapConfigsWithBundles(List<String> applicationBundles, List<ComponentEntry> components) {
+        writeBootstrapConfigs(components.toArray(new ComponentEntry[0]));
+        StringBuilder bundles = new StringBuilder();
+        for (int i = 0; i < applicationBundles.size(); i++) {
+            bundles.append("bundles[" + i + "] \"" + applicationBundles.get(i) + "\"\n");
+        }
+        dirConfigSource.writeConfig("application-bundles", String.format("bundles[%s]\n%s", applicationBundles.size(), bundles));
+    }
+
     protected void writeBootstrapConfigs(ComponentEntry... componentEntries) {
         dirConfigSource.writeConfig("platform-bundles", "");
         dirConfigSource.writeConfig("application-bundles", "");
@@ -115,11 +111,29 @@ public class ContainerTestBase {
         }
 
         String asConfig(int position) {
-            return "<config>\n" + //
-                    "components[" + position + "].id \"" + componentId + "\"\n" + //
-                    "components[" + position + "].classId \"" + classId.getName() + "\"\n" + //
-                    "components[" + position + "].configId \"" + dirConfigSource.configId() + "\"\n" + //
-                    "</config>";
+            return  "components[" + position + "].id \"" + componentId + "\"\n" +
+                    "components[" + position + "].classId \"" + classId.getName() + "\"\n" +
+                    "components[" + position + "].configId \"" + dirConfigSource.configId() + "\"\n" ;
+        }
+    }
+
+
+    public static class TestDeconstructor implements ComponentDeconstructor {
+
+        final TestOsgi osgi;
+
+        public TestDeconstructor(TestOsgi osgi) {
+            this.osgi = osgi;
+        }
+
+        @Override
+        public void deconstruct(long generation, List<Object> components, Collection<Bundle> bundles) {
+            components.forEach(component -> {
+                if (component instanceof ContainerTest.DestructableComponent vespaComponent) {
+                    vespaComponent.deconstruct();
+                }
+            });
+            bundles.forEach(osgi::removeBundle);
         }
     }