Bundle uninstall and snapshot #672

ahgittin · 2017-05-08T12:36:16Z

Supports bundle install and auto-reinstall for snapshot POSTs.

Doesn't do anything for auto-upgrade but if you POST a new snapshot ZIP then redeploy, the newer version gets used. Also cleanly uninstalls catalog items.

doesn't yet remove catalog items, and no effort yet to test reinstall. (need to record the defining bundle on registered types.)

…emantics rebind can break however -- we need to phase the bundle installs and the bundle starts

also recognise bundles from their originally installed URL, and misc other related cleanups; upgrade tests and others now passing! note: transformers are only allowed to delete bundles, not change

Tests assume BOMs wouldn't be installed, and either test wasn't expecting to read BOM or BOM had a problem

Removal of XML catalog support caused minor conflicts in imports and tests, easily resolved.

ahgittin · 2017-05-08T16:04:51Z

Failure due to deadlock described and fixed in #671 . Tests pass locally.

geomacy · 2017-05-09T16:27:42Z

@ahgittin have made a start to this but not got all the way through it yet; looks great; I like the tests testClassAccessAfterUninstall and friends

conflicts - imports (resolved), and osgi test jars (rebuilt)

geomacy

Looks great; I like that the catalog items can now include the references to the bundles they are installed from.

Some minor comments, the main one of which is in discoverManifestFromCatalog.bom.

I haven't tested this yet but will run some tests too.

geomacy · 2017-05-10T16:37:44Z

core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java

+                        ((BasicManagedBundle)suppliedKnownBundleMetadata).setVersion(mbFromUrl.getVersion());
+                    }
+                }
+                if (installedBundle.isAbsent() && suppliedKnownBundleMetadata.getOsgiUniqueUrl()!=null) {


The sequence of ifs here looks like it has the potential to overwrite installedBundle repeatedly; are these various tests meant to be exclusive possibilities? Perhaps there should be elses between the ifs here, with the clauses organised in an appropriate order.

there's an isAbsent check in each if test and the result of each body might be an absent so `else wouldn't be sufficient

geomacy · 2017-05-11T09:04:51Z

core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java

+
+    /** See {@link OsgiArchiveInstaller#install()} - this exposes custom options */
+    @Beta
+    public ReferenceWithError<OsgiBundleInstallationResult> install(@Nullable ManagedBundle knownBundleMetadata, @Nullable InputStream zipIn,


It's interesting that the only place this is called is in test code; in one sense this maybe isn't needed! But good to have it for completeness's sake.

geomacy · 2017-05-11T09:31:39Z

core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java

+        try {
+            FileOutputStream fos = new FileOutputStream(zipFile);
+            Streams.copy(zipIn, fos);
+            zipIn.close();


Do the closes in the finally.

geomacy · 2017-05-11T09:45:00Z

core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java

+            throw new IllegalArgumentException("Missing bundle version in BOM or MANIFEST");
+        }
+        if (discoveredManifest.getMainAttributes().getValue(Attributes.Name.MANIFEST_VERSION)==null) {
+            discoveredManifest.getMainAttributes().putValue(Attributes.Name.MANIFEST_VERSION.toString(), "1.0");


Instead of "1.0", should this not be consistent with what's declared in the catalog.bom, if any - i.e. should be discoveredBomVersionedName.getVersion() (with checks that discoveredBomVersionedName != null if needed?)

I think this is always "1.0" (I think it's about the version of the manifest format?), rather than being the version of the code within the jar.

ah good point thanks @aledsage

have extracted "1.0" as a private static final with comment that this is what it has to be

geomacy · 2017-05-11T12:30:55Z

core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java

-
+    private File osgiCacheDir;
+    Map<String, ManagedBundle> managedBundles = MutableMap.of();
+    Map<VersionedName, String> managedBundlesByNam = MutableMap.of();


typo managedBundlesByNam

geomacy · 2017-05-11T12:35:53Z

core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java

+                    }
+                }
+                result.code = OsgiBundleInstallationResult.ResultCode.INSTALLED_NEW_BUNDLE;
+                result.message = "Installed "+result.getMetadata().getVersionedName()+" with ID "+result.getMetadata().getId();


Worth adding a log message in here as well, and below.

geomacy · 2017-05-11T12:49:41Z

core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiManager.java

+     * <p>
+     * Callers should typically fail if anything from this bundle is in use.
+     */
+    public void uninstallUploadedBundle(ManagedBundle bundleMetadata) {


Again, this is only called from test code. Arguably the Catalog API is missing a delete method to let you remove bundles you've added via upload. That would be another PR, of course.

+1 to needing delete

geomacy · 2017-05-11T13:01:46Z

core/src/main/java/org/apache/brooklyn/core/typereg/BasicRegisteredType.java

    final RegisteredTypeKind kind;
    final String symbolicName;
    final String version;
+    String containingBundle;


Would it not be better to make this a VersionedName?

geomacy · 2017-05-11T13:05:35Z

api/src/main/java/org/apache/brooklyn/api/catalog/CatalogItem.java


    public String getSymbolicName();
+
+    public String getContainingBundle();


Why not make this a VersionedName?

I've done this and a commit, but then reverted it. I'm not sure everything comes from a bundle just yet. When it does we can reinstate. To me it is tied up with use of search paths etc.

geomacy · 2017-05-11T13:19:24Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/CatalogResource.java

+                    //RegisteredType ci = mgmt.getTypeRegistry().get(id);
+
+                    CatalogItem<?, ?> ci = CatalogUtils.getCatalogItemOptionalVersion(mgmt, id);
+                    CatalogTransformer.catalogItemSummary(brooklynU, ci, ui.getBaseUriBuilder());


Shouldn't this be

CatalogItemSummary summary = CatalogTransformer.catalogItemSummary(brooklynU, ci, ui.getBaseUriBuilder()); result.types().put(id, summary);

absolutely -- good spot!

geomacy · 2017-05-26T14:57:40Z

I have finally got round to doing a test on this, sorry to have taken so long.

I will attach a couple of zip files here for test purposes, each being a separate -SNAPSHOT revision of a simple blueprint. The second builds upon the first, getting a bit more complicated.

My first test zip file causes an error when I try to upload it; what you see on the command line is

br catalog add file://$(pwd)/test672a.zip
json: cannot unmarshal string into Go value of type models.CatalogEntitySummary

and in the Karaf log you get

2017-05-26 15:46:42,844 WARN  121 o.a.b.u.c.j.ErrorAndToStringUnknownTypeSerializer [qtp941342355-186] Standard serialization not possible for class org.apache.brooklyn.rest.resources.CatalogResource$BundleInstallationRestResult (org.apache.brooklyn.rest.resources.CatalogResource$BundleInstallationRestResult@3f7a9dbf)
... lots of repetition deleted...
com.fasterxml.jackson.databind.JsonMappingException: Infinite recursion (StackOverflowError) (through reference chain: org.apache.brooklyn.core.relations.ByObjectBasicRelationSupport["source"]->org.apache.brooklyn.core.typereg.BasicManagedBundle["relations"]->org.apache.brooklyn.core.relations.ByObjectBasicRelationSupport["source"]->org.apache.brooklyn.core.typereg.BasicManagedBundle["relations"]->org.apache.brookly
        at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:694)
        at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:157)
        at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:672)
        at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:678)
        at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:157)
        at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:672)
        ... many lines deleted...

