You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2017/11/16 19:58:02 UTC
[GitHub] brooklyn-server pull request #899: [WIP] Fix force remove bundle referenced ...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/899
[WIP] Fix force remove bundle referenced by other bundle
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server fix-force-remove-bundle-referenced-by-other-bundle
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/899.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 #899
----
commit d33abf516a258b3985c0fede525b7a7e1e363776
Author: Aled Sage <al...@gmail.com>
Date: 2017-11-16T14:57:01Z
Refactor CatalogResourceTest
commit b5753501752ee52f27cc481858532825234278f6
Author: Aled Sage <al...@gmail.com>
Date: 2017-11-16T15:33:30Z
CatalogResourceTest: add more tests
commit 419f03f1eeae4d1093ca59f33950c3f7ff42055b
Author: Aled Sage <al...@gmail.com>
Date: 2017-11-16T18:47:54Z
Bundle-upgrade tests: tidies
commit 928c09925a0fbfa237d4cd4a9e050e9d69a2c712
Author: Aled Sage <al...@gmail.com>
Date: 2017-11-16T18:48:56Z
bundle-force-remove: don’t re-install if come across reference
----
---
[GitHub] brooklyn-server pull request #899: Fix force remove bundle referenced by oth...
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/899#discussion_r152286086
--- Diff: launcher/src/test/java/org/apache/brooklyn/launcher/AbstractBrooklynLauncherRebindTest.java ---
@@ -184,7 +190,87 @@ protected File newTmpBundle(Map<String, byte[]> files, VersionedName bundleName,
}
return bf;
}
+
+ BundleFile.Builder bundleBuilder() {
+ return new BundleFile.Builder(this);
+ }
+ protected static class BundleFile {
--- End diff --
nice. maybe make its own class?
---
[GitHub] brooklyn-server issue #899: Fix force remove bundle referenced by other bund...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
It's definitely inconsistent to give a 400 on `IGNORING_BUNDLE_AREADY_INSTALLED` but a 202 on `IGNORING_BUNDLE_FORCIBLY_REMOVED`. i don't think there's any compelling reason to make that be a 400 and we should change it now before 1.0.
I lean towards `201 Created` if we actually add the bundle, and a `200 OK` (or `202 Accepted` ?) if we've ignored but for a benign reason -- and apply this across the board. no difference if `force=true`.
`202` should only be used when we return a task object for tracking.
---
[GitHub] brooklyn-server issue #899: Fix force remove bundle referenced by other bund...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
looks great. really nice tests.
a few minor comments, that's all.
---
[GitHub] brooklyn-server pull request #899: Fix force remove bundle referenced by oth...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/899
---
[GitHub] brooklyn-server pull request #899: Fix force remove bundle referenced by oth...
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/899#discussion_r152285616
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiBundleInstallationResult.java ---
@@ -98,6 +102,12 @@ void setIgnoringAlreadyInstalled() {
message = "Bundle "+getMetadata().getVersionedName()+" already installed as "+getMetadata().getId();
}
+ void setIgnoringForciblyRemoved(VersionedName requestedBundle, Optional<VersionedName> replacementBundle) {
+ code = OsgiBundleInstallationResult.ResultCode.IGNORING_BUNDLE_FORCIBLY_REMOVED;
+ message = "Bundle "+requestedBundle+" forcibly removed, "
+ +(replacementBundle.isPresent() ? "upgraded to "+replacementBundle.get().getOsgiVersionString() : "no upgrade defined");
--- End diff --
use `Maybe` with `isPresentAndNotNull()` ?
---
[GitHub] brooklyn-server issue #899: Fix force remove bundle referenced by other bund...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
Oops, I meant `201 Created` rather than 202.
Agreed - I've changed it to return 200 (see most recent commits).
Can you review again please @ahgittin?
---
[GitHub] brooklyn-server pull request #899: Fix force remove bundle referenced by oth...
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/899#discussion_r152285416
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/ha/OsgiArchiveInstaller.java ---
@@ -437,13 +463,19 @@ private synchronized void close() {
}
} else {
// No such Brooklyn-managed bundle.
-
+
// Check if likely-looking bundle already installed to OSGi subsystem, but brooklyn not aware of it.
// This will often happen on a karaf restart where bundle was cached by karaf, so we need to allow it;
// can also happen if brooklyn.libraries references an existing bundle.
//
// If we're not certain that the bundle is identical
+ // First check if this bundle is forcibly removed (and optionally upgraded).
+ // If so, don't install it - return the replacement, if any.
+ Maybe<VersionedName> forcedReplacementBundle = CatalogUpgrades.tryGetBundleForcedReplaced(mgmt(), inferredMetadata.getVersionedName());
+ if (forcedReplacementBundle.isPresent()) {
+ return generateForciblyRemovedResult(inferredMetadata.getVersionedName(), Optional.fromNullable(forcedReplacementBundle.get()));
--- End diff --
suggest pass `Maybe` in second argument, ie `forcedReplacementBundle`, and use `Maybe.isPresentAndNotNull()`
---
[GitHub] brooklyn-server issue #899: [WIP] Fix force remove bundle referenced by othe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
FAILURE
4515 tests run, 0 skipped, 0 failed.
--none--
---
[GitHub] brooklyn-server issue #899: Fix force remove bundle referenced by other bund...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
@ahgittin One thing I haven’t done is to behave differently if `?force=true` is used (to fail, rather than just returning a code that indicates their request to add the bundle was ignored because it has been forcibly removed). Do you think it’s worth adding that?
On a similar note, `CatalogResource.createFromArchive()` turns a `IGNORING_BUNDLE_AREADY_INSTALLED` into a 400. The 400 feels wrong, but given we’re explicitly doing that in the code then feels like we should probably leave it and not change the behaviour.
I’ve *not* done the same for `IGNORING_BUNDLE_FORCIBLY_REMOVED`, so that will return a 202 I think. That feels wrong - we didn’t create anything. I’m tempted to change that to give a 400 as well. This would give the same error whether or not `force` was supplied.
Thoughts?
---
[GitHub] brooklyn-server issue #899: Fix force remove bundle referenced by other bund...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
perfect, merging
---
[GitHub] brooklyn-server pull request #899: Fix force remove bundle referenced by oth...
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/899#discussion_r152321945
--- Diff: launcher/src/test/java/org/apache/brooklyn/launcher/AbstractBrooklynLauncherRebindTest.java ---
@@ -184,7 +190,87 @@ protected File newTmpBundle(Map<String, byte[]> files, VersionedName bundleName,
}
return bf;
}
+
+ BundleFile.Builder bundleBuilder() {
+ return new BundleFile.Builder(this);
+ }
+ protected static class BundleFile {
--- End diff --
It's currently tied to `AbstractBrooklynLauncherRebindTest`: it uses `test.newTmpBundle(...)`, which stores the created bundle file in the field `tmpFiles` so that they are deleted in `tearDown`.
Might well be worth extracting this into a utility that is re-used across multiple tests instead. I'll stick with it as-is for now, until we want to re-use it somewhere else, and we break that `tmpFiles` link.
---
[GitHub] brooklyn-server issue #899: [WIP] Fix force remove bundle referenced by othe...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
retest this please
jenkins timed out (the final test being run started within a few seconds of the suite being aborted, so I don't think it's caused by that test hanging):
```
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.19.1:test (default-test) on project brooklyn-rest-server: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:2.19.1:test failed: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
```
The jenkins console also showed:
```
Could not update commit status of the Pull Request on GitHub.
org.kohsuke.github.HttpException: Server returned HTTP response code: 201, message: 'Created' for URL: https://api.github.com/repos/apache/brooklyn-server/statuses/353bc5408115121da6ce95093e3891b021fd2ccb
at org.kohsuke.github.Requester.parse(Requester.java:633)
at org.kohsuke.github.Requester.parse(Requester.java:594)
at org.kohsuke.github.Requester._to(Requester.java:272)
at org.kohsuke.github.Requester.to(Requester.java:234)
at org.kohsuke.github.GHRepository.createCommitStatus(GHRepository.java:1071)
at org.jenkinsci.plugins.ghprb.extensions.status.GhprbSimpleStatus.createCommitStatus(GhprbSimpleStatus.java:272)
at org.jenkinsci.plugins.ghprb.extensions.status.GhprbSimpleStatus.onBuildComplete(GhprbSimpleStatus.java:231)
at org.jenkinsci.plugins.ghprb.GhprbBuilds.onCompleted(GhprbBuilds.java:192)
at org.jenkinsci.plugins.ghprb.GhprbBuildListener.onCompleted(GhprbBuildListener.java:26)
at hudson.model.listeners.RunListener.fireCompleted(RunListener.java:211)
at hudson.model.Run.execute(Run.java:1782)
at hudson.maven.MavenModuleSetBuild.run(MavenModuleSetBuild.java:543)
at hudson.model.ResourceController.execute(ResourceController.java:97)
at hudson.model.Executor.run(Executor.java:419)
Caused by: java.io.IOException: Failed to deserialize {"url":"https://api.github.com/repos/apache/brooklyn-server/statuses/353bc5408115121da6ce95093e3891b021fd2ccb","id":4325058854,"state":"failure","description":"FAILURE\n 4515 tests run, 0 skipped, 0 failed.","target_url":"https://builds.apache.org/job/brooklyn-server-pull-requests/2771/","context":"Jenkins: brooklyn-server-pull-request","created_at":"2017-11-17T17:06:31Z","updated_at":"2017-11-17T17:06:31Z","creator":{"login":"asfgit","id":1341245,"avatar_url":"https://avatars1.githubusercontent.com/u/1341245?v=4","gravatar_id":"","url":"https://api.github.com/users/asfgit","html_url":"https://github.com/asfgit","followers_url":"https://api.github.com/users/asfgit/followers","following_url":"https://api.github.com/users/asfgit/following{/other_user}","gists_url":"https://api.github.com/users/asfgit/gists{/gist_id}","starred_url":"https://api.github.com/users/asfgit/starred{/owner}{/repo}","subscriptions_url":"https://api.githu
b.com/users/asfgit/subscriptions","organizations_url":"https://api.github.com/users/asfgit/orgs","repos_url":"https://api.github.com/users/asfgit/repos","events_url":"https://api.github.com/users/asfgit/events{/privacy}","received_events_url":"https://api.github.com/users/asfgit/received_events","type":"User","site_admin":false}}
at org.kohsuke.github.Requester.parse(Requester.java:618)
... 13 more
Caused by: com.fasterxml.jackson.databind.JsonMappingException: Numeric value (4325058854) out of range of int
at [Source: {"url":"https://api.github.com/repos/apache/brooklyn-server/statuses/353bc5408115121da6ce95093e3891b021fd2ccb","id":4325058854,"state":"failure","description":"FAILURE\n 4515 tests run, 0 skipped, 0 failed.","target_url":"https://builds.apache.org/job/brooklyn-server-pull-requests/2771/","context":"Jenkins: brooklyn-server-pull-request","created_at":"2017-11-17T17:06:31Z","updated_at":"2017-11-17T17:06:31Z","creator":{"login":"asfgit","id":1341245,"avatar_url":"https://avatars1.githubusercontent.com/u/1341245?v=4","gravatar_id":"","url":"https://api.github.com/users/asfgit","html_url":"https://github.com/asfgit","followers_url":"https://api.github.com/users/asfgit/followers","following_url":"https://api.github.com/users/asfgit/following{/other_user}","gists_url":"https://api.github.com/users/asfgit/gists{/gist_id}","starred_url":"https://api.github.com/users/asfgit/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asfgit/subscriptions","organi
zations_url":"https://api.github.com/users/asfgit/orgs","repos_url":"https://api.github.com/users/asfgit/repos","events_url":"https://api.github.com/users/asfgit/events{/privacy}","received_events_url":"https://api.github.com/users/asfgit/received_events","type":"User","site_admin":false}}; line: 1, column: 127]
at [Source: {"url":"https://api.github.com/repos/apache/brooklyn-server/statuses/353bc5408115121da6ce95093e3891b021fd2ccb","id":4325058854,"state":"failure","description":"FAILURE\n 4515 tests run, 0 skipped, 0 failed.","target_url":"https://builds.apache.org/job/brooklyn-server-pull-requests/2771/","context":"Jenkins: brooklyn-server-pull-request","created_at":"2017-11-17T17:06:31Z","updated_at":"2017-11-17T17:06:31Z","creator":{"login":"asfgit","id":1341245,"avatar_url":"https://avatars1.githubusercontent.com/u/1341245?v=4","gravatar_id":"","url":"https://api.github.com/users/asfgit","html_url":"https://github.com/asfgit","followers_url":"https://api.github.com/users/asfgit/followers","following_url":"https://api.github.com/users/asfgit/following{/other_user}","gists_url":"https://api.github.com/users/asfgit/gists{/gist_id}","starred_url":"https://api.github.com/users/asfgit/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asfgit/subscriptions","organi
zations_url":"https://api.github.com/users/asfgit/orgs","repos_url":"https://api.github.com/users/asfgit/repos","events_url":"https://api.github.com/users/asfgit/events{/privacy}","received_events_url":"https://api.github.com/users/asfgit/received_events","type":"User","site_admin":false}}; line: 1, column: 117] (through reference chain: org.kohsuke.github.GHCommitStatus["id"])
at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:388)
at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:348)
at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.wrapAndThrow(BeanDeserializerBase.java:1607)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:278)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:140)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3798)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2842)
at org.kohsuke.github.Requester.parse(Requester.java:616)
... 13 more
Caused by: com.fasterxml.jackson.core.JsonParseException: Numeric value (4325058854) out of range of int
at [Source: {"url":"https://api.github.com/repos/apache/brooklyn-server/statuses/353bc5408115121da6ce95093e3891b021fd2ccb","id":4325058854,"state":"failure","description":"FAILURE\n 4515 tests run, 0 skipped, 0 failed.","target_url":"https://builds.apache.org/job/brooklyn-server-pull-requests/2771/","context":"Jenkins: brooklyn-server-pull-request","created_at":"2017-11-17T17:06:31Z","updated_at":"2017-11-17T17:06:31Z","creator":{"login":"asfgit","id":1341245,"avatar_url":"https://avatars1.githubusercontent.com/u/1341245?v=4","gravatar_id":"","url":"https://api.github.com/users/asfgit","html_url":"https://github.com/asfgit","followers_url":"https://api.github.com/users/asfgit/followers","following_url":"https://api.github.com/users/asfgit/following{/other_user}","gists_url":"https://api.github.com/users/asfgit/gists{/gist_id}","starred_url":"https://api.github.com/users/asfgit/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/asfgit/subscriptions","organi
zations_url":"https://api.github.com/users/asfgit/orgs","repos_url":"https://api.github.com/users/asfgit/repos","events_url":"https://api.github.com/users/asfgit/events{/privacy}","received_events_url":"https://api.github.com/users/asfgit/received_events","type":"User","site_admin":false}}; line: 1, column: 127]
at com.fasterxml.jackson.core.JsonParser._constructError(JsonParser.java:1702)
at com.fasterxml.jackson.core.base.ParserMinimalBase._reportError(ParserMinimalBase.java:558)
at com.fasterxml.jackson.core.base.ParserBase.convertNumberToInt(ParserBase.java:928)
at com.fasterxml.jackson.core.base.ParserBase._parseIntValue(ParserBase.java:866)
at com.fasterxml.jackson.core.base.ParserBase.getIntValue(ParserBase.java:694)
at com.fasterxml.jackson.databind.deser.std.NumberDeserializers$IntegerDeserializer.deserialize(NumberDeserializers.java:306)
at com.fasterxml.jackson.databind.deser.std.NumberDeserializers$IntegerDeserializer.deserialize(NumberDeserializers.java:286)
at com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:504)
at com.fasterxml.jackson.databind.deser.impl.FieldProperty.deserializeAndSet(FieldProperty.java:108)
at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:276)
... 17 more
Finished: ABORTED
```
---
[GitHub] brooklyn-server issue #899: [WIP] Fix force remove bundle referenced by othe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/brooklyn-server/pull/899
FAILURE
4936 tests run, 0 skipped, 1 failed.
--none--
---