You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2017/10/24 16:43:31 UTC
[GitHub] brooklyn-server pull request #868: WIP - allow types from different bundles ...
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/868
WIP - allow types from different bundles if equivalent
previously this was only allowed if older bundle was a wrapper, or if it was forced;
this could cause problems if such a disallowed addition was forced and then you tried to rebind!
now it stores the bundle from which a registered type comes, so we can sensibly have several types.
we could even allow them to be different - which would of course cause chaos for users,
but if we allow multi-tenant catalogs it will be essential (we simply have to filter for visible
types when looking at the registry, and prevent bundle name collisions - eg by prefixing with tenant)
though for now we don't allow them to be different.
needs testing and test cases still
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server types-disallow-same-type-different-contents-in-different-bundles
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/868.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #868
----
commit c380e6becf3100311620f569fd77b2f1facbc4a0
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-10-24T16:38:43Z
WIP - allow types from different bundles if equivalent
previously this was only allowed if older bundle was a wrapper, or if it was forced;
this could cause problems if such a disallowed addition was forced and then you tried to rebind!
now it stores the bundle from which a registered type comes, so we can sensibly have several types.
we could even allow them to be different - which would of course cause chaos for users,
but if we allow multi-tenant catalogs it will be essential (we simply have to filter for visible
types when looking at the registry, and prevent bundle name collisions - eg by prefixing with tenant)
though for now we don't allow them to be different.
needs testing and test cases still
----
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147362460
--- Diff: core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java ---
@@ -1492,7 +1503,7 @@ protected OsgiBundleInstallationResult addItemsOsgi(String yaml, boolean forceUp
OsgiBundleInstallationResult result = null;
try {
- result = osgiManager.get().install(null, new FileInputStream(bf), true, true, forceUpdate).get();
+ result = osgiManager.get().install(new BasicManagedBundle(vn.getSymbolicName(), vn.getVersionString(), null), new FileInputStream(bf), true, true, forceUpdate).get();
--- End diff --
agree re passing `OsgiManager` rather than it wrapped in `Maybe`, i've done that now
---
[GitHub] brooklyn-server issue #868: WIP - allow types from different bundles if equi...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
there's a few more subtle fixes in progress, but one i want to record here. consider:
1) i upload a BOM containing `type1` and `type2` without a bundle name, say it becomes `anonymous-brooklyn-wrapped-bom-bundle-11111:0.0.0-SNAPSHOT`
2) i update this and re-upload, with new definitions for the same types, say now the bundle gets given a name `anonymous-brooklyn-wrapped-bom-bundle-22222:0.0.0-SNAPSHOT`
in this case we _don't_ want to have multiple instances, and the expectation is that the previous types are replaced by this one, then `anonymous-bundle-11111` is noted as being empty (and it was noted as a wrapper bundle) so it is removed. so we should allow replacement of types even from different bundles _if_ the old one is a wrapper bundle.
but say now we
3) put `type1` in a full-fledged bundle call it `bund` (not a wrapper) and install that, with again a different plan body
if this is allowed to replace `type1` then we get into a potentially dangerous state on rebind, that if `bund` is read first and then `anonymous-bundle-22222`, the latter bundle will fail because it can't install an incompatible `type1`. we could try to load anonymous items first but not sure we can guarantee this (eg if `type1` depends on something that is loaded later). but if this _isn't_ allowed to replace `type1` then we make it hard to move away from anonymous bundles.
if `bund` includes both `type1` and `type2`, which it normally would, then there is no problem because the anonymous bundles will be deleted when we did (3), but it's quite hard to check if `bund` redeclares everything or just a subset from anonymous bundles.
what i think the resolution should be is that _only_ anonymous wrapper bundles can give different definitions for types from other anonymous wrapper bundles. this has two consequences:
(A) if you deploy anonymous BOMs which contain conflicting definitions of types and some of them don't contain all the types then you may see different definitions after rebind
(B) if you want to transition from a wrapped bundle to a full bundle, you must keep the types the same and change in a subsequent deployment (or bump the type's version which is probably wise anyway)
consqeuence (B) is fine i think. consequence (A) might be irritating but it doesn't break rebind and we discourage anonymous bundles so I think we can live with it.
@aledsage ?
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147119918
--- Diff: core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java ---
@@ -1492,7 +1503,7 @@ protected OsgiBundleInstallationResult addItemsOsgi(String yaml, boolean forceUp
OsgiBundleInstallationResult result = null;
try {
- result = osgiManager.get().install(null, new FileInputStream(bf), true, true, forceUpdate).get();
+ result = osgiManager.get().install(new BasicManagedBundle(vn.getSymbolicName(), vn.getVersionString(), null), new FileInputStream(bf), true, true, forceUpdate).get();
--- End diff --
has to be non-osgi version string -- that's the reason we pass in. previously it was inferring version from osgi bundle, then using it where the non-osgi version string was wanted.
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147090854
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -74,28 +92,40 @@ public BasicBrooklynTypeRegistry(ManagementContext mgmt) {
}
private Iterable<RegisteredType> getAllWithoutCatalog(Predicate<? super RegisteredType> filter) {
- // TODO thread safety
// TODO optimisation? make indexes and look up?
- return Iterables.filter(localRegisteredTypes.values(), filter);
+ return Locks.withLock(localRegistryLock.readLock(),
+ () -> localRegisteredTypesAndContainingBundles.values().stream().
+ flatMap(m -> m.values().stream()).filter(filter::apply).collect(Collectors.toList()) );
}
private Maybe<RegisteredType> getExactWithoutLegacyCatalog(String symbolicName, String version, RegisteredTypeLoadingContext constraint) {
- // TODO look in any nested/private registries
- RegisteredType item = localRegisteredTypes.get(symbolicName+":"+version);
+ RegisteredType item = Locks.withLock(localRegistryLock.readLock(),
+ ()-> getBestValue(localRegisteredTypesAndContainingBundles.get(symbolicName+":"+version)) );
return RegisteredTypes.tryValidate(item, constraint);
}
+ private RegisteredType getBestValue(Map<String, RegisteredType> m) {
+ if (m==null) return null;
+ if (m.isEmpty()) return null;
+ if (m.size()==1) return m.values().iterator().next();
+ // get the highest version of first alphabetical - to have a canonical order
+ return m.get( Ordering.from(VersionedNameStringComparator.INSTANCE).max(m.keySet()) );
--- End diff --
This probably feels ok for now, but it also feels like we can become a lot stricter (and thus simpler and more predictable):
* forbid conflicting types of the same version from getting into the catalog in the first place!
* prefer the type from our own bundle, or from our own dependencies declared in `brooklyn.libraries` (i.e. pass in the context of which bundle is looking it up).
I think you said you were deliberately supporting conflicting types from different bundles, because multi-tenancy could then be implemented just be at the bundle level. I think doing that makes our life more complicated and error prone in the medium term. And long-term we might well not implement multi-tenancy in that way.
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147092903
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -74,28 +92,40 @@ public BasicBrooklynTypeRegistry(ManagementContext mgmt) {
}
private Iterable<RegisteredType> getAllWithoutCatalog(Predicate<? super RegisteredType> filter) {
- // TODO thread safety
// TODO optimisation? make indexes and look up?
- return Iterables.filter(localRegisteredTypes.values(), filter);
+ return Locks.withLock(localRegistryLock.readLock(),
+ () -> localRegisteredTypesAndContainingBundles.values().stream().
+ flatMap(m -> m.values().stream()).filter(filter::apply).collect(Collectors.toList()) );
}
private Maybe<RegisteredType> getExactWithoutLegacyCatalog(String symbolicName, String version, RegisteredTypeLoadingContext constraint) {
- // TODO look in any nested/private registries
- RegisteredType item = localRegisteredTypes.get(symbolicName+":"+version);
+ RegisteredType item = Locks.withLock(localRegistryLock.readLock(),
+ ()-> getBestValue(localRegisteredTypesAndContainingBundles.get(symbolicName+":"+version)) );
return RegisteredTypes.tryValidate(item, constraint);
}
+ private RegisteredType getBestValue(Map<String, RegisteredType> m) {
+ if (m==null) return null;
+ if (m.isEmpty()) return null;
+ if (m.size()==1) return m.values().iterator().next();
+ // get the highest version of first alphabetical - to have a canonical order
+ return m.get( Ordering.from(VersionedNameStringComparator.INSTANCE).max(m.keySet()) );
+ }
+
@SuppressWarnings("deprecation")
@Override
public Iterable<RegisteredType> getMatching(Predicate<? super RegisteredType> filter) {
--- End diff --
Worth noting that the semantics of this method have changed, and same for `getAll`: you might now get multiple `RegisteredType`s with the same name:version. Normally those will be identical (just coming from different bundles), but it may not be guaranteed that they are identical!
---
[GitHub] brooklyn-server issue #868: WIP - allow types from different bundles if equi...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
there are failing tests this needs to revisit
```
CatalogOsgiVersionMoreEntityTest.testMoreEntityV1ThenV2GivesV2:181->AbstractYamlTest.addCatalogItems:248 » IllegalState
CatalogOsgiVersionMoreEntityTest.testMoreEntityV2ThenV1GivesV1:162->AbstractYamlTest.addCatalogItems:248 » IllegalState
CatalogOsgiYamlVersioningTest.testAddSameVersionWithoutBundleWorksIfItemIsSnapshot:62->assertJustOneBundle:97 Expected one bundle after installing the same; but had: [BasicManagedBundle{symbolicName=brooklyn-catalog-bom-lh1iWaI0, version=0.0.0.SNAPSHOT, url=null}, BasicManagedBundle{symbolicName=brooklyn-catalog-bom-XL3tI4t4, version=0.0.0.SNAPSHOT, url=null}] expected [true] but found [false]
CatalogYamlEntityOsgiTypeRegistryTest>CatalogYamlEntityTest.testReplacementFailureLeavesPreviousItemFromAnonymousBundleIntact:685->doTestReplacementFailureLeavesPreviousIntact:74->CatalogYamlEntityTest.doTestReplacementFailureLeavesPreviousIntact:725 Failed containsLiteral(DeliberatelyMissing): Bundle BasicManagedBundle{symbolicName=brooklyn-catalog-bom-tfLpTgDV, version=0.0.0.SNAPSHOT, url=null} failed installation: Error installing catalog items: Cannot add BasicRegisteredType[my.catalog.app.id.load:0.1.2-SNAPSHOT;brooklyn-catalog-bom-tfLpTgDV:0.0.0.SNAPSHOT] to catalog; it is different to BasicRegisteredType[my.catalog.app.id.load:0.1.2-SNAPSHOT;brooklyn-catalog-bom-hWIMdfBx:0.0.0.SNAPSHOT], and my.catalog.app.id.load:0.1.2-SNAPSHOT is defined in different bundle (throwing)
CatalogYamlRebindTest.testDeleteEmptyBundleRemovedFromPersistence:423 Collection has wrong size 4 (expected 2): [hcb07jab3e, hcb07jab3e.jar, jtom2vc1rq, jtom2vc1rq.jar]
```
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147079793
--- Diff: core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java ---
@@ -1492,7 +1503,7 @@ protected OsgiBundleInstallationResult addItemsOsgi(String yaml, boolean forceUp
OsgiBundleInstallationResult result = null;
try {
- result = osgiManager.get().install(null, new FileInputStream(bf), true, true, forceUpdate).get();
+ result = osgiManager.get().install(new BasicManagedBundle(vn.getSymbolicName(), vn.getVersionString(), null), new FileInputStream(bf), true, true, forceUpdate).get();
--- End diff --
Should we use `vn.getOsgiVersionString()`, given that it's the OSGi bundle's metadata?
---
[GitHub] brooklyn-server issue #868: allow types from different bundles if equivalent
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
Thanks @ahgittin - merging.
---
[GitHub] brooklyn-server issue #868: WIP - allow types from different bundles if equi...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
what do you mean by forbidding "the force-delete of part of a bundle" ? You mean "delete type X" ? it's only used by legacy rest call (which won't work very well, deleted item comes back after rebind), and by calling code iterating over a bundle to delete it, which is messy as it's unsynched, but good enough, and can be cleaned up later IMO (i considered adding a "delete bundle" method here but thought it out of scope).
--
i didn't worry about CME's but you're right, they'll be no fun if they happen IRL
---
[GitHub] brooklyn-server pull request #868: WIP - allow types from different bundles ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r146686839
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -498,10 +524,34 @@ public void delete(VersionedName type) {
// throw new NoSuchElementException("No catalog item found with id "+type);
}
+ /** removes the given registered type in the noted bundle;
+ * if not known in that bundle tries deleting from legacy catalog.
+ * @throws NoSuchElementException if not found */
public void delete(RegisteredType type) {
- delete(type.getVersionedName());
+ Map<String, RegisteredType> m = localRegisteredTypesAndContainingBundles.get(type.getId());
+ if (m!=null) {
+ RegisteredType removedItem = m.remove(type.getContainingBundle());
+ if (m.isEmpty()) {
+ synchronized (localRegisteredTypesAndContainingBundles) {
+ m = localRegisteredTypesAndContainingBundles.get(type.getId());
+ if (m!=null && m.isEmpty()) {
+ localRegisteredTypesAndContainingBundles.remove(type.getId());
+ } else {
+ // either removed in parallel or no longer empty
+ }
+ }
+ if (removedItem==null) {
+ throw new NoSuchElementException("Requested to delete "+type+" from "+type.getContainingBundle()+", "
+ + "but that type was not known in that bundle, it is in "+m.keySet()+" instead");
+ }
+ }
+ } else {
+ // try legacy delete
+ delete(type.getVersionedName());
--- End diff --
Feels dangerous (but a small race): another thread might be adding a `RegisteredType` (with same id, from another bundle) `localRegisteredTypesAndContainingBundles`. The `delete(VersionedName)` will then delete the thing added by the other thread.
Can we instead separate out the legacy deletion, so it doesn't try to also again delete from `localRegisteredTypesAndContainingBundles`?
---
[GitHub] brooklyn-server issue #868: WIP - allow types from different bundles if equi...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
above notes are implemented, and tests fixed, and concurrency issues resolved
given that we had several tests fail i'm comfortable with the level of testing this gets
ready for real review @aledsage
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/868
---
[GitHub] brooklyn-server issue #868: allow types from different bundles if equivalent
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
@ahgittin I wonder if it's time to "break backwards compatibility", in time for major release (Brooklyn 1.0.0) so that `bundle: ` is required when deploying an auto-wrapped .bom file! It's a simple thing for users to add to their existing .bom files, with the addition their .bom file would still work with 0.x, and we can make the error message very clear when they do a `br catalog add`.
If we did that, our lives would become a lot simpler I believe. If we don't do it now, then it's going to be harder to justify breaking backwards compatibility (to require `bundle: `) until we're doing a 2.0 release.
Thoughts? Would that make life a lot easier for us? (If you agree, then this is definitely one for the mailing list!).
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147359682
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -74,28 +92,40 @@ public BasicBrooklynTypeRegistry(ManagementContext mgmt) {
}
private Iterable<RegisteredType> getAllWithoutCatalog(Predicate<? super RegisteredType> filter) {
- // TODO thread safety
// TODO optimisation? make indexes and look up?
- return Iterables.filter(localRegisteredTypes.values(), filter);
+ return Locks.withLock(localRegistryLock.readLock(),
+ () -> localRegisteredTypesAndContainingBundles.values().stream().
+ flatMap(m -> m.values().stream()).filter(filter::apply).collect(Collectors.toList()) );
}
private Maybe<RegisteredType> getExactWithoutLegacyCatalog(String symbolicName, String version, RegisteredTypeLoadingContext constraint) {
- // TODO look in any nested/private registries
- RegisteredType item = localRegisteredTypes.get(symbolicName+":"+version);
+ RegisteredType item = Locks.withLock(localRegistryLock.readLock(),
+ ()-> getBestValue(localRegisteredTypesAndContainingBundles.get(symbolicName+":"+version)) );
return RegisteredTypes.tryValidate(item, constraint);
}
+ private RegisteredType getBestValue(Map<String, RegisteredType> m) {
+ if (m==null) return null;
+ if (m.isEmpty()) return null;
+ if (m.size()==1) return m.values().iterator().next();
+ // get the highest version of first alphabetical - to have a canonical order
+ return m.get( Ordering.from(VersionedNameStringComparator.INSTANCE).max(m.keySet()) );
+ }
+
@SuppressWarnings("deprecation")
@Override
public Iterable<RegisteredType> getMatching(Predicate<? super RegisteredType> filter) {
--- End diff --
It's not so much the semantics of the method as what is allowed in catalog
i'm updating release notes:
```
1. BOM files that do not declare a version now give the version of the bundle to their entities,
rather than the default 0.0.0-SNAPSHOT version.
1. Some catalog methods may return the same type multiple times, if contained in multiple bundles.
Previously only one of the bundle's definition of the type was returned.
Except for anonymous bundles it is no longer allowed to have give items with the same name and version.
(This is required to prevent Brooklyn from getting in to a state where it cannot rebind.)
```
---
[GitHub] brooklyn-server issue #868: allow types from different bundles if equivalent
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
retest this please
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147079416
--- Diff: core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java ---
@@ -1492,7 +1503,7 @@ protected OsgiBundleInstallationResult addItemsOsgi(String yaml, boolean forceUp
OsgiBundleInstallationResult result = null;
try {
- result = osgiManager.get().install(null, new FileInputStream(bf), true, true, forceUpdate).get();
+ result = osgiManager.get().install(new BasicManagedBundle(vn.getSymbolicName(), vn.getVersionString(), null), new FileInputStream(bf), true, true, forceUpdate).get();
--- End diff --
(Unrelated) Feels wrong that we pass into this method `Maybe<OsgiManager> osgiManager`, and then just call `osgiManager.get()` without checking it's present at all. Looking at its usage, the callers always check - so the method signature should be just `OsgiManager` instead of a maybe.
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147361992
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -368,13 +399,77 @@ public void addToLocalUnpersistedTypeRegistry(RegisteredType type, boolean canFo
if (!type.getId().equals(type.getSymbolicName()+":"+type.getVersion()))
Asserts.fail("Registered type "+type+" has ID / symname mismatch");
- RegisteredType oldType = mgmt.getTypeRegistry().get(type.getId());
- if (oldType==null || canForce || BrooklynVersionSyntax.isSnapshot(oldType.getVersion())) {
- log.debug("Inserting "+type+" into "+this);
- localRegisteredTypes.put(type.getId(), type);
- } else {
- assertSameEnoughToAllowReplacing(oldType, type);
- }
+ Locks.withLock(localRegistryLock.writeLock(),
+ () -> {
+ Map<String, RegisteredType> knownMatchingTypesByBundles = localRegisteredTypesAndContainingBundles.get(type.getId());
+ if (knownMatchingTypesByBundles==null) {
+ knownMatchingTypesByBundles = MutableMap.of();
+ localRegisteredTypesAndContainingBundles.put(type.getId(), knownMatchingTypesByBundles);
+ }
+
+ Set<String> oldContainingBundlesToRemove = MutableSet.of();
+ boolean newIsWrapperBundle = isWrapperBundle(type.getContainingBundle());
+ for (RegisteredType existingT: knownMatchingTypesByBundles.values()) {
+ String reasonForDetailedCheck = null;
+ boolean sameBundle = Objects.equals(existingT.getContainingBundle(), type.getContainingBundle());
+ boolean oldIsWrapperBundle = isWrapperBundle(existingT.getContainingBundle());
+ if (sameBundle || (oldIsWrapperBundle && newIsWrapperBundle)) {
+ // allow replacement (different plan for same type) if either
+ // it's the same bundle or the old one was a wrapper, AND
+ // either we're forced or in snapshot-land
+ if (!sameBundle) {
+ // if old is wrapper bundle, we have to to remove the old record
+ oldContainingBundlesToRemove.add(existingT.getContainingBundle());
+ }
+ if (canForce) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because force is on");
+ continue;
+ }
+ if (BrooklynVersionSyntax.isSnapshot(type.getVersion())) {
+ if (existingT.getContainingBundle()!=null) {
+ if (BrooklynVersionSyntax.isSnapshot(VersionedName.fromString(existingT.getContainingBundle()).getVersionString())) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because both are snapshot");
+ continue;
+ } else {
+ reasonForDetailedCheck = "the containing bundle "+existingT.getContainingBundle()+" is not a SNAPSHOT and addition is not forced";
+ }
+ } else {
+ // can this occur?
+ reasonForDetailedCheck = "the containing bundle of the type is unknown (cannot confirm it is snapshot)";
+ }
+ } else {
+ reasonForDetailedCheck = "the type is not a SNAPSHOT and addition is not forced";
+ }
+ } else if (oldIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in a named bundle replacing an item from an anonymous bundle-wrapped BOM, so definitions must be the same (or else give it a different version)";
+ } else if (newIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in an anonymous bundle-wrapped BOM replacing an item from a named bundle, so definitions must be the same (or else give it a different version)";
+ } else {
+ reasonForDetailedCheck = type.getId()+" is defined in different bundle";
+ }
+ assertSameEnoughToAllowReplacing(existingT, type, reasonForDetailedCheck);
+ }
+
+ log.debug("Inserting "+type+" into "+this+
+ (oldContainingBundlesToRemove.isEmpty() ? "" : " (removing entry from "+oldContainingBundlesToRemove+")"));
+ for (String oldContainingBundle: oldContainingBundlesToRemove) {
+ knownMatchingTypesByBundles.remove(oldContainingBundle);
+ }
+ knownMatchingTypesByBundles.put(type.getContainingBundle(), type);
+ });
+ }
+
+ private boolean isWrapperBundle(String bundleNameVersion) {
+ if (bundleNameVersion==null) return true;
+ Maybe<OsgiManager> osgi = ((ManagementContextInternal)mgmt).getOsgiManager();
+ // if not osgi, everything is treated as a wrapper bundle
+ if (osgi.isAbsent()) return true;
--- End diff --
that would be nice. yes, pseudo-osgi `OsgiManager` might be cleaner than checks throughout our code, depending how much smarts are needed there. alternatively it could spin up felix on demand.
other thing to consider is _only_ osgi, though that make make running individual IDE tests too cumbersome to set up and/or slow to run.
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147096369
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -368,13 +399,77 @@ public void addToLocalUnpersistedTypeRegistry(RegisteredType type, boolean canFo
if (!type.getId().equals(type.getSymbolicName()+":"+type.getVersion()))
Asserts.fail("Registered type "+type+" has ID / symname mismatch");
- RegisteredType oldType = mgmt.getTypeRegistry().get(type.getId());
- if (oldType==null || canForce || BrooklynVersionSyntax.isSnapshot(oldType.getVersion())) {
- log.debug("Inserting "+type+" into "+this);
- localRegisteredTypes.put(type.getId(), type);
- } else {
- assertSameEnoughToAllowReplacing(oldType, type);
- }
+ Locks.withLock(localRegistryLock.writeLock(),
+ () -> {
+ Map<String, RegisteredType> knownMatchingTypesByBundles = localRegisteredTypesAndContainingBundles.get(type.getId());
+ if (knownMatchingTypesByBundles==null) {
+ knownMatchingTypesByBundles = MutableMap.of();
+ localRegisteredTypesAndContainingBundles.put(type.getId(), knownMatchingTypesByBundles);
+ }
+
+ Set<String> oldContainingBundlesToRemove = MutableSet.of();
+ boolean newIsWrapperBundle = isWrapperBundle(type.getContainingBundle());
+ for (RegisteredType existingT: knownMatchingTypesByBundles.values()) {
+ String reasonForDetailedCheck = null;
+ boolean sameBundle = Objects.equals(existingT.getContainingBundle(), type.getContainingBundle());
+ boolean oldIsWrapperBundle = isWrapperBundle(existingT.getContainingBundle());
+ if (sameBundle || (oldIsWrapperBundle && newIsWrapperBundle)) {
+ // allow replacement (different plan for same type) if either
+ // it's the same bundle or the old one was a wrapper, AND
+ // either we're forced or in snapshot-land
+ if (!sameBundle) {
+ // if old is wrapper bundle, we have to to remove the old record
+ oldContainingBundlesToRemove.add(existingT.getContainingBundle());
+ }
+ if (canForce) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because force is on");
+ continue;
+ }
+ if (BrooklynVersionSyntax.isSnapshot(type.getVersion())) {
+ if (existingT.getContainingBundle()!=null) {
+ if (BrooklynVersionSyntax.isSnapshot(VersionedName.fromString(existingT.getContainingBundle()).getVersionString())) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because both are snapshot");
+ continue;
+ } else {
+ reasonForDetailedCheck = "the containing bundle "+existingT.getContainingBundle()+" is not a SNAPSHOT and addition is not forced";
+ }
+ } else {
+ // can this occur?
+ reasonForDetailedCheck = "the containing bundle of the type is unknown (cannot confirm it is snapshot)";
+ }
+ } else {
+ reasonForDetailedCheck = "the type is not a SNAPSHOT and addition is not forced";
+ }
+ } else if (oldIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in a named bundle replacing an item from an anonymous bundle-wrapped BOM, so definitions must be the same (or else give it a different version)";
+ } else if (newIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in an anonymous bundle-wrapped BOM replacing an item from a named bundle, so definitions must be the same (or else give it a different version)";
+ } else {
+ reasonForDetailedCheck = type.getId()+" is defined in different bundle";
+ }
+ assertSameEnoughToAllowReplacing(existingT, type, reasonForDetailedCheck);
+ }
+
+ log.debug("Inserting "+type+" into "+this+
+ (oldContainingBundlesToRemove.isEmpty() ? "" : " (removing entry from "+oldContainingBundlesToRemove+")"));
+ for (String oldContainingBundle: oldContainingBundlesToRemove) {
+ knownMatchingTypesByBundles.remove(oldContainingBundle);
--- End diff --
This is hard to follow - it looks reasonable, but it's hard to reason about because there are so many situations. Also the context/granularity of how this is used feels strange - other code like `BasicBrooklynCatalog.collectCatalogItemsFromItemMetadataBlock` is calling this repeatedly for each thing in a bundle (. If something goes wrong, it's then up to the caller to revert those actions. But this call side-effects what we had recorded against for other bundles. So hard to see how we get back to the previous state on such a rollback.
I come back to the idea of making `bundle: ` compulsory. That would simplify this code.
Longer term, I also think we want a "working view" of the `BrooklynTypeRegistry` that can apply a bunch of changes and then commit or abort them. But that's a bigger topic of conversation (completely separate from this PR!).
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147361400
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -368,13 +399,77 @@ public void addToLocalUnpersistedTypeRegistry(RegisteredType type, boolean canFo
if (!type.getId().equals(type.getSymbolicName()+":"+type.getVersion()))
Asserts.fail("Registered type "+type+" has ID / symname mismatch");
- RegisteredType oldType = mgmt.getTypeRegistry().get(type.getId());
- if (oldType==null || canForce || BrooklynVersionSyntax.isSnapshot(oldType.getVersion())) {
- log.debug("Inserting "+type+" into "+this);
- localRegisteredTypes.put(type.getId(), type);
- } else {
- assertSameEnoughToAllowReplacing(oldType, type);
- }
+ Locks.withLock(localRegistryLock.writeLock(),
+ () -> {
+ Map<String, RegisteredType> knownMatchingTypesByBundles = localRegisteredTypesAndContainingBundles.get(type.getId());
+ if (knownMatchingTypesByBundles==null) {
+ knownMatchingTypesByBundles = MutableMap.of();
+ localRegisteredTypesAndContainingBundles.put(type.getId(), knownMatchingTypesByBundles);
+ }
+
+ Set<String> oldContainingBundlesToRemove = MutableSet.of();
+ boolean newIsWrapperBundle = isWrapperBundle(type.getContainingBundle());
+ for (RegisteredType existingT: knownMatchingTypesByBundles.values()) {
+ String reasonForDetailedCheck = null;
+ boolean sameBundle = Objects.equals(existingT.getContainingBundle(), type.getContainingBundle());
+ boolean oldIsWrapperBundle = isWrapperBundle(existingT.getContainingBundle());
+ if (sameBundle || (oldIsWrapperBundle && newIsWrapperBundle)) {
+ // allow replacement (different plan for same type) if either
+ // it's the same bundle or the old one was a wrapper, AND
+ // either we're forced or in snapshot-land
+ if (!sameBundle) {
+ // if old is wrapper bundle, we have to to remove the old record
+ oldContainingBundlesToRemove.add(existingT.getContainingBundle());
+ }
+ if (canForce) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because force is on");
+ continue;
+ }
+ if (BrooklynVersionSyntax.isSnapshot(type.getVersion())) {
+ if (existingT.getContainingBundle()!=null) {
+ if (BrooklynVersionSyntax.isSnapshot(VersionedName.fromString(existingT.getContainingBundle()).getVersionString())) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because both are snapshot");
+ continue;
+ } else {
+ reasonForDetailedCheck = "the containing bundle "+existingT.getContainingBundle()+" is not a SNAPSHOT and addition is not forced";
+ }
+ } else {
+ // can this occur?
+ reasonForDetailedCheck = "the containing bundle of the type is unknown (cannot confirm it is snapshot)";
+ }
+ } else {
+ reasonForDetailedCheck = "the type is not a SNAPSHOT and addition is not forced";
+ }
+ } else if (oldIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in a named bundle replacing an item from an anonymous bundle-wrapped BOM, so definitions must be the same (or else give it a different version)";
+ } else if (newIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in an anonymous bundle-wrapped BOM replacing an item from a named bundle, so definitions must be the same (or else give it a different version)";
+ } else {
+ reasonForDetailedCheck = type.getId()+" is defined in different bundle";
+ }
+ assertSameEnoughToAllowReplacing(existingT, type, reasonForDetailedCheck);
+ }
+
+ log.debug("Inserting "+type+" into "+this+
+ (oldContainingBundlesToRemove.isEmpty() ? "" : " (removing entry from "+oldContainingBundlesToRemove+")"));
+ for (String oldContainingBundle: oldContainingBundlesToRemove) {
+ knownMatchingTypesByBundles.remove(oldContainingBundle);
--- End diff --
Reverting / rolling-back to undo side effects is no different to as before, is it? (Except there may be fewer types which need to be restored.)
Agree with complexity creep but feels better we take that pain than give it to users who would have to switch and change code for new mental model which still feels a little premature as discussed on ML. Plus I think once we nail this I expect we can leave it alone.
+1 to snapshotting type registry for rollback and other purposes, that's one of the reasons I prefer this approach: once legacy catalog is removed it should be much easier than with previous legacy catalog as it's just the local map of maps defined in this class which needs to be copied, two-levels deep; that removes a lot of pain around rollback (plus it could allow per-tenant views of types based on which bundles they are allowed to access)
---
[GitHub] brooklyn-server issue #868: allow types from different bundles if equivalent
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/868
all comments addressed @aledsage - one minor code change re `OsgiManager`
build seems stuck. passes locally (just checked) so i think merge now?
---
[GitHub] brooklyn-server pull request #868: WIP - allow types from different bundles ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r146684621
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -368,12 +370,51 @@ public void addToLocalUnpersistedTypeRegistry(RegisteredType type, boolean canFo
if (!type.getId().equals(type.getSymbolicName()+":"+type.getVersion()))
Asserts.fail("Registered type "+type+" has ID / symname mismatch");
- RegisteredType oldType = mgmt.getTypeRegistry().get(type.getId());
- if (oldType==null || canForce || BrooklynVersionSyntax.isSnapshot(oldType.getVersion())) {
- log.debug("Inserting "+type+" into "+this);
- localRegisteredTypes.put(type.getId(), type);
- } else {
- assertSameEnoughToAllowReplacing(oldType, type);
+ Map<String, RegisteredType> knownMatchingTypesByBundles = localRegisteredTypesAndContainingBundles.get(type.getId());
+ if (knownMatchingTypesByBundles!=null && !knownMatchingTypesByBundles.isEmpty()) {
+ for (RegisteredType existingT: knownMatchingTypesByBundles.values()) {
--- End diff --
Risks `ConcurrentModificationException`. Elsewhere it synchronizes on the (non-concurrent) `Map<String, RegisteredType` to do `put`, but here we iterate over it without synchronizing. Perhaps take a copy while synchronized on it, and then iterate over that copy? Or probably safer to just synchronize on the map for all of this block.
---
[GitHub] brooklyn-server pull request #868: allow types from different bundles if equ...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/868#discussion_r147098752
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java ---
@@ -368,13 +399,77 @@ public void addToLocalUnpersistedTypeRegistry(RegisteredType type, boolean canFo
if (!type.getId().equals(type.getSymbolicName()+":"+type.getVersion()))
Asserts.fail("Registered type "+type+" has ID / symname mismatch");
- RegisteredType oldType = mgmt.getTypeRegistry().get(type.getId());
- if (oldType==null || canForce || BrooklynVersionSyntax.isSnapshot(oldType.getVersion())) {
- log.debug("Inserting "+type+" into "+this);
- localRegisteredTypes.put(type.getId(), type);
- } else {
- assertSameEnoughToAllowReplacing(oldType, type);
- }
+ Locks.withLock(localRegistryLock.writeLock(),
+ () -> {
+ Map<String, RegisteredType> knownMatchingTypesByBundles = localRegisteredTypesAndContainingBundles.get(type.getId());
+ if (knownMatchingTypesByBundles==null) {
+ knownMatchingTypesByBundles = MutableMap.of();
+ localRegisteredTypesAndContainingBundles.put(type.getId(), knownMatchingTypesByBundles);
+ }
+
+ Set<String> oldContainingBundlesToRemove = MutableSet.of();
+ boolean newIsWrapperBundle = isWrapperBundle(type.getContainingBundle());
+ for (RegisteredType existingT: knownMatchingTypesByBundles.values()) {
+ String reasonForDetailedCheck = null;
+ boolean sameBundle = Objects.equals(existingT.getContainingBundle(), type.getContainingBundle());
+ boolean oldIsWrapperBundle = isWrapperBundle(existingT.getContainingBundle());
+ if (sameBundle || (oldIsWrapperBundle && newIsWrapperBundle)) {
+ // allow replacement (different plan for same type) if either
+ // it's the same bundle or the old one was a wrapper, AND
+ // either we're forced or in snapshot-land
+ if (!sameBundle) {
+ // if old is wrapper bundle, we have to to remove the old record
+ oldContainingBundlesToRemove.add(existingT.getContainingBundle());
+ }
+ if (canForce) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because force is on");
+ continue;
+ }
+ if (BrooklynVersionSyntax.isSnapshot(type.getVersion())) {
+ if (existingT.getContainingBundle()!=null) {
+ if (BrooklynVersionSyntax.isSnapshot(VersionedName.fromString(existingT.getContainingBundle()).getVersionString())) {
+ log.debug("Addition of "+type+" to replace "+existingT+" allowed because both are snapshot");
+ continue;
+ } else {
+ reasonForDetailedCheck = "the containing bundle "+existingT.getContainingBundle()+" is not a SNAPSHOT and addition is not forced";
+ }
+ } else {
+ // can this occur?
+ reasonForDetailedCheck = "the containing bundle of the type is unknown (cannot confirm it is snapshot)";
+ }
+ } else {
+ reasonForDetailedCheck = "the type is not a SNAPSHOT and addition is not forced";
+ }
+ } else if (oldIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in a named bundle replacing an item from an anonymous bundle-wrapped BOM, so definitions must be the same (or else give it a different version)";
+ } else if (newIsWrapperBundle) {
+ reasonForDetailedCheck = type.getId()+" is in an anonymous bundle-wrapped BOM replacing an item from a named bundle, so definitions must be the same (or else give it a different version)";
+ } else {
+ reasonForDetailedCheck = type.getId()+" is defined in different bundle";
+ }
+ assertSameEnoughToAllowReplacing(existingT, type, reasonForDetailedCheck);
+ }
+
+ log.debug("Inserting "+type+" into "+this+
+ (oldContainingBundlesToRemove.isEmpty() ? "" : " (removing entry from "+oldContainingBundlesToRemove+")"));
+ for (String oldContainingBundle: oldContainingBundlesToRemove) {
+ knownMatchingTypesByBundles.remove(oldContainingBundle);
+ }
+ knownMatchingTypesByBundles.put(type.getContainingBundle(), type);
+ });
+ }
+
+ private boolean isWrapperBundle(String bundleNameVersion) {
+ if (bundleNameVersion==null) return true;
+ Maybe<OsgiManager> osgi = ((ManagementContextInternal)mgmt).getOsgiManager();
+ // if not osgi, everything is treated as a wrapper bundle
+ if (osgi.isAbsent()) return true;
--- End diff --
[bigger topic than this PR, but raising it here while I'm thinking about it].
Now that osgi is the "only" way to run brooklyn (except for tests, and for developers using a quick launch in their IDE?), I wonder how we want to handle `osgi.isAbsent()` long term. For example, instead of handling it in our code in lots of places, perhaps tests should inject a pseudo-osgi-manager that just tracks bundle install/delete/find, and does a simple peek inside it for a catalog.bom at the root. Anything more complicated would fail-fast.
Let's discuss that separately, but it does feel like a way to simplify the production code (making our test framework code slightly more complicated instead).
---