diff options
11 files changed, 124 insertions, 54 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..972d6677e3b 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,76 @@ 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(); + + // No duplicate bundles should be allowed until the next call to useBundles. + osgi.allowDuplicateBundles(Set.of()); + + // 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 +130,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 dbf2ba4a9a8..e45a5bf2ac5 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 @@ -72,6 +72,7 @@ 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 { Collection<Bundle> obsoleteBundles = new HashSet<>(); @@ -83,7 +84,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); @@ -168,7 +171,7 @@ public class Container { return componentGraph; } - 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 +179,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, 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..04ed52db53b 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 @@ -9,6 +9,7 @@ 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; @@ -70,7 +71,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,6 +101,27 @@ public class ApplicationBundleLoaderTest { } + @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()); + + // Revert to the previous generation, as will be done upon a failed reconfig. + Collection<Bundle> bundlesToUninstall = bundleLoader.revertToPreviousGeneration(); + + assertEquals(1, bundlesToUninstall.size()); + assertEquals(BUNDLE_2.getSymbolicName(), bundlesToUninstall.iterator().next().getSymbolicName()); + + // Both bundles are still current, until the bundle from the failed gen will be uninstalled. + // Uninstalling is handled by the Deconstructor, not included in this test setup. + assertEquals(2, osgi.getCurrentBundles().size()); + + // 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)); + } + private static Map<String, Bundle> testBundles() { return Map.of(BUNDLE_1_REF.value(), BUNDLE_1, BUNDLE_2_REF.value(), BUNDLE_2); 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..cf3d1cef5f8 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 @@ -51,6 +51,7 @@ class TestOsgi extends MockOsgi { @Override public void allowDuplicateBundles(Collection<Bundle> bundles) { + allowedDuplicates.clear(); allowedDuplicates.addAll(bundles); } 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..7aa5d7b1ce7 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 @@ -66,6 +66,11 @@ public class ContainerTestBase { } @Override + public Collection<Bundle> revertApplicationBundles() { + return emptySet(); + } + + @Override public Bundle getBundle(ComponentSpecification spec) { throw new UnsupportedOperationException("getBundle not supported."); } diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java b/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java index 438ad8d8ebe..f8c158ab178 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/application/OsgiFramework.java @@ -94,7 +94,7 @@ public interface OsgiFramework { * Allows this framework to install duplicates of the given collection of bundles. Duplicate detection * is handled by the {@link com.yahoo.jdisc.core.BundleCollisionHook}. * - * @param bundles The bundles to allow duplicates of + * @param bundles The bundles to allow duplicates of. An empty collection will prohibit any duplicates. */ void allowDuplicateBundles(Collection<Bundle> bundles); diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java index 2f90b4e067f..3212bb4e6de 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/BundleCollisionHook.java @@ -34,7 +34,7 @@ public class BundleCollisionHook implements CollisionHook, EventHook, FindHook { private static final Logger log = Logger.getLogger(BundleCollisionHook.class.getName()); private ServiceRegistration<?> registration; - private Map<Bundle, BsnVersion> allowedDuplicates = new HashMap<>(5); + private final Map<Bundle, BsnVersion> allowedDuplicates = new HashMap<>(5); public void start(BundleContext context) { if (registration != null) { @@ -50,11 +50,10 @@ public class BundleCollisionHook implements CollisionHook, EventHook, FindHook { } /** - * Adds a collection of bundles to the allowed duplicates. - * Also clears any previous allowed duplicates of the new allowed duplicates. + * Sets a collection of bundles to allow duplicates for. */ synchronized void allowDuplicateBundles(Collection<Bundle> bundles) { - allowedDuplicates.values().removeAll(bundles.stream().map(BsnVersion::new).collect(Collectors.toSet())); + allowedDuplicates.clear(); for (var bundle : bundles) { allowedDuplicates.put(bundle, new BsnVersion(bundle)); } diff --git a/jdisc_core_test/integration_test/src/test/java/com/yahoo/jdisc/core/BundleCollisionHookIntegrationTest.java b/jdisc_core_test/integration_test/src/test/java/com/yahoo/jdisc/core/BundleCollisionHookIntegrationTest.java index 07797838a4b..713f0397984 100644 --- a/jdisc_core_test/integration_test/src/test/java/com/yahoo/jdisc/core/BundleCollisionHookIntegrationTest.java +++ b/jdisc_core_test/integration_test/src/test/java/com/yahoo/jdisc/core/BundleCollisionHookIntegrationTest.java @@ -81,14 +81,18 @@ public class BundleCollisionHookIntegrationTest { } @Test - public void duplicates_whitelist_can_be_updated_with_additional_bundles() throws Exception { + public void duplicates_whitelist_is_reset_upon_each_call() throws Exception { Bundle bundleL = startBundle(felix, "cert-l1.jar"); Bundle bundleMl = startBundle(felix, "cert-ml.jar"); felix.allowDuplicateBundles(Collections.singleton(bundleL)); felix.allowDuplicateBundles(Collections.singleton(bundleMl)); - startBundle(felix, "cert-l1-dup.jar"); - startBundle(felix, "cert-ml-dup.jar"); + try { + startBundle(felix, "cert-l1-dup.jar"); + fail("Expected exception due to duplicate bundles"); + } catch (BundleException e) { + assertTrue(e.getMessage().contains("Bundle symbolic name and version are not unique")); + } } @Test @@ -97,8 +101,7 @@ public class BundleCollisionHookIntegrationTest { Bundle bundleB = startBundle(felix, "cert-b.jar"); // Makes A and B visible to each other - felix.allowDuplicateBundles(Set.of(bundleA)); - felix.allowDuplicateBundles(Set.of(bundleB)); + felix.allowDuplicateBundles(Set.of(bundleA, bundleB)); List<Bundle> visibleBundles = felix.getBundles(bundleA); @@ -117,8 +120,7 @@ public class BundleCollisionHookIntegrationTest { Bundle bundleB = startBundle(felix, "cert-b.jar"); // Makes A and B visible to each other - felix.allowDuplicateBundles(Set.of(bundleA)); - felix.allowDuplicateBundles(Set.of(bundleB)); + felix.allowDuplicateBundles(Set.of(bundleA, bundleB)); List<Bundle> visibleBundles = felix.getBundles(bundleA); assertEquals(3, visibleBundles.size()); |