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/11/01 17:47:14 UTC
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
GitHub user ahgittin opened a pull request:
https://github.com/apache/brooklyn-server/pull/873
Upgrade types and bundles as per bundle manifest headers
Follow on from #872 which actually applies the upgrade in most places.
One known gap, and possibly some unknown gaps, but the main cases are working.
Suggest revisiting post-YOML as that will make it easier to handle upgrades due to a single instantiation path (instead of the many we currently have).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ahgittin/brooklyn-server bundle-upgrade-actually-upgrade
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/873.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 #873
----
commit a3673e4baadf6054c738e97dd3aaa3f5cc29fcc2
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-10-31T17:14:36Z
store catalog upgrade instructions in osgi manager, with convenience in CatalogUpgrades
commit 8b470ae58e27d49d228b54613f8d8faca5d5d729
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-11-01T11:31:42Z
store catalog upgrades, and use when resolving persistence records
catalog upgrades now stored in TypeRegistry, with conveniences for finding upgrades
markers and comments for shifting to a "load registered type" semantics instead of "load java type"
still todo is use this when we come to deploy - as per other tests
commit 1453dbaadb20f108563335b082d54f247499a96d
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date: 2017-11-01T14:30:36Z
look at upgrade targets if type or bundle not found when rebinding instances and when deploying
this addresses most the places i found in the course of several tests.
there may well be others, and not all the places give good logging,
but those can be improved in the fullness of time
(and with yoml we have many fewer creation code paths so things get simpler!)
one thing that isn't addressed is within a persisted entity spec;
this unpacks the definition at deploy time and persists the unpacking,
rather than persisting the given entity spec yaml for use later.
i think this can be addressed later.
----
---
[GitHub] brooklyn-server issue #873: Upgrade types and bundles as per bundle manifest...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/873
retest this please
non-det failure:
```
Error Message
expected one element but was: <{myKey=myDefault}, {myKey=valIsV1}>
Stacktrace
java.lang.IllegalArgumentException: expected one element but was: <{myKey=myDefault}, {myKey=valIsV1}>
at org.apache.brooklyn.enricher.stock.UpdatingMapTest.testUpdateMapEmitsEventOnChange(UpdatingMapTest.java:126)
Standard Output
2017-11-01 17:53:23,945 INFO TESTNG INVOKING CONFIGURATION: "Surefire test" - @AfterMethod org.apache.brooklyn.core.test.BrooklynMgmtUnitTestSupport.tearDown()
2017-11-01 17:53:23,947 INFO Stopping application BasicApplicationImpl{id=o3zm3t4g8w}
2017-11-01 17:53:23,952 INFO Stopped application BasicApplicationImpl{id=o3zm3t4g8w}
2017-11-01 17:53:23,954 INFO TESTNG PASSED CONFIGURATION: "Surefire test" - @AfterMethod org.apache.brooklyn.core.test.BrooklynMgmtUnitTestSupport.tearDown() finished in 9 ms
2017-11-01 17:53:23,954 INFO TESTNG INVOKING CONFIGURATION: "Surefire test" - @BeforeMethod org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport.setUp()
2017-11-01 17:53:23,954 INFO Added external config supplier named 'brooklyn-demo-sample': org.apache.brooklyn.core.config.external.InPlaceExternalConfigSupplier@715fceaf
2017-11-01 17:53:23,963 INFO TESTNG PASSED CONFIGURATION: "Surefire test" - @BeforeMethod org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport.setUp() finished in 9 ms
2017-11-01 17:53:23,963 INFO TESTNG INVOKING: "Surefire test" - org.apache.brooklyn.enricher.stock.UpdatingMapTest.testUpdateMapEmitsEventOnChange()
2017-11-01 17:53:23,979 INFO TESTNG FAILED: "Surefire test" - org.apache.brooklyn.enricher.stock.UpdatingMapTest.testUpdateMapEmitsEventOnChange() finished in 15 ms
java.lang.IllegalArgumentException: expected one element but was: <{myKey=myDefault}, {myKey=valIsV1}>
at com.google.common.collect.Iterators.getOnlyElement(Iterators.java:317)
at com.google.common.collect.Iterables.getOnlyElement(Iterables.java:289)
at org.apache.brooklyn.enricher.stock.UpdatingMapTest.testUpdateMapEmitsEventOnChange(UpdatingMapTest.java:126)
```
---
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
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/873#discussion_r149347438
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BundleUpgradeParser.java ---
@@ -312,6 +335,90 @@ public static boolean contains(Iterable<VersionRangedName> names, VersionedName
public static boolean contains(VersionRangedName range, VersionedName name) {
return range.getSymbolicName().equals(name.getSymbolicName()) && range.getOsgiVersionRange().includes(name.getOsgiVersion());
}
+
+ @Beta
+ public static void storeInManagementContext(CatalogUpgrades catalogUpgrades, ManagementContext managementContext) {
+ ((BasicBrooklynTypeRegistry)managementContext.getTypeRegistry()).storeCatalogUpgradesInstructions(catalogUpgrades);
+ }
+
+ @Beta @Nonnull
+ public static CatalogUpgrades getFromManagementContext(ManagementContext managementContext) {
+ CatalogUpgrades result = ((BasicBrooklynTypeRegistry)managementContext.getTypeRegistry()).getCatalogUpgradesInstructions();
+ if (result!=null) {
+ return result;
+ }
+ return builder().build();
+ }
+
+ @Beta
+ public static String getBundleUpgradedIfNecessary(ManagementContext mgmt, String vName) {
+ if (vName==null) return null;
+ Maybe<OsgiManager> osgi = ((ManagementContextInternal)mgmt).getOsgiManager();
+ if (osgi.isAbsent()) {
+ // ignore upgrades if not osgi
+ return vName;
+ }
+ if (osgi.get().getManagedBundle(VersionedName.fromString(vName))!=null) {
+ // bundle installed, prefer that to upgrade
+ return vName;
+ }
+ return getItemUpgradedIfNecessary(mgmt, vName, (cu,vn) -> cu.getUpgradesForBundle(vn));
+ }
+ @Beta
+ public static String getTypeUpgradedIfNecessary(ManagementContext mgmt, String vName) {
+ if (vName==null || mgmt.getTypeRegistry().get(vName)!=null) {
+ // item found (or n/a), prefer that to upgrade
+ return vName;
+ }
+ // callers should use BrooklynTags.newUpgradedFromTag if creating a spec,
+ // then warn on instantiation, done only for entities currently.
+ // (yoml will allow us to have one code path for all the different creation routines.)
+ // persistence/rebind also warns.
+ return getItemUpgradedIfNecessary(mgmt, vName, (cu,vn) -> cu.getUpgradesForType(vn));
+ }
+
+ private interface LookupFunction {
+ public Set<VersionedName> lookup(CatalogUpgrades cu, VersionedName vn);
+ }
+ private static String getItemUpgradedIfNecessary(ManagementContext mgmt, String vName, LookupFunction lookupF) {
+ Set<VersionedName> r = lookupF.lookup(getFromManagementContext(mgmt), VersionedName.fromString(vName));
+ if (!r.isEmpty()) return r.iterator().next().toString();
+
+ if (log.isTraceEnabled()) {
+ log.trace("Could not find '"+vName+"' and no upgrades specified; subsequent failure or warning possible unless that is a direct java class reference");
+ }
+ return vName;
+ }
+
+ /** This method is used internally to mark places we need to update when we switch to persisting and loading
+ * registered type IDs instead of java types, as noted on RebindIteration.load */
+ @Beta
+ public static boolean markerForCodeThatLoadsJavaTypesButShouldLoadRegisteredType() {
+ // return true if we use registered types, and update callers not to need this method
+ return false;
+ }
+
+ @Beta
+ CatalogUpgrades withTypeCleared(VersionedName versionedName) {
+ return builder().addAll(this).clearUpgradesProvidedByType(versionedName).build();
+ }
+ @Beta
+ CatalogUpgrades withBundleCleared(VersionedName versionedName) {
+ return builder().addAll(this).clearUpgradesProvidedByType(versionedName).build();
--- End diff --
yes!
---
[GitHub] brooklyn-server issue #873: Upgrade types and bundles as per bundle manifest...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/873
Have patched the known gap around entity spec. Those tests now passing.
That required a surprising series of tweaks: to how EntitySpec is resolved/persisted, to how default values are coerced/resolved, and to which iterables are allowed to be traversed for deep resolution. All changes I think are good independent of each other - it just so happened the first exposed the second etc.
---
So now not aware of anything which isn't upgraded as far as we can. It is still a problem that we persist the _java_ type names rather than the _registered type_ names, so changes in an upgraded registered type definition are not actually picked up -- but at least we record the correct type name and look for java in the correct bundle. And many of the places we want to change to work with RT instead of JT are now easy to find for when we do that change (and tests are aware of change semantics) by modifying and searching for uses of:
CatalogUpgrades.markerForCodeThatLoadsJavaTypesButShouldLoadRegisteredType()
---
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
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/873#discussion_r149346988
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java ---
@@ -341,7 +371,7 @@ public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingC
public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) {
if (reader.hasMoreChildren()) {
Class<?> type = readClassType(reader, mapper);
-// Class<?> type2 = context.getRequiredType();
+ // could confirm it is subtype of context.getRequiredType()
--- End diff --
TODO to me implies a bit stronger intention, here it's a "not yet sure should we do"
---
[GitHub] brooklyn-server issue #873: Upgrade types and bundles as per bundle manifest...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on the issue:
https://github.com/apache/brooklyn-server/pull/873
I have tested rebinding using the [brooklyn-etcd](https://github.com/brooklyncentral/brooklyn-etcd) entity, where the following was added to its `pom.xml`:
```
<plugin>
<groupId>org.apache.felix</groupId>
<artifactId>maven-bundle-plugin</artifactId>
<extensions>true</extensions>
<configuration>
<instructions>
<Brooklyn-Catalog-Force-Remove-Bundles>*</Brooklyn-Catalog-Force-Remove-Bundles>
<Brooklyn-Catalog-Upgrade-For-Bundles>*</Brooklyn-Catalog-Upgrade-For-Bundles>
</instructions>
</configuration>
<executions>
<execution>
<id>bundle-manifest</id>
<phase>process-classes</phase>
<goals>
<goal>manifest</goal>
</goals>
</execution>
</executions>
</plugin>
```
and this appears to work.
---
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
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/873#discussion_r149052420
--- Diff: launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherRebindCatalogOsgiTest.java ---
@@ -740,20 +744,82 @@ public void testRebindUpgradeEntity() throws Exception {
Entity entity = Iterables.getOnlyElement( Iterables.getOnlyElement(launcherLast.getManagementContext().getApplications()).getChildren() );
// assert it was updated
+ Assert.assertEquals(entity.getCatalogItemId(), "simple-entity:2.0.0");
- Assert.assertEquals(entity.getCatalogItemId(), "simple-entity:1.0.0");
- // TODO when upgrades work at level of catalog item ID and bundle ID, do below instead of above
-// Assert.assertEquals(entity.getCatalogItemId(), "simple-entity:2.0.0");
+ if (CatalogUpgrades.markerForCodeThatLoadsJavaTypesButShouldLoadRegisteredType()) {
+ Assert.assertEquals(Entities.deproxy(entity).getClass().getName(), BasicEntityImpl.class.getName());
+ } else {
+ Assert.assertEquals(Entities.deproxy(entity).getClass().getName(), "com.example.brooklyn.test.osgi.entities.SimpleEntityImpl");
+ }
+ }
+
+ @Test
+ public void testRebindUpgradeReferencedEntityFromCatalogAndDeployment() throws Exception {
+ File initialBomFileV2 = prepForRebindRemovedItemTestReturningBomV2(false, true);
--- End diff --
I presume this should pass `CatalogUpgrades.markerForCodeThatLoadsJavaTypesButShouldLoadRegisteredType()` as the first arg, given that the `startupAssertions` will adjust its assertions based on that.
---
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
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/873#discussion_r149347708
--- Diff: launcher/src/test/java/org/apache/brooklyn/launcher/BrooklynLauncherRebindCatalogOsgiTest.java ---
@@ -740,20 +744,82 @@ public void testRebindUpgradeEntity() throws Exception {
Entity entity = Iterables.getOnlyElement( Iterables.getOnlyElement(launcherLast.getManagementContext().getApplications()).getChildren() );
// assert it was updated
+ Assert.assertEquals(entity.getCatalogItemId(), "simple-entity:2.0.0");
- Assert.assertEquals(entity.getCatalogItemId(), "simple-entity:1.0.0");
- // TODO when upgrades work at level of catalog item ID and bundle ID, do below instead of above
-// Assert.assertEquals(entity.getCatalogItemId(), "simple-entity:2.0.0");
+ if (CatalogUpgrades.markerForCodeThatLoadsJavaTypesButShouldLoadRegisteredType()) {
+ Assert.assertEquals(Entities.deproxy(entity).getClass().getName(), BasicEntityImpl.class.getName());
+ } else {
+ Assert.assertEquals(Entities.deproxy(entity).getClass().getName(), "com.example.brooklyn.test.osgi.entities.SimpleEntityImpl");
+ }
+ }
+
+ @Test
+ public void testRebindUpgradeReferencedEntityFromCatalogAndDeployment() throws Exception {
+ File initialBomFileV2 = prepForRebindRemovedItemTestReturningBomV2(false, true);
--- End diff --
yes
---
[GitHub] brooklyn-server issue #873: Upgrade types and bundles as per bundle manifest...
Posted by robertgmoss <gi...@git.apache.org>.
Github user robertgmoss commented on the issue:
https://github.com/apache/brooklyn-server/pull/873
LGTM
---
[GitHub] brooklyn-server issue #873: Upgrade types and bundles as per bundle manifest...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/873
Thanks @aledsage @robertgmoss - comments addressed and merged
---
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/873
---
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
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/873#discussion_r149034987
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java ---
@@ -341,7 +371,7 @@ public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingC
public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) {
if (reader.hasMoreChildren()) {
Class<?> type = readClassType(reader, mapper);
-// Class<?> type2 = context.getRequiredType();
+ // could confirm it is subtype of context.getRequiredType()
--- End diff --
Preference for a "TODO" prefix if the comment is about what we could do, rather than about what the code does
---
[GitHub] brooklyn-server pull request #873: Upgrade types and bundles as per bundle m...
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/873#discussion_r149049524
--- Diff: core/src/main/java/org/apache/brooklyn/core/typereg/BundleUpgradeParser.java ---
@@ -312,6 +335,90 @@ public static boolean contains(Iterable<VersionRangedName> names, VersionedName
public static boolean contains(VersionRangedName range, VersionedName name) {
return range.getSymbolicName().equals(name.getSymbolicName()) && range.getOsgiVersionRange().includes(name.getOsgiVersion());
}
+
+ @Beta
+ public static void storeInManagementContext(CatalogUpgrades catalogUpgrades, ManagementContext managementContext) {
+ ((BasicBrooklynTypeRegistry)managementContext.getTypeRegistry()).storeCatalogUpgradesInstructions(catalogUpgrades);
+ }
+
+ @Beta @Nonnull
+ public static CatalogUpgrades getFromManagementContext(ManagementContext managementContext) {
+ CatalogUpgrades result = ((BasicBrooklynTypeRegistry)managementContext.getTypeRegistry()).getCatalogUpgradesInstructions();
+ if (result!=null) {
+ return result;
+ }
+ return builder().build();
+ }
+
+ @Beta
+ public static String getBundleUpgradedIfNecessary(ManagementContext mgmt, String vName) {
+ if (vName==null) return null;
+ Maybe<OsgiManager> osgi = ((ManagementContextInternal)mgmt).getOsgiManager();
+ if (osgi.isAbsent()) {
+ // ignore upgrades if not osgi
+ return vName;
+ }
+ if (osgi.get().getManagedBundle(VersionedName.fromString(vName))!=null) {
+ // bundle installed, prefer that to upgrade
+ return vName;
+ }
+ return getItemUpgradedIfNecessary(mgmt, vName, (cu,vn) -> cu.getUpgradesForBundle(vn));
+ }
+ @Beta
+ public static String getTypeUpgradedIfNecessary(ManagementContext mgmt, String vName) {
+ if (vName==null || mgmt.getTypeRegistry().get(vName)!=null) {
+ // item found (or n/a), prefer that to upgrade
+ return vName;
+ }
+ // callers should use BrooklynTags.newUpgradedFromTag if creating a spec,
+ // then warn on instantiation, done only for entities currently.
+ // (yoml will allow us to have one code path for all the different creation routines.)
+ // persistence/rebind also warns.
+ return getItemUpgradedIfNecessary(mgmt, vName, (cu,vn) -> cu.getUpgradesForType(vn));
+ }
+
+ private interface LookupFunction {
+ public Set<VersionedName> lookup(CatalogUpgrades cu, VersionedName vn);
+ }
+ private static String getItemUpgradedIfNecessary(ManagementContext mgmt, String vName, LookupFunction lookupF) {
+ Set<VersionedName> r = lookupF.lookup(getFromManagementContext(mgmt), VersionedName.fromString(vName));
+ if (!r.isEmpty()) return r.iterator().next().toString();
+
+ if (log.isTraceEnabled()) {
+ log.trace("Could not find '"+vName+"' and no upgrades specified; subsequent failure or warning possible unless that is a direct java class reference");
+ }
+ return vName;
+ }
+
+ /** This method is used internally to mark places we need to update when we switch to persisting and loading
+ * registered type IDs instead of java types, as noted on RebindIteration.load */
+ @Beta
+ public static boolean markerForCodeThatLoadsJavaTypesButShouldLoadRegisteredType() {
+ // return true if we use registered types, and update callers not to need this method
+ return false;
+ }
+
+ @Beta
+ CatalogUpgrades withTypeCleared(VersionedName versionedName) {
+ return builder().addAll(this).clearUpgradesProvidedByType(versionedName).build();
+ }
+ @Beta
+ CatalogUpgrades withBundleCleared(VersionedName versionedName) {
+ return builder().addAll(this).clearUpgradesProvidedByType(versionedName).build();
--- End diff --
Should this instead call `clearUpgradesProvidedByBundle`?
---