Auto wrap YAML BOMs as catalog bundles #746

ahgittin · 2017-06-28T11:31:32Z

Wraps pure-YAML BOMs uploaded to catalog in a bundle, creating a name and version if needed.

This allows us to see all items added as a unit. This will allow us to prefer loading co-defined items (though that's not fully done, still relies on preferring search path when doing item lookup), and to edit or remove that item as a set (crucially we now keep the BOM, previously we discarded it!).

Docs changed to reflect this and explain differences in another PR.

The basic wrapping was pretty simple but a surprising number of subtleties emerged which this fixes:

java scanning doesn't work in OSGi; docs and errors make this clear
impacts replacement semantics (now it does a hash on the bundle to make most things the same, though a re-post of a non-SNAPSHOT BOM that doesn't declare a bundle will now fail because the random bundle name is stored on the item as containingBundle and is different on a re-post; and needed extra work to remove leaked anonymous bundles)
had to change the way we set up the libraries / search path, to exclude this "bundle" (mainly so tests are unchanged, but also improves efficiency for simple BOMs; also the code is cleaner now)

And there is one thing that emerged that I haven't fixed: on rebind it became vital to handle the situation where dependent bundles might not be loaded before the depender bundle; see failing testtestLongReferenceSequence. Not entirely sure how to do this /cc @neykov @geomacy @aledsage.

Depends on #743.

creates a new bundle when given yaml in catalog; bundle name and version now recommended in the BOM. scan-java option tweaked but in a way that is consistent with the past and sensible in the new world. auto-wrapped bundles are identified with a header so we can simplify their handling in some cases (scanning, search paths). some things clumsy and need fixed: * uploading a different BOM (or bundle) at the same version says "ignoring because identical"; osgi identicality check should look at contents * auto-wrapped bundles are added to the library search path (no need for this as the bundles are empty) * failed installations keep the bundle installed, blocking subsequent installs; probably don't install unless forced? * warn if different version declared in BOM some things clumsy but we could live with: * containing bundles are added as libraries by munging the yaml; now we have a record to that so can do a bit better than munge yaml * if bundle has no name, a random one is chosen (probably deprecated this behaviour); if you re-submit we don't know it's the same bundle so we don't detect it's a bundle update; this means: * uploading the exact same non-snapshot BOM twice will fail the second time saying the items are different (because containing bundle is different) * uploading an updated item in a snapshot BOM will correctly replace, but the old bundle will still be around (just masked; though we can fix that)

scanning for OSGi can be made to work in some cases if we have the jar, but it won't work after rebind, or even in some cases if persisting (race deleting the JAR), plus there are likely classloading issues; note OSGi deliberately doesn't give us the JAR. java scanning is discouraged by OSGi; it wants us to explicitly list things e.g. in the catalog.bom, which is fine.

…accordingly This permits us to allow the same BOM YAML to be added multiple times (whether or not bundles are declared), except where it's a non-snapshot BOM that doesn't declare a bundle, because the containing bundle will be different there. This also fixes a bug where containingBundle wasn't persisted; not a big problem as we re-read bundle items, but cleaner to have it working.

new batch of tests around catalog versioning with OSGi

including explicit test for new load order problems: * during rebind osgi bundles can have BOM loaded in any order, but they expect their deps to be available two remaining problems: * wrapper library is added (changing assertions about numbers) * added containing library changes YAML (changing assertions about comments)

…rapper

There is no point to them (they aren't OSGi). Fixes bug where uploading the same BOM multiple times, could cause bundle leakage. And tidy logging, more tests.

… CatalogItem added to non-persisting BasicBrooklynTypeRegistry, currently without validation; with quite a few tests, and previous failing cobundle-reference tests now uncommented. deletion support added as beta to TypeRegistry, works alongside Catalog deletion. not done, lots of tests failing still, but lots working, and shape of code pretty good. (even if the two-path code in BasicBrooklynCatalog is ugly, it's private and we'll be able to simplify a lot if/when we cut the old catalog code) need to next: perform some validation on the added registered types, as a secondary phase so we can add all types first, and populate supertypes then, which should allow all tests to pass,

with support for resolving subsequently, solving problems with references; also shifts most things to being in _unpersisted_ type registry, repopulated on rebind, instead of the clunky catalog which persists items as XML additional todo items: * make tests pass * have all OSGi routines use the new `catalog.addTypes...` instead of `.addItems` * fix clash in removing empty bundles

ahgittin · 2017-06-30T02:10:21Z

have done a big job to make things use the TypeRegistry instead of the old catalog, and to add to type registry in an UNRESOLVED state first. this fixes the big problem by adding types from all bundles into an unresolved state, then resolving them.

it has ended up much bigger than i meant BUT it has several main advantages:

we no longer persist catalog item XML for things where we persist the OSGi bundle
we're very close to being able to remove nearly all the clunky "catalog" code; apart from items that were persisted as XML everything uses the new type registry now

the logic for guessing the supertypes is messy, hooking in to the old Guessing interpreter in a brittle way, but if we plan to remove old catalog code (and annotations scanning) we can rewrite it all to be much simpler and nicer.

note this PR is still WIP, not working 100%; we don't support templates in the registry yet, locations need attention, and there are quite a few TODO items to look at. but wanted to give people sight of it.

…problem quite a few holes to fill, but confirmed working in a few places. also makes sure not to detect and delete empty wrapper bundles until we've finished adding them.

some failing tests still: * still need to address TEMPLATEs in OSGi * deprecation / catalog.xml persistence

these are no longer supported; need to think through how to address

…eprint check now permit uploading the same BOM twice, forgive different containing bundle in this case (some cleverness to allow that, but preserves compatibility there; code needed changing anyway to allow adding unresovled items then replacing as resolved items)

note items added via OSGi will no longer show up in calls to search the catalog; callers should use type registry instead. at this point most things are done except: * REST calls to catalog won't show things added via bundles (as they are in TypeRegistry) * TEMPLATE items aren't supported * LOCATION items aren't put/removed from BasicLocationRegistry (but I think that is no longer needed?) need to fix those then can declare victory on this!

ahgittin · 2017-07-03T16:29:40Z

This now addresses templates in TypeRegistry -- simply using a "catalog_template" tag. There was no immediate need for "deployable" or "final" semantics so those have not been added.

This also addresses a newly-found issue to do with locations: the location registry had a hokey relationship with the old catalog; it's now a cleaner facade. See comments in BasicLocationRegistry for this.

Finally I found a way to preserve the capability where the same BOM YAML is uploaded multiple times: if the bundle of the currently installed type was a wrapper, we now allow to replace it if the plan itself is unchanged. This removes the backwards compatibility issue introduced around uploading a BOM multiple times and it disallowing because the containing bundle was changed.

The REST API however still points at the old catalog routines, so it won't yet show items added through OSGi. This is a showstopper of course because all BOMs will be added through OSGi. Fix for that coming soon.

instead of the messy old way where catalog added/removed in the location registry; means it works for things added directly to TypeRegistry now (and those tests pass) one minor change to backwards compatibility, the internal LocationDefinition ID now contains the version. it isn't used and in other cases is a random string anyway. this will fix bugs where mulitple versions of the same location would result in just one being returned.

ahgittin · 2017-07-12T08:31:38Z

@geomacy comments thus far addressed, including CatalogBomScanner removed

geomacy

hi @ahgittin still working through this and hopefully will be finished soon, but I've enough of a question for you to make it worth adding this comment. Can you check out my remarks in CatalogBundleLoader and OsgiManager in particular and let me know what you think?

geomacy · 2017-07-13T15:13:26Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java

            LOG.debug("No BOM found in {} {} {}", CatalogUtils.bundleIds(bundle));
        }

        if (!applicationsPermitted.apply(bundle)) {


maybe we should even just get rid of this 'if' and should just let the bundle install apps?

the applications whitelist was a feature of the CatalogBomScanner but do we really need to do it? On thinking about it, why not just do without the new removeApplications method below, and let it install apps (templates)?

agree re whtielist, removed that.

am hesitant about removing some of these methods as i find when i do that there's always something using it. have added a warning and marked the legacy one deprecated but left enough that we can use it if we really have to, for one version,.

geomacy · 2017-07-13T15:23:26Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java

+     * to be either Strings or Maps of String -> String or bundles. Will skip items that are not.
     * <p>
     * If a string is supplied, this tries heuristically to identify whether a reference is a bundle or a URL, as follows:
     * - if the string contains a slash, it is treated as a URL (or classpath reference), e.g. <code>/file.txt</code>;


Comment at 394 references {@link RegisteredTypeNaming#isGoodTypeColonVersion(String)} which no longer exists

geomacy · 2017-07-13T16:17:58Z

core/src/main/java/org/apache/brooklyn/core/location/BasicLocationRegistry.java

    }

-    @Override
+    @Override @Deprecated


need a since comment here too

i omit them if it's covered by parent's javadoc

geomacy · 2017-07-14T08:29:51Z

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

+                    // (rebind is deferred, as are tests, but REST is not)
+                    MutableList<String> firstFive = MutableList.copyOf(Iterables.limit(result.catalogItemsInstalled, 5));
+                    log.info(result.message+", items: "+firstFive+
+                        (result.catalogItemsInstalled.size() > 5 ? " (and others, "+result.catalogItemsInstalled.size()+" total)" : "") );


log.debug the rest?

geomacy · 2017-07-14T08:44:18Z

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

    // (load order gets confusing with auto-scanning...)
-    public List<? extends CatalogItem<?,?>> loadCatalogBom(Bundle bundle) {
-        return MutableList.copyOf(loadCatalogBom(mgmt, bundle));
+    public List<? extends CatalogItem<?,?>> loadCatalogBomLegacy(Bundle bundle) {


This isn't called now that you've removed the CatalogBomScanner, so you can remove it and the method below as well, and get rid of the legacy parameter in loadCatalogBomInternal, and also remove legacy methods in CatalogBundleLoader.

geomacy · 2017-07-14T10:18:37Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java

     * @throws RuntimeException if the catalog items failed to be added to the catalog
     */
-    public Iterable<? extends CatalogItem<?, ?>> scanForCatalog(Bundle bundle) {
+    public Iterable<? extends CatalogItem<?, ?>> scanForCatalogLegacy(Bundle bundle) {


Now that you've removed CatalogBomScanner this method isn't used any more, I think we can get rid of it and the one below. See also comments on OsgiManager.java. I think we can also get rid of applicationsPermitted in this class, in the one place that its constructor is called the predicate is hardcoded to true. The interesting case is on line 98 below.

good idea, done

geomacy · 2017-07-14T10:50:19Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBundleLoader.java

+                LOG.warn("Bundle "+bundle+" containing BOM is not managed by Brooklyn; using legacy item installation");
+                legacy = true;
+            }
+            if (legacy) {


With the removal of the CatalogBomScanner I think the legacy parameter in this method's signature can be removed. This line is the interesting case where the code still calls the traditional catalog addItems() in the case where there is no managed bundle known in Brooklyn's management context. I'm wondering if this is still a path we need to support, or whether in fact now that the scanner is removed we can just call addTypesFromBundleBom in all cases within this method.

geomacy · 2017-07-14T15:53:08Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBomScanner.java

@@ -1,130 +0,0 @@
-/*


Need to remove it from blueprint.xml as well. Actually you would want to remove from line 122 to 131.

good spot; done

geomacy

Hi @ahgittin I've a couple more substantial observations added here....

geomacy · 2017-07-14T16:33:23Z

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

                osgiManager.managedBundlesRecord.addManagedBundle(result);
                result.code = OsgiBundleInstallationResult.ResultCode.INSTALLED_NEW_BUNDLE;
-                result.message = "Installed "+result.getMetadata().getVersionedName()+" with ID "+result.getMetadata().getId();
+                result.message = "Installed Brooklyn catalog bundle "+result.getMetadata().getVersionedName()+" with ID "+result.getMetadata().getId()+" ["+result.bundle.getBundleId()+"]";


this will happen if you install a catalog.bom which doesn't come from OSGi and which declare a bundle. we should be declaring a bundle in all BOMs. i've added this (some cheekily pushed to other projects).

the odd thing though is that the "delete empty BOMs" logic should be cleaning this up. i'm looking at that. (i think all other comments are addressed.)

I see, so we'll need a bundle declaration here in brooklyn-dist as well as in the server-cli in brooklyn-server.

@geomacy have pushed tests which confirm the logic to delete now-empty bundles is working correctly when it runs for items like this (the idea is if you add a second catalog with the same items as the first, the items become attached to the second so the first one is empty and we delete it).

i suspect in the problem you describe that for some reason it wasn't being triggered, either on startup or on rebind. if you added another BOM it would probably clean out the old items.

we could run the "delete empties" more often but i'm inclined not to given it's already a warning to add something without a bundle declared and there's a workaround.

I've added the bundle declaration at: apache/brooklyn-dist#99

A related recommended improvement would be to put catalog.bom into each of the brooklyn-library sub-projects, and ensure that those bundles are referenced and installed as brooklyn-managed bundles in the default.catalog.bom (or the library-*.bom it calls). Not that important however as we want to move away from those java library items in any case. More important for yaml bundles in blueprint repo to follow the new pattern.

geomacy · 2017-07-14T16:48:13Z

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

+                    // 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();


We'll need some more checks around this, or maybe a rethink of how it's done altogether - try deploying this bom file via the REST API:

brooklyn.catalog: id: hack bundle: org.apache.brooklyn.karaf-init version: 0.12.0.SNAPSHOT name: Hack itemType: template item: services: - type: org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess

and it will happily uninstall our karaf-init bundle; killing Brooklyn altogether (not the Karaf process). You can even do

brooklyn.catalog: id: hack bundle: org.apache.karaf.bundle.core version: 4.0.8 name: Hack itemType: template item: services: - type: org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess

which Karaf doesn't react well to.

GIGO -- adding a bundle should be (and is, at "add catalog" level, though we probably want finer grained) blockable by an RBAC scheme. but i don't think we need to prevent it at a code level, and someone who has permission to install bundles can break things in many ways, that's not something we need to (or can) prevent.

remove bom-scanner config, and declare bundle in catalog.bom

tidies to accompany apache/brooklyn-server#746 remove bom-scanner config, and declare bundle in catalog.bom

observed these in normal operation so bumping them down, with justification

geomacy · 2017-07-18T11:35:56Z

The build break on 98d9b0a is genuine, I get the same thing:

Failed tests: 
  ApplicationsYamlOsgiTest>ApplicationsYamlTest.testDoesNotWrapCatalogItemAndUsesCatalogName:216->ApplicationsYamlTest.assertDoesNotWrap:436 expected [catalogLevel] but found [Application (wiifyf67tn)]
  ApplicationsYamlOsgiTest>ApplicationsYamlTest.testNameInCatalogMetadata:364->ApplicationsYamlTest.assertDoesNotWrap:436 expected [My name in top-level] but found [Application (co8ot9tzt2)]
  ApplicationsYamlOsgiTest>ApplicationsYamlTest.testNameInItemMetadata:382->ApplicationsYamlTest.assertDoesNotWrap:436 expected [My name in item metadata] but found [Application (u1xjn8fsai)]
  ApplicationsYamlTest.testDoesNotWrapCatalogItemAndUsesCatalogName:216->assertDoesNotWrap:436 expected [catalogLevel] but found [Application (s5iotaeji9)]
  ApplicationsYamlTest.testNameInCatalogMetadata:364->assertDoesNotWrap:436 expected [My name in top-level] but found [Application (ojj1x9bq1v)]
  ApplicationsYamlTest.testNameInItemMetadata:382->assertDoesNotWrap:436 expected [My name in item metadata] but found [Application (hdis9rqcee)]
  CatalogOsgiYamlEntityTest.testOsgiNotLeakingToParent:539 Unexpected error message: java.lang.IllegalStateException: Bundle BasicManagedBundle{symbolicName=org.apache.brooklyn.test.osgi.entities.SimpleEntity, version=0.1.2.update, url=null} failed installation: Error installing catalog items: Failed to install org.apache.brooklyn.test.osgi.entities.SimpleEntity:0.1.2.update, types [BasicRegisteredType[org.apache.brooklyn.test.osgi.entities.SimpleEntity:0.1.2-update;null]] gave errors; 2 errors including: UnsupportedTypePlanException: Invalid plan; format could not be recognized, none of the available transformers [brooklyn-camp:CampTypePlanTransformer, java-type-name:JavaClassNameTypePlanTransformer] support BasicRegisteredType[org.apache.brooklyn.test.osgi.entities.SimpleEntity:0.1.2-update;null] expected [true] but found [false]
  CatalogYamlEntityNameTest.testUsesNameInCatalogItemMetadataIfNonSupplied:50 expected [nameInItemMetadata] but found [BasicEntity:faw9]
  CatalogYamlEntityOsgiTypeRegistryTest>CatalogYamlEntityTest.testAddCatalogItemVerySimple:60 expected 'services:' block: BasicRegisteredType[my.catalog.app.id.load:0.1.2;null]
type: org.apache.brooklyn.entity.stock.BasicEntity expected [true] but found [false]
  CatalogYamlEntityOsgiTypeRegistryTest>CatalogYamlEntityTest.testCreateSpecFromCatalogItem:371 » IllegalState
  CatalogYamlEntityTest.testAddCatalogItemVerySimple:60 expected 'services:' block: BasicRegisteredType[my.catalog.app.id.load:0.1.2;null]
type: org.apache.brooklyn.entity.stock.BasicEntity expected [true] but found [false]

…atalog spec transformer andre-enable camp items in rebind-osgi tests a different way

ahgittin · 2017-07-18T12:43:59Z

thanks @geomacy - let's watch this run, hopefully tests now all good!

ahgittin · 2017-07-18T14:41:37Z

tests good now @geomacy

geomacy

@ahgittin have been through this all and run some tests, but would like to get some more testing done before we merge.

A couple of typos and such below.

One question - now if you delete something from the catalog that you added in a bom, it only stays deleted until the next restart (as the whole catalog.bom gets read again and the entity that was deleted is still in it). How should we handle this - say in the REST delete call, start throwing exceptions in such cases rather than accept the delete? Or indeed just live with it for the time being, until we can introduce a bundle level delete?

geomacy · 2017-07-17T15:31:08Z

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

+            List<RegisteredType> itemsRT = MutableList.of();
+            for (CatalogItem<?, ?> ci: items) {
+                RegisteredType rt = brooklyn().getTypeRegistry().get(ci.getId());
+                if (rt!=null) itemsRT.add(rt);


For clarity this might be better as itemsRT.add( rt != null ? rt : RegisteredTypes.of(ci)), or have { } round the limbs of the if

geomacy · 2017-07-17T16:43:23Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/CatalogTransformer.java

+        return iconUrl;
+    }
+
+    private static Set<Object> makeTags(EntitySpec<?> spec, RegisteredType item) {


This is basically a duplicate of the existing one, why not make it makeTags(EntitySpec<?> spec, Set<Object> sourceTags and call it as makeTags(spec, item.getTags()) or makeTags(spec, item.tags().getTags()) as appropriate.

this is done, just GH isn't noticing

geomacy · 2017-07-18T08:59:36Z

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

        if (!Objects.equal(getOsgiVersionString(), other.getOsgiVersionString())) return false;
        if (!Objects.equal(url, other.getUrl())) return false;
+        if (other instanceof BasicManagedBundle) {
+            // make equality symetricm, OsgiBunde equals this iff this equals OsgiBundle;


typo "symetricm"

geomacy · 2017-07-18T11:46:56Z

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

    @Override
    public int hashCode() {
+        // checksum deliberately omitted here to match with OsgiBundleWithUrl
        return Objects.hashCode(symbolicName, version, url);


should this read Objects.hashCode(symbolicName, getOsgiVersionString(), url) for consistency with the check that's done in equals()?

geomacy · 2017-07-18T11:49:50Z

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

+        if (!Objects.equal(aliases, other.aliases)) return false;
+        if (!Objects.equal(bundles, other.bundles)) return false;
+        if (!Objects.equal(containingBundle, other.containingBundle)) return false;
+        if (!Objects.equal(deprecated, other.deprecated)) return false;


missing if (!Objects.equal(description, other. description)) return false;

this is done, just GH isn't noticing

geomacy · 2017-07-18T12:07:06Z

core/src/main/java/org/apache/brooklyn/util/core/LoaderDispatcher.java

        public Maybe<Class<?>> tryLoadFrom(BrooklynClassLoadingContext loader, String className) {
            try {
-                return Maybe.<Class<?>>of(loader.loadClass(className));
+                // return Maybe.<Class<?>>of(loader.loadClass(className));


Don't think this commented line is needed.

geomacy · 2017-07-18T13:30:17Z

utils/common/src/main/java/org/apache/brooklyn/util/exceptions/Exceptions.java

        return propagate(msg, throwable, false);
    }

-    /** As {@link #propagate(String, Throwable)} but always re-wraps including the given message. */


should be propagate(Throwable)

geomacy · 2017-07-18T13:49:55Z

utils/common/src/main/java/org/apache/brooklyn/util/osgi/VersionedName.java


-    /** As {@link #equals(Object)} but accepting the argument as equal if versions are identical when injected to OSGi-valid versions */
+    /** As {@link #equals(Object)} but accepting the argument as equal 
+     * if versions are identical when injected to OSGi-valid versions,


do you mean when coerced to OSGi-valid versions (or "transformed"?)

geomacy

Couple more mini observations

geomacy · 2017-07-18T16:19:43Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java

+        }
+        subCatalog.addToClasspath(new String[] { "file:"+fJar.getAbsolutePath() });
+        Collection<CatalogItemDtoAbstract<?, ?>> result = scanAnnotationsInternal(mgmt, subCatalog, MutableMap.of("version", containingBundle.getSuppliedVersionString()), containingBundle);
+        fJar.delete();


Could be put into a finally for the try at 1056, i.e. do everything above in the try.

geomacy · 2017-07-18T16:20:26Z

core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java

+            } catch (FileNotFoundException e) {
+                throw Exceptions.propagate(e);
+            }
+            bf.delete();


put into a finally

ahgittin · 2017-07-19T13:54:37Z

thx @geomacy -- great comments. all addressed.

geomacy

@ahgittin looks good to me.

The only (sort of an) issue that I have noticed in further testing today is in the situation where you add a .bom which refers to a missing type; e.g. I have a sequence of boms defining entities base-software-process-n that depend on each other as b3 -> b2 -> b1 and installed first b1 then b3 (b2 missing). When adding b3 it correctly tells me

Unable to match plan item: Service[name=<null>,description=<null>,serviceType=base-software-process-2

But nevertheless base-software-process-3 does show up in the catalog listing after that. I assume it's being picked up for listing even though its type is UNRESOLVED.

If you try to deploy it the error you get is ReferencedUnresolvedTypeException: Reference to base-software-process-3:0.2-SNAPSHOT in plan but that type cannot be resolved (recursive plan or premature evaluation?), which is again correct.

I'm happy to merge this as-is or if you want to do something about the above, merge when ready, and the observation above can be fixed up subsequently.

ahgittin · 2017-07-19T16:00:37Z

@geomacy yes, there's an open question how to treat unresolved items, whether to uninstall the bundle (so it could be fixed) or leave them in the unresolved state; i think the latter (current beahviour) is pretty good, but we'll see what we find

as for deletion you're right there's no easy way to delete a bundle, that's a known issue, to be fixed soon. (you have to stop brooklyn and go into persisted state, or run some hairy groovy.)

so i think merging.

geomacy · 2017-07-19T16:10:13Z

👍 merging

@neykov

Auto wrap YAML BOMs as catalog bundles Wraps pure-YAML BOMs uploaded to catalog in a bundle, creating a name and version if needed. This allows us to see all items added as a unit. This will allow us to prefer loading co-defined items (though that's not fully done, still relies on preferring search path when doing item lookup), and to edit or remove that item as a set (crucially we now keep the BOM, previously we discarded it!). Docs changed to reflect this and explain differences in another PR. The basic wrapping was pretty simple but a surprising number of subtleties emerged which this fixes: * java scanning doesn't work in OSGi; docs and errors make this clear * impacts replacement semantics (now it does a hash on the bundle to make most things the same, though a re-post of a non-SNAPSHOT BOM that doesn't declare a bundle will now fail because the random bundle name is stored on the item as containingBundle and is different on a re-post; and needed extra work to remove leaked anonymous bundles) * had to change the way we set up the libraries / search path, to exclude this "bundle" (mainly so tests are unchanged, but also improves efficiency for simple BOMs; also the code is cleaner now) And there is one thing that emerged that I *haven't* fixed: on rebind it became vital to handle the situation where dependent bundles might not be loaded before the depender bundle; see failing test`testLongReferenceSequence`. Not entirely sure how to do this /cc @neykov @geomacy @aledsage. Depends on #743.

This reverts commit c02b3e3, reversing changes made to fe31292.

ahgittin added 11 commits June 27, 2017 11:13

add routine to create new temp bundle

97b50e4

support force when wrapping a BOM

f16bda9

new batch of tests around catalog versioning with OSGi

comments on BOM scanning

e32d74d

add library bundles directly, without munging yaml, and only if not w…

dc43f03

…rapper

Delete wrapper bundles who have no types installed.

480a0c7

There is no point to them (they aren't OSGi). Fixes bug where uploading the same BOM multiple times, could cause bundle leakage. And tidy logging, more tests.

restore test bundle counts, and resolve library URLs on install

21e93d2

Merge branch 'version-new-rules' into osgi-auto-wrap-yaml

12ea23b

ahgittin mentioned this pull request Jun 28, 2017

Catalog internal references #748

Merged

ahgittin added 4 commits June 28, 2017 16:50

Merge branch 'catalog-internal-references' into osgi-auto-wrap-yaml

41b9894

Merge branch 'catalog-internal-references' into 2

4318237

ahgittin added 4 commits June 30, 2017 14:08

roughly working to validate types in a separate pass, fixing the big …

bf31100

…problem quite a few holes to fill, but confirmed working in a few places. also makes sure not to detect and delete empty wrapper bundles until we've finished adding them.

misc fixes related to type-reg / osgi, making the tests work

02cfe50

some failing tests still: * still need to address TEMPLATEs in OSGi * deprecation / catalog.xml persistence

remove osgi deprecation, disable, and transform tests

580c00d

these are no longer supported; need to think through how to address

Merge branch 'master' into osgi-auto-wrap-yaml

915e3bf

ahgittin force-pushed the osgi-auto-wrap-yaml branch from 04da669 to 915e3bf Compare June 30, 2017 14:06

ahgittin added 3 commits June 30, 2017 17:03

Merge branch 'master' into osgi-auto-wrap-yaml

ce85007

ahgittin force-pushed the osgi-auto-wrap-yaml branch from 5840ff1 to be2e7f0 Compare July 3, 2017 16:30

support templates for RegisteredType instances, using tags

7a42cf8

ahgittin force-pushed the osgi-auto-wrap-yaml branch from be2e7f0 to 7a42cf8 Compare July 3, 2017 17:23

ahgittin added 2 commits July 12, 2017 09:20

address PR comments

d0183c2

remove CatalogBomScanner

5868124

geomacy reviewed Jul 14, 2017

View reviewed changes

ahgittin added 3 commits July 17, 2017 11:26

remove more of the legacy bom-scanning support

a9e770f

remove BomScanner reference in blueprint.xml

d6fd2ae

address misc PR comments

c39eb71

ahgittin added a commit to ahgittin/brooklyn-dist that referenced this pull request Jul 18, 2017

tidies to accompany apache/brooklyn-server#746

c5c5fa7

remove bom-scanner config, and declare bundle in catalog.bom

tests for deletion of empty wrapper bundles

98d9b0a

ahgittin force-pushed the osgi-auto-wrap-yaml branch from f9c1af3 to 98d9b0a Compare July 18, 2017 08:11

asfgit pushed a commit to apache/brooklyn-dist that referenced this pull request Jul 18, 2017

Closes #99

e53a9c9

tidies to accompany apache/brooklyn-server#746 remove bom-scanner config, and declare bundle in catalog.bom

tidy warnings

c2d9bff

observed these in normal operation so bumping them down, with justification

fix broken tests by reverting the increased flexibility to the java c…

c7034e6

…atalog spec transformer andre-enable camp items in rebind-osgi tests a different way

geomacy reviewed Jul 18, 2017

View reviewed changes

address review comments

97f2524

geomacy approved these changes Jul 19, 2017

View reviewed changes

ahgittin added a commit to ahgittin/brooklyn-server that referenced this pull request Jul 19, 2017

This closes apache#746

6581be4

asfgit merged commit 97f2524 into apache:master Jul 19, 2017

ahgittin mentioned this pull request Jul 21, 2017

Allow equivalent plans when replacing #772

Merged

Graeme-Miller pushed a commit to Graeme-Miller/brooklyn-server that referenced this pull request Aug 29, 2017

Revert "Closes apache#746"

2fc5dc8

This reverts commit c02b3e3, reversing changes made to fe31292.

Auto wrap YAML BOMs as catalog bundles #746

Auto wrap YAML BOMs as catalog bundles #746

Uh oh!

Conversation

ahgittin commented Jun 28, 2017

Uh oh!

ahgittin commented Jun 30, 2017

Uh oh!

ahgittin commented Jul 3, 2017

Uh oh!

ahgittin commented Jul 12, 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

geomacy commented Jul 18, 2017

Uh oh!

ahgittin commented Jul 18, 2017

Uh oh!

ahgittin commented Jul 18, 2017

Uh oh!

geomacy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jul 14, 2017 •

edited

Loading

geomacy Jul 14, 2017 •

edited

Loading

geomacy left a comment •

edited

Loading

geomacy Jul 18, 2017 •

edited

Loading

geomacy left a comment •

edited

Loading