diff options
author | gjoranv <gv@verizonmedia.com> | 2022-08-31 14:11:38 +0200 |
---|---|---|
committer | gjoranv <gv@verizonmedia.com> | 2022-08-31 14:11:38 +0200 |
commit | f869fa7fc3fbce76bc19745201bb62484dfc8efe (patch) | |
tree | b8d8e622116ef172e43395f6c9671504f2a740d4 | |
parent | 60e7b087648bc3c5fa4c4a06ca75f740e176f9d5 (diff) |
Revert "Clean up bundles and allowed duplicates after a failed reconfig."
This reverts commit b0a398eaeadfaf12e31bcfef2e41892439db1149.
11 files changed, 54 insertions, 124 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 972d6677e3b..f0ca5aee8a2 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,14 +6,13 @@ 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. @@ -25,15 +24,16 @@ public class ApplicationBundleLoader { private static final Logger log = Logger.getLogger(ApplicationBundleLoader.class.getName()); - // The active bundles for the current application generation. + /** + * 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 + */ 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,76 +48,54 @@ public class ApplicationBundleLoader { */ public synchronized Set<Bundle> useBundles(List<FileReference> newFileReferences) { - obsoleteBundles = removeObsoleteBundles(newFileReferences); - Set<Bundle> bundlesToUninstall = new LinkedHashSet<>(obsoleteBundles.values()); + Set<FileReference> obsoleteReferences = getObsoleteFileReferences(newFileReferences); + Set<Bundle> bundlesToUninstall = getObsoleteBundles(obsoleteReferences); log.info("Bundles to schedule for uninstall: " + bundlesToUninstall); osgi.allowDuplicateBundles(bundlesToUninstall); + removeInactiveFileReferences(obsoleteReferences); - bundlesFromNewGeneration = installBundles(newFileReferences); + installBundles(newFileReferences); BundleStarter.startBundles(reference2Bundle.values()); log.info(installedBundlesMessage()); return bundlesToUninstall; } - /** - * 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; + private Set<FileReference> getObsoleteFileReferences(List<FileReference> newReferences) { + Set<FileReference> obsoleteReferences = new HashSet<>(reference2Bundle.keySet()); + obsoleteReferences.removeAll(newReferences); + return obsoleteReferences; } /** - * 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. + * Returns the bundles that will not be retained by the new application generation. */ - private Map<FileReference, Bundle> removeObsoleteBundles(List<FileReference> newReferences) { - Map<FileReference, Bundle> obsoleteReferences = new LinkedHashMap<>(reference2Bundle); - newReferences.forEach(obsoleteReferences::remove); + private Set<Bundle> getObsoleteBundles(Set<FileReference> obsoleteReferences) { + return obsoleteReferences.stream().map(reference2Bundle::get).collect(Collectors.toSet()); + } - obsoleteReferences.forEach(reference2Bundle::remove); - return obsoleteReferences; + private void removeInactiveFileReferences(Set<FileReference> fileReferencesToRemove) { + fileReferencesToRemove.forEach(reference2Bundle::remove); } - /** - * Returns the set of new bundles that were installed. - */ - private Map<FileReference, Bundle> installBundles(List<FileReference> references) { + private void 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()) 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(); + if (!bundlesToInstall.isEmpty()) { + if (bundleInstaller.hasFileDistribution()) { + installWithFileDistribution(bundlesToInstall, bundleInstaller); + } else { + log.warning("Can't retrieve bundles since file distribution is disabled."); + } } } - private Map<FileReference, Bundle> installWithFileDistribution(Set<FileReference> bundlesToInstall, - FileAcquirerBundleInstaller bundleInstaller) { - var newBundles = new LinkedHashMap<FileReference, Bundle>(); - + private void installWithFileDistribution(Set<FileReference> bundlesToInstall, + FileAcquirerBundleInstaller bundleInstaller) { for (FileReference reference : bundlesToInstall) { try { log.info("Installing bundle with reference '" + reference.value() + "'"); @@ -130,13 +108,11 @@ 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 3fd8b8dc6a9..363ff26eea9 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,15 +99,9 @@ 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 e45a5bf2ac5..dbf2ba4a9a8 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,7 +72,6 @@ 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<>(); @@ -84,9 +83,7 @@ public class Container { 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(); - deconstructFailedGraph(oldGraph, newGraph, newBundlesFromFailedGen); + deconstructFailedGraph(oldGraph, newGraph); throw e; } Runnable cleanupTask = createPreviousGraphDeconstructionTask(oldGraph, newGraph, obsoleteBundles); @@ -171,7 +168,7 @@ public class Container { return componentGraph; } - private void deconstructFailedGraph(ComponentGraph currentGraph, ComponentGraph failedGraph, Collection<Bundle> bundlesFromFailedGraph) { + private void deconstructFailedGraph(ComponentGraph currentGraph, ComponentGraph failedGraph) { Set<Object> currentComponents = Collections.newSetFromMap(new IdentityHashMap<>(currentGraph.size())); currentComponents.addAll(currentGraph.allConstructedComponentsAndProviders()); @@ -179,7 +176,7 @@ public class Container { for (Object component : failedGraph.allConstructedComponentsAndProviders()) { if (!currentComponents.contains(component)) unusedComponents.add(component); } - destructor.deconstruct(failedGraph.generation(), unusedComponents, bundlesFromFailedGraph); + destructor.deconstruct(failedGraph.generation(), unusedComponents, List.of()); } 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 02f42fdb9b8..a3ebc909a6e 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,6 +9,7 @@ import org.osgi.framework.Bundle; import java.util.Collection; import java.util.Set; +import java.util.stream.Collectors; import static java.util.Collections.emptySet; @@ -16,8 +17,6 @@ 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 @@ -25,6 +24,7 @@ import static java.util.Collections.emptySet; public interface Osgi { default void installPlatformBundles(Collection<String> bundlePaths) { + //System.out.println("installPlatformBundles " + bundlePaths); } /** @@ -32,21 +32,12 @@ public interface Osgi { * and therefore should be scheduled for uninstalling. */ default Set<Bundle> useApplicationBundles(Collection<FileReference> bundles) { - 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() { + //System.out.println("useBundles " + bundles.stream().map(Object::toString).collect(Collectors.joining(", "))); return emptySet(); } default Class<?> resolveClass(BundleInstantiationSpecification spec) { + //System.out.println("resolving class " + spec.classId); try { return Class.forName(spec.classId.getName()); } catch (ClassNotFoundException e) { @@ -55,6 +46,7 @@ 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 10cd41cce08..c8fa901dd0a 100644 --- a/container-core/src/main/java/com/yahoo/osgi/Osgi.java +++ b/container-core/src/main/java/com/yahoo/osgi/Osgi.java @@ -22,7 +22,6 @@ 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 04ed52db53b..cf8cc6053cd 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,7 +9,6 @@ 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; @@ -71,6 +70,7 @@ 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,27 +101,6 @@ 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 cf3d1cef5f8..2e22af889ed 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,7 +51,6 @@ 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 7aa5d7b1ce7..4383bd08f71 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,11 +66,6 @@ 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 f8c158ab178..438ad8d8ebe 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. An empty collection will prohibit any duplicates. + * @param bundles The bundles to allow duplicates of */ 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 3212bb4e6de..2f90b4e067f 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 final Map<Bundle, BsnVersion> allowedDuplicates = new HashMap<>(5); + private Map<Bundle, BsnVersion> allowedDuplicates = new HashMap<>(5); public void start(BundleContext context) { if (registration != null) { @@ -50,10 +50,11 @@ public class BundleCollisionHook implements CollisionHook, EventHook, FindHook { } /** - * Sets a collection of bundles to allow duplicates for. + * Adds a collection of bundles to the allowed duplicates. + * Also clears any previous allowed duplicates of the new allowed duplicates. */ synchronized void allowDuplicateBundles(Collection<Bundle> bundles) { - allowedDuplicates.clear(); + allowedDuplicates.values().removeAll(bundles.stream().map(BsnVersion::new).collect(Collectors.toSet())); 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 713f0397984..07797838a4b 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,18 +81,14 @@ public class BundleCollisionHookIntegrationTest { } @Test - public void duplicates_whitelist_is_reset_upon_each_call() throws Exception { + public void duplicates_whitelist_can_be_updated_with_additional_bundles() 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)); - 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")); - } + startBundle(felix, "cert-l1-dup.jar"); + startBundle(felix, "cert-ml-dup.jar"); } @Test @@ -101,7 +97,8 @@ public class BundleCollisionHookIntegrationTest { Bundle bundleB = startBundle(felix, "cert-b.jar"); // Makes A and B visible to each other - felix.allowDuplicateBundles(Set.of(bundleA, bundleB)); + felix.allowDuplicateBundles(Set.of(bundleA)); + felix.allowDuplicateBundles(Set.of(bundleB)); List<Bundle> visibleBundles = felix.getBundles(bundleA); @@ -120,7 +117,8 @@ public class BundleCollisionHookIntegrationTest { Bundle bundleB = startBundle(felix, "cert-b.jar"); // Makes A and B visible to each other - felix.allowDuplicateBundles(Set.of(bundleA, bundleB)); + felix.allowDuplicateBundles(Set.of(bundleA)); + felix.allowDuplicateBundles(Set.of(bundleB)); List<Bundle> visibleBundles = felix.getBundles(bundleA); assertEquals(3, visibleBundles.size()); |