Redesign application bundle loading with a 'complete' phase.

- To be able to uninstall bundles from a config generation that has
  failed, without affecting the bundles from the last successful
  generation. (Component reconfig with unchanged bundles was
  not handled correctly.)

7 files changed, 98 insertions, 27 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 c294a7dd3ec..c1cf18582d5 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
@@ -2,6 +2,7 @@
 package com.yahoo.container.core.config;
 
 import com.yahoo.config.FileReference;
+import com.yahoo.container.di.Osgi.GenerationStatus;
 import com.yahoo.osgi.Osgi;
 import org.osgi.framework.Bundle;
 
@@ -37,9 +38,12 @@ public class ApplicationBundleLoader {
     // The bundles that exclusively belong to the current application generation.
     private Map<FileReference, Bundle> bundlesFromNewGeneration = Map.of();
 
+    private boolean readyForNewBundles = true;
+
     private final Osgi osgi;
     private final FileAcquirerBundleInstaller bundleInstaller;
 
+
     public ApplicationBundleLoader(Osgi osgi, FileAcquirerBundleInstaller bundleInstaller) {
         this.osgi = osgi;
         this.bundleInstaller = bundleInstaller;
@@ -48,8 +52,12 @@ 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()
      */
     public synchronized Set<Bundle> 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());
@@ -61,15 +69,42 @@ public class ApplicationBundleLoader {
         BundleStarter.startBundles(activeBundles.values());
         log.info(installedBundlesMessage());
 
+        readyForNewBundles = false;
+
         return bundlesToUninstall;
     }
 
+    public synchronized Collection<Bundle> completeGeneration(GenerationStatus status) {
+        Collection<Bundle> ret = List.of();
+        if (readyForNewBundles) return ret;
+
+        if (status == GenerationStatus.SUCCESS) {
+            commitBundles();
+        } else {
+            ret = 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()}.
+     */
+    private void commitBundles() {
+        bundlesFromNewGeneration = Map.of();
+        obsoleteBundles = Map.of();
+        readyForNewBundles = true;
+    }
+
     /**
      * 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() {
+    private Collection<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();
@@ -83,6 +118,7 @@ public class ApplicationBundleLoader {
         bundlesFromNewGeneration = Map.of();
         obsoleteBundles = Map.of();
 
+        readyForNewBundles = true;
         return ret;
     }
 
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 8b720f0d48f..f573de1f4ac 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
@@ -104,9 +104,8 @@ public class HandlersConfigurerDi {
         }
 
         @Override
-        public Collection<Bundle> revertApplicationBundles() {
-            log.info("Reverting to bundles from the previous generation.");
-            return applicationBundleLoader.revertToPreviousGeneration();
+        public Collection<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 b1bf45290df..b4a22566eef 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
@@ -70,20 +70,26 @@ public class Container {
     // 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 {
+            ComponentGraph newGraph;
             Collection<Bundle> obsoleteBundles = new HashSet<>();
-            ComponentGraph newGraph = waitForNewConfigGenAndCreateGraph(oldGraph, fallbackInjector, isInitializing, obsoleteBundles);
-            newGraph.reuseNodes(oldGraph);
+            try {
+                newGraph = waitForNewConfigGenAndCreateGraph(oldGraph, fallbackInjector, isInitializing, obsoleteBundles);
+                newGraph.reuseNodes(oldGraph);
+            } catch (Throwable t) {
+                log.warning("Failed to set up component graph - uninstalling latest bundles. Bootstrap generation: " + getBootstrapGeneration());
+                osgi.completeBundleGeneration(Osgi.GenerationStatus.FAILURE);
+                throw t;
+            }
             try {
                 constructComponents(newGraph);
             } catch (Throwable e) {
-                log.log(Level.WARNING, String.format(
-                        "Failed to construct graph for generation '%d' - scheduling partial graph for deconstruction",
-                        newGraph.generation()), e);
-
-                Collection<Bundle> newBundlesFromFailedGen = osgi.revertApplicationBundles();
+                log.warning("Failed to construct components for generation '" + newGraph.generation() + "' - scheduling partial graph for deconstruction");
+                Collection<Bundle> newBundlesFromFailedGen = osgi.completeBundleGeneration(Osgi.GenerationStatus.FAILURE);
                 deconstructFailedGraph(oldGraph, newGraph, newBundlesFromFailedGen);
                 throw e;
             }
+            // TODO: take obsoleteBundles as return value here!
+            osgi.completeBundleGeneration(Osgi.GenerationStatus.SUCCESS);
             Runnable cleanupTask = createPreviousGraphDeconstructionTask(oldGraph, newGraph, obsoleteBundles);
             return new ComponentGraphResult(newGraph, cleanupTask);
         } catch (Throwable t) {
@@ -194,7 +200,7 @@ public class Container {
 
     private Set<Bundle> installApplicationBundles(Map<ConfigKey<? extends ConfigInstance>, ConfigInstance> configsIncludingBootstrapConfigs) {
         ApplicationBundlesConfig applicationBundlesConfig = getConfig(applicationBundlesConfigKey, configsIncludingBootstrapConfigs);
-        return osgi.useApplicationBundles(applicationBundlesConfig.bundles(), getComponentsGeneration());
+        return 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 73d39c50f02..c13ebca6f62 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
@@ -24,10 +24,14 @@ import static java.util.Collections.emptySet;
  */
 public interface Osgi {
 
+    enum GenerationStatus { SUCCESS, FAILURE }
+
     default void installPlatformBundles(Collection<String> bundlePaths) {
     }
 
     /**
+     * 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.
      *
@@ -40,13 +44,14 @@ public interface Osgi {
     }
 
     /**
-     * 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.
+     * 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.
      *
-     * Returns the set of bundles that was exclusively used by the new generation,
-     * and therefore should be scheduled for uninstalling.
+     * @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.
      */
-    default Collection<Bundle> revertApplicationBundles() {
+    default Collection<Bundle> completeBundleGeneration(GenerationStatus status) {
         return emptySet();
     }
 
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 7f223b0f5a0..fdd44fe358d 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,6 +1,7 @@
 // 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.container.di.Osgi.GenerationStatus;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.osgi.framework.Bundle;
@@ -15,6 +16,7 @@ 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.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -47,8 +49,28 @@ public class ApplicationBundleLoaderTest {
     }
 
     @Test
+    void generation_must_be_marked_complete_before_using_new_bundles() {
+        bundleLoader.useBundles(List.of(BUNDLE_1_REF));
+        assertThrows(IllegalStateException.class,
+                     () -> bundleLoader.useBundles(List.of(BUNDLE_1_REF)));
+    }
+
+    @Test
+    void no_bundles_are_marked_obsolete_upon_reconfig_with_unchanged_bundles() {
+        bundleLoader.useBundles(List.of(BUNDLE_1_REF));
+        bundleLoader.completeGeneration(GenerationStatus.SUCCESS);
+
+        Set<Bundle> obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_1_REF, BUNDLE_2_REF));
+
+        // No bundles are obsolete
+        assertTrue(obsoleteBundles.isEmpty());
+    }
+
+    @Test
     void new_bundle_can_be_installed_in_reconfig() {
         bundleLoader.useBundles(List.of(BUNDLE_1_REF));
+        bundleLoader.completeGeneration(GenerationStatus.SUCCESS);
+
         Set<Bundle> obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_1_REF, BUNDLE_2_REF));
 
         // No bundles are obsolete
@@ -73,6 +95,8 @@ public class ApplicationBundleLoaderTest {
     @Test
     void unused_bundle_is_marked_obsolete_after_reconfig() {
         bundleLoader.useBundles(List.of(BUNDLE_1_REF));
+        bundleLoader.completeGeneration(GenerationStatus.SUCCESS);
+
         Set<Bundle> obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_2_REF));
 
         // The returned set of obsolete bundles contains bundle-1
@@ -97,11 +121,13 @@ public class ApplicationBundleLoaderTest {
     @Test
     void previous_generation_can_be_restored_after_a_failed_reconfig() {
         bundleLoader.useBundles(List.of(BUNDLE_1_REF));
+        bundleLoader.completeGeneration(GenerationStatus.SUCCESS);
+
         Set<Bundle> obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_2_REF));
         assertEquals(BUNDLE_1.getSymbolicName(), obsoleteBundles.iterator().next().getSymbolicName());
 
         // Revert to the previous generation, as will be done upon a failed reconfig.
-        Collection<Bundle> bundlesToUninstall = bundleLoader.revertToPreviousGeneration();
+        Collection<Bundle> bundlesToUninstall = bundleLoader.completeGeneration(GenerationStatus.FAILURE);
 
         assertEquals(1, bundlesToUninstall.size());
         assertEquals(BUNDLE_2.getSymbolicName(), bundlesToUninstall.iterator().next().getSymbolicName());
@@ -117,18 +143,17 @@ public class ApplicationBundleLoaderTest {
     }
 
     @Test
-    void bundles_are_unaffected_by_failed_reconfig_with_unchanged_bundles() {
+    void bundles_from_committed_config_generation_are_not_uninstalled_upon_future_failed_reconfig() {
         bundleLoader.useBundles(List.of(BUNDLE_1_REF));
-        Set<Bundle> obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_1_REF));
-        assertTrue(obsoleteBundles.isEmpty());
+        bundleLoader.completeGeneration(GenerationStatus.SUCCESS);
 
         // Revert to the previous generation, as will be done upon a failed reconfig.
-        Collection<Bundle> bundlesToUninstall = bundleLoader.revertToPreviousGeneration();
+        Collection<Bundle> bundlesToUninstall = bundleLoader.completeGeneration(GenerationStatus.FAILURE);
 
         assertEquals(0, bundlesToUninstall.size());
         assertEquals(1, osgi.getCurrentBundles().size());
 
-        obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_1_REF));
+        Set<Bundle> obsoleteBundles = bundleLoader.useBundles(List.of(BUNDLE_1_REF));
         assertTrue(obsoleteBundles.isEmpty());
         assertEquals(1, osgi.getCurrentBundles().size());
     }
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 b48aae2e300..d634d7b7ce2 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
@@ -70,8 +70,8 @@ public class TestOsgi extends MockOsgi implements com.yahoo.container.di.Osgi {
     }
 
     @Override
-    public Collection<Bundle> revertApplicationBundles() {
-        return bundleLoader.revertToPreviousGeneration();
+    public Collection<Bundle> completeBundleGeneration(GenerationStatus status) {
+        return bundleLoader.completeGeneration(status);
     }
 
     public void removeBundle(Bundle 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 23649f8de72..5fbb035136f 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
@@ -10,13 +10,13 @@ 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.List;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -252,7 +252,7 @@ public class ContainerTest extends ContainerTestBase {
         try {
             newGraph.get(1, TimeUnit.SECONDS);
             fail("Expected waiting for new config.");
-        } catch (Exception ignored) {
+        } catch (TimeoutException ignored) {
             // expect to time out
         }