I tested the same file against master and it loads ok there, so I think there is something in the PR to cause this.

I will see if I can make a bit of time next week to look at this again and investigate what's going on.

geomacy · 2017-05-27T08:53:18Z

Test files attached (both cause the problem described above.

test672a.zip
test672b.zip

… as per PR" This reverts commit f4205fd. The "containingBundle" might, for a transitional period, need to support a reference to a catalog item instead of a bundle, in which case OSGi conventions might not be valid and we want this to be a string. TBC. Will add comment to that effect in the code shortly. If/when reinstated we should also ensure the deserialization accepts strings (should probably also give a custom serializer so VersionedName is always written as a string to prevent bloat in the serialized format).

ahgittin · 2017-06-05T10:59:31Z

Great comments @geomacy - most addressed. Now working on the bug you found which is caused by making the "simple" BasicManagedBundle a BrooklynObject which has extra baggage and needs special serialization.

preserves backwards compatibility, adding a detail=true flag which gives richer info. see extended comment in code. also fixes bug where the added/changed items weren't actually listed, and where it tried to return the full BasicManagedBundle when it just wants an ID.

ahgittin · 2017-06-05T13:03:18Z

This should address everything, including backwards compatibility so the existing CLI works (though it doesn't show the detail). New PR coming for the CLI to be able to show the detail.

All changes minor and obvious in light of the PR except for 33d9fd6#diff-3da051230841536d82a7bcceabdb280aR113 which has a detailed comment in the code (above the method). @geomacy LMK if you think that is too bad or worth it as a temporary fix until we have a new /bundles endpoint.

geomacy · 2017-06-05T14:38:55Z

@ahgittin changes look good. I'm comfortable with the variant return type on createFromArchive, especially given the @Beta annotation on it. I shall pull this and try to get my test going and hopefully merge.

geomacy · 2017-06-06T12:23:10Z

Tests all look good, I added test672a.zip to catalog

[geoff:] master(+0/-889)* ± br catalog add test672a.zip
myTestEnt:0.12.0.SNAPSHOT
myOtherTest:0.12.0.SNAPSHOT
testent1:0.12.0.SNAPSHOT

then deployed a testEnt1 from it, then added the revised snapshot version of the blueprint by adding test672b.zip:

[geoff:] master(+0/-889)* ± br catalog add test672b.zip
myTestEnt:0.12.0.SNAPSHOT
myOtherTest:0.12.0.SNAPSHOT
myOtherTest2:0.12.0.SNAPSHOT
furtherTest:0.12.0.SNAPSHOT
testent1:0.12.0.SNAPSHOT
testent2:0.12.0.SNAPSHOT

and again deployed a testEnt1; on this second deployment the updated definition from the second zip was used and the entity created with two children.

LGTM, will merge.

Bundle uninstall and snapshot Supports bundle install and auto-reinstall for snapshot POSTs. Doesn't do anything for auto-upgrade but if you POST a new snapshot ZIP then redeploy, the newer version gets used. Also cleanly uninstalls catalog items.

ahgittin · 2017-06-06T15:57:25Z

Thanks @geomacy. Merged apache/brooklyn-client#53 following @Graeme-Miller 's review there but as I'm a go newbie (g00b ?) appreciate any comments you have there.

Fixes break from apache#672, I merged without re-running unit tests :-(

Check versionedName to avoid NPE. Fixes break from #672, I merged without re-running unit tests :-(

ahgittin · 2017-06-07T11:11:04Z

Thanks @geomacy for the super-quick follow-up fix in #719.

Fixes break from apache#672, I merged without re-running unit tests :-(

neykov · 2017-06-15T10:29:32Z

I've been catching up on some of the merged PRs and noticed the following. When doing br catalog add . with the following catalog.bom in the folder:

brooklyn.catalog:
  bundle: org.apache.brooklyn.test
  version: "0.1.0-SNAPSHOT"
  itemType: entity
  item:
    type: org.apache.brooklyn.core.test.entity.TestEntity

I get the error:

Server error (400): invalid version "0.1.0-SNAPSHOT": non-numeric "0-SNAPSHOT": NumberFormatException: For input string: "0-SNAPSHOT"

In catalog.bom we are using maven style versioning and the OSGi style of versioning differs from it, mostly by enforcing a .delimiter for the extra tags. It's not restricted to SNAPSHOT versions (which is not a special version in OSGi). It applies to any tagged version.

The code must take this into account and:

Convert maven style versions to osgi style versions when initializing the bundle metadata. There's OsgiUtils.toOsgiVersion(String) to do the mapping which uses the same code as maven does to do the conversion.
Keep the duality hidden to the internals. The version from catalog.bom should be the one we see the catalog item created with, etc.
For the case where there's no catalog.bom and we rely on the MANIFEST.MF - try to get the maven version and use that for the user-visible parts. For example Implementation-Version header or pom.properties. Fallback to the Version header when no alternative source is available.
The conversion is one way - we can't recover the maven version from an OSGi version. This means we can't directly map from a bundle version to a catalog version without additional metadata.

geomacy · 2017-06-15T10:39:22Z

Very good point Svet. I got my test above working with snapshot versions exactly by supplying the OSGI version in the bom, without really thinking about whether that was the right thing to do or not:

brooklyn.catalog:
  bundle: io.cloudsoft.cli44
  version: 0.12.0.SNAPSHOT

but indeed it's not right that someone should have to do that.

+1 to your bullets above.

geomacy · 2017-06-15T18:12:15Z

P.S. I can have a look at implementing those changes, unless anyone else is particularly keen to do so?

ahgittin · 2017-06-16T15:51:22Z

@neykov @geomacy I've been working in this area for a different reason - if people use v1 as a version a few minor things break.

wdyt about using OSGi versions everywhere, treating anything else including maven versions, as legacy/deprecated? i'm hoping in future all catalog item versions inherit from the bundle version, IE we stop versioning items separately, but that needs to be discussed. (if you installed a non-bundle BOM and bundle would be created.)

for convenience if uploading a bundle with a maven (non-osgi) version, we should convert it (one-way) to an osgi version, and also log a warning. if we do that i guess there is a question should we use the converted-to-osgi version string for items that inherit (items that define their own version will use what they define but if that differs from bundle maybe we will warn?)

geomacy · 2017-06-19T08:34:26Z

@ahgittin re. the version numbers; suggest we take this to the mailing list to disuss, but my feeling is that people are used to using Maven versions and it's convenient to regard those as "the" version for a module ( = bundle). Could be annoying/error-prone to have to maintain both a version in a pom.xml and another (separate, but almost the same) version in a catalog.bom.

neykov · 2017-06-19T08:55:07Z

I'm not sold on the idea, but that could be because I spend too much time in Java land.
OSGi style versioning feels niche - not widely known/used. It's more restrictive. We'll still need to special case SNAPSHOT even though it's not a thing in OSGi. semver which is much wider used, even outside of the Java community, better matches our current (maven) versioning style.

I like the vision of having bundle-first design, but feel it's hidden from the user so we are free not not leak the details to the UI.

In any case this deserves a mailing list discussion.

ahgittin · 2017-06-19T13:57:41Z

Thanks @geomacy @neykov -- good points. Will research and take to ML.

ahgittin · 2017-06-21T17:31:39Z

Results of ML discussion summarised in apache/brooklyn-docs#198 , with PR to follow on this project implementing that (which will be referenced there)

ahgittin added 7 commits May 5, 2017 12:43

basic support for uninstalling bundle, and sketch of reinstall

bc11ab2

doesn't yet remove catalog items, and no effort yet to test reinstall. (need to record the defining bundle on registered types.)

record the containing bundle for any catalog item / registered type

8a4886f

uninstall removes catalog items

f76118c

refactor osgi installation to facilitate upgrades and richer return s…

1452454

…emantics rebind can break however -- we need to phase the bundle installs and the bundle starts

install and start in separate phases when rebinding or loading libs

062fdc4

also recognise bundles from their originally installed URL, and misc other related cleanups; upgrade tests and others now passing! note: transformers are only allowed to delete bundles, not change

Fixup of tests which failed following BOM auto-install

abdb13e

Tests assume BOMs wouldn't be installed, and either test wasn't expecting to read BOM or BOM had a problem

Merge branch 'master' into bundle-uninstall-and-snapshot

71ede73

Removal of XML catalog support caused minor conflicts in imports and tests, easily resolved.

Merge branch 'master' into bundle-uninstall-and-snapshot

f562ce7

conflicts - imports (resolved), and osgi test jars (rebuilt)

geomacy reviewed May 11, 2017

View reviewed changes

ahgittin added 6 commits June 5, 2017 11:05

address PR comments - the straightforward ones

bb7c04d

utils for working with VersionedName better

3cf028a

record VersionedName instead of string for containing bundle, as per PR

f4205fd

notes on containing bundle being a versioned name

5fad03d

synchronize osgi bundle managers inside dedicated package-private class

e1b0be1

ahgittin force-pushed the bundle-uninstall-and-snapshot branch from 1954ee2 to 33d9fd6 Compare June 5, 2017 13:00

ahgittin mentioned this pull request Jun 5, 2017

show bundle info and message when doing catalog add apache/brooklyn-client#53

Merged

Merge branch 'master' into bundle-uninstall-and-snapshot

55def2e

asfgit merged commit 55def2e into apache:master Jun 6, 2017

geomacy added a commit to geomacy/brooklyn-server that referenced this pull request Jun 6, 2017

Check versionedName to avoid NPE.

c545381

Fixes break from apache#672, I merged without re-running unit tests :-(

geomacy mentioned this pull request Jun 6, 2017

Check versionedName to avoid NPE. #719

Merged

asfgit pushed a commit that referenced this pull request Jun 6, 2017

Closes #719

30292c9

Check versionedName to avoid NPE. Fixes break from #672, I merged without re-running unit tests :-(

ahgittin deleted the bundle-uninstall-and-snapshot branch June 7, 2017 11:11

robertgmoss pushed a commit to robertgmoss/brooklyn-server that referenced this pull request Jun 7, 2017

Check versionedName to avoid NPE.

c905724

Fixes break from apache#672, I merged without re-running unit tests :-(

ahgittin mentioned this pull request Jun 16, 2017

Clean up inference around parsing name:version strings #737

Merged

This was referenced Jun 21, 2017

New versioning rules prep #740

Merged

Version new rules #743

Merged


		public String getSymbolicName();

		public String getContainingBundle();

Bundle uninstall and snapshot #672

Bundle uninstall and snapshot #672

Uh oh!

Conversation

ahgittin commented May 8, 2017

Uh oh!

ahgittin commented May 8, 2017

Uh oh!

geomacy commented May 9, 2017

Uh oh!

geomacy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geomacy commented May 26, 2017

Uh oh!

geomacy commented May 27, 2017

Uh oh!

ahgittin commented Jun 5, 2017

Uh oh!

ahgittin commented Jun 5, 2017

Uh oh!

geomacy commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geomacy commented Jun 6, 2017

Uh oh!

ahgittin commented Jun 6, 2017

Uh oh!

ahgittin commented Jun 7, 2017

Uh oh!

neykov commented Jun 15, 2017

Uh oh!

geomacy commented Jun 15, 2017

Uh oh!

geomacy commented Jun 15, 2017

Uh oh!

ahgittin commented Jun 16, 2017

Uh oh!

geomacy commented Jun 19, 2017

Uh oh!

neykov commented Jun 19, 2017

Uh oh!

ahgittin commented Jun 19, 2017

Uh oh!

geomacy commented Jun 5, 2017 •

edited

Loading