From 277cb22aa6b8553bb5369dfd4d1d5eba2185a858 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 21 Sep 2017 11:35:25 +0100 Subject: [PATCH 1/6] fix bundle install issue if already osgi-installed, and tidy take the URL from the existing osgi-installed bundle. much better logging. --- .../core/catalog/internal/CatalogUtils.java | 4 +- .../core/mgmt/ha/OsgiArchiveInstaller.java | 97 +++++++++++++------ 2 files changed, 67 insertions(+), 34 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java index 973f56da10..471ce6497f 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java @@ -215,8 +215,8 @@ public static void installLibraries(ManagementContext managementContext, @Nullab } if (log.isDebugEnabled()) { logDebugOrTraceIfRebinding(log, - "Registered {} bundles in {}", - new Object[]{libraries.size(), Time.makeTimeStringRounded(timer)}); + "Registered {} bundle{} in {}", + new Object[]{libraries.size(), Strings.s(libraries.size()), Time.makeTimeStringRounded(timer)}); } } } diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java index 15430e592d..f00b96f8e6 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java @@ -129,52 +129,85 @@ private synchronized void init() { } private synchronized void makeLocalZipFileFromInputStreamOrUrl() { + Maybe existingOsgiInstalledBundle = Maybe.absent(); + Maybe existingBrooklynInstalledBundle = Maybe.absent(); if (zipIn==null) { - Maybe installedBundle = Maybe.absent(); if (suppliedKnownBundleMetadata!=null) { - // if no input stream, look for a URL and/or a matching bundle + // if no input stream (zipIn), look for a URL and/or a matching bundle if (!suppliedKnownBundleMetadata.isNameResolved()) { - ManagedBundle mbFromUrl = osgiManager.getManagedBundleFromUrl(suppliedKnownBundleMetadata.getUrl()); - if (mbFromUrl!=null) { + existingBrooklynInstalledBundle = Maybe.ofDisallowingNull(osgiManager.getManagedBundleFromUrl(suppliedKnownBundleMetadata.getUrl())); + if (existingBrooklynInstalledBundle.isPresent()) { // user supplied just a URL (eg brooklyn.libraries), but we recognise it, // so don't try to reload it, just record the info we know about it to retrieve the bundle - ((BasicManagedBundle)suppliedKnownBundleMetadata).setSymbolicName(mbFromUrl.getSymbolicName()); - ((BasicManagedBundle)suppliedKnownBundleMetadata).setVersion(mbFromUrl.getSuppliedVersionString()); + ((BasicManagedBundle)suppliedKnownBundleMetadata).setSymbolicName(existingBrooklynInstalledBundle.get().getSymbolicName()); + ((BasicManagedBundle)suppliedKnownBundleMetadata).setVersion(existingBrooklynInstalledBundle.get().getSuppliedVersionString()); } } - if (installedBundle.isAbsent() && suppliedKnownBundleMetadata.getOsgiUniqueUrl()!=null) { - installedBundle = Osgis.bundleFinder(osgiManager.framework).requiringFromUrl(suppliedKnownBundleMetadata.getOsgiUniqueUrl()).find(); + if (existingOsgiInstalledBundle.isAbsent() && suppliedKnownBundleMetadata.getOsgiUniqueUrl()!=null) { + existingOsgiInstalledBundle = Osgis.bundleFinder(osgiManager.framework).requiringFromUrl(suppliedKnownBundleMetadata.getOsgiUniqueUrl()).find(); } - if (installedBundle.isAbsent() && suppliedKnownBundleMetadata.getUrl()!=null) { - installedBundle = Osgis.bundleFinder(osgiManager.framework).requiringFromUrl(suppliedKnownBundleMetadata.getUrl()).find(); + if (existingOsgiInstalledBundle.isAbsent() && suppliedKnownBundleMetadata.getUrl()!=null) { + existingOsgiInstalledBundle = Osgis.bundleFinder(osgiManager.framework).requiringFromUrl(suppliedKnownBundleMetadata.getUrl()).find(); } - if (installedBundle.isAbsent() && suppliedKnownBundleMetadata.isNameResolved()) { - installedBundle = Osgis.bundleFinder(osgiManager.framework).symbolicName(suppliedKnownBundleMetadata.getSymbolicName()).version(suppliedKnownBundleMetadata.getSuppliedVersionString()).find(); + if (existingOsgiInstalledBundle.isAbsent() && suppliedKnownBundleMetadata.isNameResolved()) { + existingOsgiInstalledBundle = Osgis.bundleFinder(osgiManager.framework).symbolicName(suppliedKnownBundleMetadata.getSymbolicName()).version(suppliedKnownBundleMetadata.getSuppliedVersionString()).find(); } - if (suppliedKnownBundleMetadata.getUrl()!=null) { - if (installedBundle.isAbsent() || force) { - // reload - zipIn = ResourceUtils.create(mgmt()).getResourceFromUrl(suppliedKnownBundleMetadata.getUrl()); + if (existingOsgiInstalledBundle.isPresent()) { + if (existingBrooklynInstalledBundle.isAbsent()) { + // try to find as brooklyn bundle based on knowledge of OSGi bundle + existingBrooklynInstalledBundle = Maybe.ofDisallowingNull(osgiManager.getManagedBundle(new VersionedName(existingOsgiInstalledBundle.get()))); } - } - } - - if (zipIn==null) { - if (installedBundle.isPresent()) { - // no way to install (no url), or no need to install (not forced); just ignore it - result.metadata = osgiManager.getManagedBundle(new VersionedName(installedBundle.get())); - if (result.metadata==null) { - // low-level installed bundle - result.metadata = new BasicManagedBundle(installedBundle.get().getSymbolicName(), installedBundle.get().getVersion().toString(), - suppliedKnownBundleMetadata!=null ? suppliedKnownBundleMetadata.getUrl() : null); + if (suppliedKnownBundleMetadata.getUrl()==null) { + // installer did not supply a usable URL, just coords + // but bundle is installed at least to OSGi + if (existingBrooklynInstalledBundle.isPresent()) { + log.debug("Detected bundle "+suppliedKnownBundleMetadata+" installed to Brooklyn already; no URL or stream supplied, so re-using existing installation"); + // if bundle is brooklyn-managed simply say "already installed" + result.metadata = existingBrooklynInstalledBundle.get(); + result.setIgnoringAlreadyInstalled(); + return; + + } else { + // if bundle is not brooklyn-managed we want to make it be so + // and for that we need to find a URL. + // some things declare usable locations, though these might be maven and + String candidateUrl = existingOsgiInstalledBundle.get().getLocation(); + log.debug("Detected bundle "+suppliedKnownBundleMetadata+" installed to OSGi but not Brooklyn; trying to find a URL to get bundle binary, candidate "+candidateUrl); + if (Strings.isBlank(candidateUrl)) { + throw new IllegalArgumentException("No input stream available and no URL could be found: no way to promote "+suppliedKnownBundleMetadata+" from "+existingOsgiInstalledBundle.get()+" to Brooklyn management"); + } + try { + // do this in special try block, not below, so we can give a better error + // (the user won't understnad the URL) + zipIn = ResourceUtils.create(mgmt()).getResourceFromUrl(candidateUrl); + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + throw new IllegalArgumentException("Could not find binary for already installed OSGi bundle "+existingOsgiInstalledBundle.get()+" (location "+candidateUrl+") when trying to promote "+suppliedKnownBundleMetadata+" to Brooklyn management", e); + } + } } + } else if (suppliedKnownBundleMetadata.getUrl()==null) { + // not installed anywhere and no URL + throw new IllegalArgumentException("No input stream available and no URL could be found: no way to install "+suppliedKnownBundleMetadata); + } + + assert zipIn!=null || suppliedKnownBundleMetadata.getUrl()!=null : "should have found a stream or inferred a URL"; + + if (zipIn!=null) { + // found input stream for existing osgi bundle + } else if (existingBrooklynInstalledBundle.isAbsent() || force) { + // reload + zipIn = ResourceUtils.create(mgmt()).getResourceFromUrl(suppliedKnownBundleMetadata.getUrl()); + } else { + // already installed, not forced, just say already installed + // (even if snapshot as this is a reference by URL, not uploaded content) + result.metadata = existingBrooklynInstalledBundle.get(); result.setIgnoringAlreadyInstalled(); return; } - result.metadata = suppliedKnownBundleMetadata; - throw new IllegalArgumentException("No input stream available and no URL could be found; nothing to install"); } - result.bundle = installedBundle.orNull(); + + result.bundle = existingOsgiInstalledBundle.orNull(); } zipFile = Os.newTempFile("brooklyn-bundle-transient-"+suppliedKnownBundleMetadata, "zip"); @@ -372,7 +405,7 @@ public ReferenceWithError install() { ((BasicManagedBundle)result.getMetadata()).setPersistenceNeeded(true); mgmt().getRebindManager().getChangeListener().onChanged(result.getMetadata()); } - log.debug(result.message + " (in osgi container)"); + log.debug(result.message + " (partial): OSGi bundle installed, with bundle start and Brooklyn management to follow"); // can now delete and close (copy has been made and is available from OsgiManager) zipFile.delete(); zipFile = null; @@ -511,7 +544,7 @@ public void run() { log.debug(result.message+", all items: "+result.typesInstalled); } } else { - log.debug(result.message+" (into Brooklyn), with no catalog items"); + log.debug(result.message+" (complete): bundle started and now managed by Brooklyn, though no catalog items found (may have installed other bundles though)"); } } From 3816fb7d7b5e167fd74bb156f26f1c8450136c1a Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 21 Sep 2017 13:06:21 +0100 Subject: [PATCH 2/6] add Streams.compare function --- .../apache/brooklyn/util/stream/Streams.java | 39 ++++++++++++++++++- .../brooklyn/util/stream/StreamsTest.java | 17 ++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/stream/Streams.java b/utils/common/src/main/java/org/apache/brooklyn/util/stream/Streams.java index ccfa1cce27..907edfe6df 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/stream/Streams.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/stream/Streams.java @@ -29,6 +29,7 @@ import java.io.Reader; import java.io.StringReader; import java.io.Writer; +import java.nio.ByteBuffer; import java.nio.charset.Charset; import java.security.DigestInputStream; import java.security.MessageDigest; @@ -104,6 +105,18 @@ public static byte[] readFully(InputStream is) { } } + /** reads the input stream fully into the given byte buffer or until the supplied buffer is full, + * returning the number of bytes read */ + public static int readFully(InputStream s, byte[] buf) throws IOException { + int count = 0; + while (count < buf.length) { + int countHere = s.read(buf, count, buf.length-count); + if (countHere<=0) return count; + count += countHere; + } + return count; + } + public static byte[] readFullyAndClose(InputStream is) { try { return readFully(is); @@ -234,5 +247,29 @@ public static String getMd5Checksum(InputStream in) { } return result.toString().toUpperCase(); } - + + /** True iff the input streams read to completion give identical contents. Streams are closed afterwards. */ + public static boolean compare(InputStream s1, InputStream s2) throws IOException { + try { + if (s1==null || s2==null) { + return s1==null && s2==null; + } + byte[] buf1 = new byte[4096], buf2 = new byte[4096]; + while (true) { + int r1 = readFully(s1, buf1); + int r2 = readFully(s2, buf2); + // do this in case HeapByteBuffer.equals has an efficient intrinsic comparison; + // curiously there is no efficient array-compare a la System.arraycopy; + // the only thing I've found is that Arrays.equals(char[], char[]) is optimized/intrinsic, + // and allegedly 8 times faster than all other array comparison routines! + // https://stackoverflow.com/questions/41153992/why-is-arrays-equalschar-char-8-times-faster-than-all-the-other-versions + if (!ByteBuffer.wrap(buf1,0,r1).equals(ByteBuffer.wrap(buf2,0,r2))) return false; + if (r1<4096) return true; + } + } finally { + closeQuietly(s1); + closeQuietly(s2); + } + } + } diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/stream/StreamsTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/stream/StreamsTest.java index 37340f47bd..cb480ce7d4 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/stream/StreamsTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/stream/StreamsTest.java @@ -19,7 +19,9 @@ package org.apache.brooklyn.util.stream; import java.io.ByteArrayInputStream; +import java.io.IOException; +import org.apache.brooklyn.util.text.Strings; import org.testng.Assert; import org.testng.annotations.Test; @@ -33,4 +35,19 @@ public void testChecksum() { ); } + + @Test + public void testComparison() throws IOException { + Assert.assertTrue(Streams.compare(Streams.newInputStreamWithContents("hello"), Streams.newInputStreamWithContents("hello"))); + Assert.assertFalse(Streams.compare(Streams.newInputStreamWithContents("hello"), Streams.newInputStreamWithContents("hello2"))); + Assert.assertFalse(Streams.compare(Streams.newInputStreamWithContents("hello2"), Streams.newInputStreamWithContents("hello"))); + Assert.assertFalse(Streams.compare(null, Streams.newInputStreamWithContents("hello"))); + Assert.assertTrue(Streams.compare(null, null)); + // test across byte buffers + Assert.assertTrue(Streams.compare(Streams.newInputStreamWithContents(Strings.repeat("hello", 1000)), + Streams.newInputStreamWithContents(Strings.repeat("hello", 1000)))); + Assert.assertFalse(Streams.compare(Streams.newInputStreamWithContents(Strings.repeat("hello", 1000)+"1"), + Streams.newInputStreamWithContents(Strings.repeat("hello", 1000)+"2"))); + } + } From 69ea984467e79dc3554d0079a662648c5e9bd922 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 21 Sep 2017 13:07:07 +0100 Subject: [PATCH 3/6] handle edge cases where importing OSGi-already-managed bundles to Brooklyn mgmt --- .../internal/BasicBrooklynCatalog.java | 3 +- .../core/catalog/internal/CatalogUtils.java | 27 +++--- .../core/mgmt/ha/OsgiArchiveInstaller.java | 85 +++++++++++++++---- .../brooklyn/core/mgmt/ha/OsgiManager.java | 36 ++++---- 4 files changed, 109 insertions(+), 42 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index f42c0a2234..c358cef7f3 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -1782,7 +1782,8 @@ public void addItem(CatalogItem item) { //assume forceUpdate for backwards compatibility log.debug("Adding manual catalog item to "+mgmt+": "+item); checkNotNull(item, "item"); - CatalogUtils.installLibraries(mgmt, item.getLibraries()); + //don't activate bundles; only intended for legacy tests where that might not work + CatalogUtils.installLibraries(mgmt, item.getLibraries(), false); if (manualAdditionsCatalog==null) loadManualAdditionsCatalog(); manualAdditionsCatalog.addEntry(getAbstractCatalogItem(item)); } diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java index 471ce6497f..5621588194 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogUtils.java @@ -168,6 +168,11 @@ public static BrooklynClassLoadingContext newClassLoadingContextForCatalogItems( * Registers all bundles with the management context's OSGi framework. */ public static void installLibraries(ManagementContext managementContext, @Nullable Collection libraries) { + installLibraries(managementContext, libraries, true); + } + /** As {@link #installLibraries(ManagementContext, Collection)} but letting caller suppress the deferred start/install + * (for use in tests where bundles' BOMs aren't resolvable). */ + public static void installLibraries(ManagementContext managementContext, @Nullable Collection libraries, boolean startBundlesAndInstallToBrooklyn) { if (libraries == null) return; ManagementContextInternal mgmt = (ManagementContextInternal) managementContext; @@ -190,16 +195,18 @@ public static void installLibraries(ManagementContext managementContext, @Nullab results.add(result); } Map errors = MutableMap.of(); - for (OsgiBundleInstallationResult r: results) { - if (r.getDeferredStart()!=null) { - try { - r.getDeferredStart().run(); - } catch (Throwable t) { - Exceptions.propagateIfFatal(t); - // above will done rollback for the failed item, but we need consistent behaviour for all libraries; - // for simplicity we simply have each bundle either fully installed or fully rolled back - // (alternative would be to roll back everything) - errors.put(r.getVersionedName().toString(), t); + if (startBundlesAndInstallToBrooklyn) { + for (OsgiBundleInstallationResult r: results) { + if (r.getDeferredStart()!=null) { + try { + r.getDeferredStart().run(); + } catch (Throwable t) { + Exceptions.propagateIfFatal(t); + // above will done rollback for the failed item, but we need consistent behaviour for all libraries; + // for simplicity we simply have each bundle either fully installed or fully rolled back + // (alternative would be to roll back everything) + errors.put(r.getVersionedName().toString(), t); + } } } } diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java index f00b96f8e6..5f053e5a52 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java @@ -23,6 +23,7 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; +import java.net.URL; import java.util.Collections; import java.util.Map; import java.util.TreeMap; @@ -81,6 +82,7 @@ class OsgiArchiveInstaller { private boolean validateTypes = true; private File zipFile; + private boolean isBringingExistingOsgiInstalledBundleUnderBrooklynManagement = false; private Manifest discoveredManifest; private VersionedName discoveredBomVersionedName; OsgiBundleInstallationResult result; @@ -178,8 +180,9 @@ private synchronized void makeLocalZipFileFromInputStreamOrUrl() { } try { // do this in special try block, not below, so we can give a better error - // (the user won't understnad the URL) + // (the user won't understand the URL) zipIn = ResourceUtils.create(mgmt()).getResourceFromUrl(candidateUrl); + isBringingExistingOsgiInstalledBundleUnderBrooklynManagement = true; } catch (Exception e) { Exceptions.propagateIfFatal(e); throw new IllegalArgumentException("Could not find binary for already installed OSGi bundle "+existingOsgiInstalledBundle.get()+" (location "+candidateUrl+") when trying to promote "+suppliedKnownBundleMetadata+" to Brooklyn management", e); @@ -360,19 +363,61 @@ public ReferenceWithError install() { } } else { result.metadata = inferredMetadata; - // no such managed bundle + // no such Brooklyn-managed bundle Maybe b = Osgis.bundleFinder(osgiManager.framework).symbolicName(result.getMetadata().getSymbolicName()).version(result.getMetadata().getSuppliedVersionString()).find(); if (b.isPresent()) { // bundle already installed to OSGi subsystem but brooklyn not aware of it; - // this will often happen on a karaf restart so don't be too strict! - // in this case let's uninstall it to make sure we have the right bundle and checksum - // (in case where user has replaced a JAR file in persisted state, - // or where they osgi installed something and are now uploading it or something else) - // but let's just assume it's the same; worst case if not user will - // have to uninstall it then reinstall it to do the replacement - // (means you can't just replace a JAR in persisted state however) - log.debug("Brooklyn install of "+result.getMetadata().getVersionedName()+" detected already loaded in OSGi; uninstalling that to reinstall as Brooklyn-managed"); - b.get().uninstall(); + // this will often happen on a karaf restart where bundle was cached by karaf, + // so we need to allow it. can also happen if brooklyn.libraries references an existing bundle. + + // determine if we are simply bringing existing installed under Brooklyn management (because url or binary content identical and not forced) + // or if the bundle should be reinstalled/updated + if (!force) { + if (!isBringingExistingOsgiInstalledBundleUnderBrooklynManagement) { + if (Objects.equal(b.get().getLocation(), suppliedKnownBundleMetadata.getUrl())) { + // installation request was for identical location, so assume we are simply bringing under mgmt + log.debug("Request to install "+inferredMetadata+" from same location "+b.get().getLocation()+ + " as existing OSGi installed (but not Brooklyn-managed) bundle "+b.get()+", so skipping reinstall"); + isBringingExistingOsgiInstalledBundleUnderBrooklynManagement = true; + } else { + // different locations, but see if we can compare input stream contents + // (prevents needless uninstall/reinstall of already installed bundles) + try { + if (Streams.compare(new FileInputStream(zipFile), new URL(b.get().getLocation()).openStream())) { + log.debug("Request to install "+inferredMetadata+" has same contents"+ + " as existing OSGi installed (but not Brooklyn-managed) bundle "+b.get()+", so skipping reinstall"); + isBringingExistingOsgiInstalledBundleUnderBrooklynManagement = true; + } else { + log.debug("Request to install "+inferredMetadata+" has different contents"+ + " as existing OSGi installed (but not Brooklyn-managed) bundle "+b.get()+", so will do reinstall"); + } + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + // probably an invalid URL on installed bundle; that's allowed + log.debug("Request to install "+inferredMetadata+" could not compare contents"+ + " with existing OSGi installed (but not Brooklyn-managed) bundle "+b.get()+", so will do reinstall (error "+e+" loading from "+b.get().getLocation()+")"); + } + } + } + } else { + if (isBringingExistingOsgiInstalledBundleUnderBrooklynManagement) { + log.debug("Request to install "+inferredMetadata+" was forced, so forcing reinstallation " + + "of existing OSGi installed (but not Brooklyn-managed) bundle "+b.get()); + isBringingExistingOsgiInstalledBundleUnderBrooklynManagement = false; + } + } + + if (isBringingExistingOsgiInstalledBundleUnderBrooklynManagement) { + result.bundle = b.get(); + + } else { + // needs reinstallation + // we could update instead of uninstall/reinstall + // note however either way we won't be able to rollback if there is a failure + log.debug("Brooklyn install of "+result.getMetadata().getVersionedName()+" detected already loaded in OSGi; uninstalling that to reinstall as Brooklyn-managed"); + b.get().uninstall(); + result.bundle = null; + } } // normal install updating = false; @@ -381,8 +426,13 @@ public ReferenceWithError install() { startedInstallation = true; try (InputStream fin = new FileInputStream(zipFile)) { if (!updating) { - assert result.getBundle()==null; - result.bundle = osgiManager.framework.getBundleContext().installBundle(result.getMetadata().getOsgiUniqueUrl(), fin); + if (isBringingExistingOsgiInstalledBundleUnderBrooklynManagement) { + assert result.getBundle()!=null; + log.debug("Brooklyn install of "+result.getMetadata().getVersionedName()+" detected already loaded "+result.getBundle()+" in OSGi can be re-used, skipping OSGi install"); + } else { + assert result.getBundle()==null; + result.bundle = osgiManager.framework.getBundleContext().installBundle(result.getMetadata().getOsgiUniqueUrl(), fin); + } } else { result.bundle.update(fin); } @@ -440,9 +490,12 @@ private void rollbackBundle() { ((BasicManagedBundle)result.getMetadata()).setPersistenceNeeded(true); mgmt().getRebindManager().getChangeListener().onChanged(result.getMetadata()); } else { - log.debug("Uninstalling bundle "+result.getVersionedName()+" (roll back of failed fresh install, no previous version to revert to)"); - osgiManager.uninstallUploadedBundle(result.getMetadata()); - + if (isBringingExistingOsgiInstalledBundleUnderBrooklynManagement) { + log.debug("Uninstalling bundle "+result.getVersionedName()+" from Brooklyn management only (rollback needed but it was already installed to OSGi)"); + } else { + log.debug("Uninstalling bundle "+result.getVersionedName()+" (roll back of failed fresh install, no previous version to revert to)"); + } + osgiManager.uninstallUploadedBundle(result.getMetadata(), false, isBringingExistingOsgiInstalledBundleUnderBrooklynManagement); ((BasicManagedBundle)result.getMetadata()).setPersistenceNeeded(true); mgmt().getRebindManager().getChangeListener().onUnmanaged(result.getMetadata()); } diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java index 603927a45d..995f7ed97f 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java @@ -393,6 +393,10 @@ public OsgiBundleInstallationResult uninstallUploadedBundle(ManagedBundle bundle * This does not throw but returns a reference containing errors and result for caller to inspect and handle. */ public ReferenceWithError uninstallUploadedBundle(ManagedBundle bundleMetadata, boolean force) { + return uninstallUploadedBundle(bundleMetadata, force, false); + } + + public ReferenceWithError uninstallUploadedBundle(ManagedBundle bundleMetadata, boolean force, boolean leaveInOsgi) { OsgiBundleInstallationResult result = new OsgiBundleInstallationResult(); result.metadata = bundleMetadata; List errors = MutableList.of(); @@ -425,22 +429,24 @@ public ReferenceWithError uninstallUploadedBundle( errors.add(e); } - Bundle bundle = framework.getBundleContext().getBundle(bundleMetadata.getOsgiUniqueUrl()); - result.bundle = bundle; - if (bundle==null) { - Exception e = new IllegalStateException("No such bundle installed in OSGi when uninstalling: "+bundleMetadata); - if (!force) Exceptions.propagate(e); - log.warn(e.getMessage()); - errors.add(e); - } else { - try { - bundle.stop(); - bundle.uninstall(); - } catch (BundleException e) { - Exceptions.propagateIfFatal(e); + if (!leaveInOsgi) { + Bundle bundle = framework.getBundleContext().getBundle(bundleMetadata.getOsgiUniqueUrl()); + result.bundle = bundle; + if (bundle==null) { + Exception e = new IllegalStateException("No such bundle installed in OSGi when uninstalling: "+bundleMetadata); if (!force) Exceptions.propagate(e); - log.warn("Error stopping and uninstalling "+bundleMetadata+": "+e); - errors.add(e); + log.warn(e.getMessage()); + errors.add(e); + } else { + try { + bundle.stop(); + bundle.uninstall(); + } catch (BundleException e) { + Exceptions.propagateIfFatal(e); + if (!force) Exceptions.propagate(e); + log.warn("Error stopping and uninstalling "+bundleMetadata+": "+e); + errors.add(e); + } } } } catch (Exception e) { From dcde6806264904ea31a71bf8565994d01b0e8d69 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Thu, 21 Sep 2017 14:08:40 +0100 Subject: [PATCH 4/6] improve making bundle from folders needed for tests now that we do more with bundles supplied in brooklyn.libraries --- .../brooklyn/util/core/osgi/BundleMaker.java | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/util/core/osgi/BundleMaker.java b/core/src/main/java/org/apache/brooklyn/util/core/osgi/BundleMaker.java index 9116c0e491..65b3aec6ee 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/osgi/BundleMaker.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/osgi/BundleMaker.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.net.URL; import java.util.Enumeration; import java.util.Map; import java.util.Set; @@ -298,16 +299,29 @@ private boolean addUrlItemRecursively(ZipOutputStream zout, String root, String return false; } - if (!addUrlDirToZipRecursively(zout, root, item, itemFound, filter)) { - // can't reliably tell if item a file or a folder (listing files), esp w classpath where folder is just a list of files, - // so try the latter and see if it succeeds - // slightly inefficient but should work fine - // only issue is that an empty dir and a file of size 0 will appear identical? + if (isKnownNotToBeADirectoryListing(item) || ! + // can't reliably tell if item a file or a folder (listing files), esp w classpath where folder is treated as a list of files, + // so if we can't tell try it as a list of files; not guaranteed, and empty dir and a file of size 0 will appear identical, but better than was + // (mainly used for tests) + addUrlDirToZipRecursively(zout, root, item, itemFound, filter)) { addUrlFileToZip(zout, root, item, filter); } return true; } + private boolean isKnownNotToBeADirectoryListing(String item) { + try { + URL url = new URL(item); + if (url.getProtocol().equals("file")) { + return !new File(url.getFile()).isDirectory(); + } + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + // ignore otherwise -- probably unknown protocol + } + return false; + } + private boolean addUrlDirToZipRecursively(ZipOutputStream zout, String root, String item, InputStream itemFound, Predicate filter) throws IOException { LineReader lr = new LineReader(new InputStreamReader(itemFound)); boolean readSubdirFile = false; @@ -325,7 +339,8 @@ private boolean addUrlDirToZipRecursively(ZipOutputStream zout, String root, Str // not a folder return false; } else { - // previous entry made clear it was a folder, but this one didn't work! + // previous entry suggested it was a folder, but this one didn't work! -- was a false positive + // but zip will be in inconsistent state, so throw throw new IllegalStateException("Failed to read entry "+line+" in "+item+" but previous entry implied it was a directory"); } } @@ -364,7 +379,7 @@ public File createTempBundle(String nameHint, Manifest mf, Map Date: Thu, 21 Sep 2017 14:29:02 +0100 Subject: [PATCH 5/6] add support for dev mode bundle urls so tests pass makes behaviour consistent with dist behaviour regarding karaf urls --- .../core/mgmt/ha/OsgiArchiveInstaller.java | 29 +++++++++++++++++-- .../brooklyn/util/core/ClassLoaderUtils.java | 7 +++-- .../util/core/ClassLoaderUtilsTest.java | 6 ++-- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java index 5f053e5a52..307dcd90ba 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java @@ -20,6 +20,7 @@ import java.io.File; import java.io.FileInputStream; +import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; @@ -34,6 +35,7 @@ import org.apache.brooklyn.api.typereg.ManagedBundle; import org.apache.brooklyn.api.typereg.RegisteredType; +import org.apache.brooklyn.core.BrooklynVersion; import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog; import org.apache.brooklyn.core.mgmt.ha.OsgiBundleInstallationResult.ResultCode; import org.apache.brooklyn.core.mgmt.internal.ManagementContextInternal; @@ -198,9 +200,26 @@ private synchronized void makeLocalZipFileFromInputStreamOrUrl() { if (zipIn!=null) { // found input stream for existing osgi bundle + } else if (existingBrooklynInstalledBundle.isAbsent() || force) { - // reload - zipIn = ResourceUtils.create(mgmt()).getResourceFromUrl(suppliedKnownBundleMetadata.getUrl()); + // reload + String url = suppliedKnownBundleMetadata.getUrl(); + if (BrooklynVersion.isDevelopmentEnvironment() && url.startsWith("system:file:")) { + // in live dists the url is usually mvn: but in dev/test karaf will prefix it with system; + // leave the url alone so we correctly dedupe when considering whether to update, but create a zip file + // so that things work consistently in dev/test (in particular ClassLoaderUtilsTest passes). + // pretty sure we have to do this, even if not replacing the osgi bundle, because we need to + // get a handle on the zip file (although we could skip if not doing persistence - but that feels even worse than this!) + try { + url = Strings.removeFromStart(url, "system:"); + File zipTemp = new BundleMaker(ResourceUtils.create()).createJarFromClasspathDir(url); + zipIn = new FileInputStream( zipTemp ); + } catch (FileNotFoundException e) { + throw Exceptions.propagate(e); + } + } else { + zipIn = ResourceUtils.create(mgmt()).getResourceFromUrl(url); + } } else { // already installed, not forced, just say already installed // (even if snapshot as this is a reference by URL, not uploaded content) @@ -217,6 +236,12 @@ private synchronized void makeLocalZipFileFromInputStreamOrUrl() { try (FileOutputStream fos = new FileOutputStream(zipFile)) { Streams.copy(zipIn, fos); zipIn.close(); + try (ZipFile zf = new ZipFile(zipFile)) { + // validate it is a valid ZIP, otherwise errors are more obscure later. + // can happen esp if user supplies a file://path/to/folder/ as the URL.openStream returns a list of that folder (!) + // the error thrown by the below is useful enough, and caller will wrap with suppliedKnownBundleMetadata details + zf.entries(); + } } catch (Exception e) { throw Exceptions.propagate(e); } finally { diff --git a/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java b/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java index c3e75be3ee..fef27dff95 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/ClassLoaderUtils.java @@ -330,8 +330,11 @@ protected Maybe tryLoadFromBundle(LoaderDispatcher dispatcher, String } return dispatcher.tryLoadFrom(bundle.get(), name); } else { - log.warn("Request for bundle '"+symbolicName+"' "+(Strings.isNonBlank(version) ? "("+version+") " : "")+"will be ignored as no framework available; will look for '"+name+"' in plain old classpath"); - return dispatcher.tryLoadFrom(classLoader, name); + Maybe result = dispatcher.tryLoadFrom(classLoader, name); + if (result.isAbsent()) { + log.warn("Request for bundle '"+symbolicName+"' "+(Strings.isNonBlank(version) ? "("+version+") " : "")+"was ignored as no framework available; and failed to find '"+name+"' in plain old classpath"); + } + return result; } } diff --git a/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java b/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java index 1477798a0a..1a6da269dc 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/ClassLoaderUtilsTest.java @@ -58,7 +58,6 @@ import org.apache.brooklyn.util.maven.MavenArtifact; import org.apache.brooklyn.util.maven.MavenRetriever; import org.apache.brooklyn.util.osgi.OsgiTestResources; -import org.apache.brooklyn.util.osgi.OsgiUtils; import org.apache.commons.io.output.ByteArrayOutputStream; import org.osgi.framework.Bundle; import org.osgi.framework.launch.Framework; @@ -216,7 +215,10 @@ public void testLoadClassInOsgiCore() throws Exception { mgmt = LocalManagementContextForTests.builder(true).enableOsgiReusable().build(); Bundle bundle = getBundle(mgmt, "org.apache.brooklyn.core"); - Entity entity = createSimpleEntity(bundle.getLocation(), clazz); + String url = bundle.getLocation(); + // NB: the above will be a system:file: url when running tests against target/classes/ -- but + // OSGi manager will accept that if running in dev mode + Entity entity = createSimpleEntity(url, clazz); ClassLoaderUtils cluMgmt = new ClassLoaderUtils(getClass(), mgmt); ClassLoaderUtils cluClass = new ClassLoaderUtils(clazz); From 9a01418bcb8e73fef78fefa5a3e42533139bc797 Mon Sep 17 00:00:00 2001 From: Alex Heneveld Date: Tue, 26 Sep 2017 13:16:07 +0100 Subject: [PATCH 6/6] expand comments as per PR review --- .../brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java index 307dcd90ba..ff84380641 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java @@ -174,7 +174,14 @@ private synchronized void makeLocalZipFileFromInputStreamOrUrl() { } else { // if bundle is not brooklyn-managed we want to make it be so // and for that we need to find a URL. - // some things declare usable locations, though these might be maven and + // the getLocation() _might_ be usable, or might be totally opaque; + // in tests we rely on the block below (see system:file:) and things + // being explicitly set, but in live and rebind deployments the URL + // in practice with karaf how we package it is of the form mvn:... + // which _does_ work in this block, so we will be able to do most + // things which rely on taking osgi-installed bundles into brooklyn mgmt + // (and if not don't think it's a big deal, we just uninstall and reinstall + // sometimes or fail with a reasonable error message) String candidateUrl = existingOsgiInstalledBundle.get().getLocation(); log.debug("Detected bundle "+suppliedKnownBundleMetadata+" installed to OSGi but not Brooklyn; trying to find a URL to get bundle binary, candidate "+candidateUrl); if (Strings.isBlank(candidateUrl)) {