Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

Conversation

@CMoH
Copy link
Contributor

@CMoH CMoH commented Nov 19, 2015

This is still work in progress, but I've decided to make the PR earlier on to achieve a quicker review process.

CMoH added 12 commits November 12, 2015 18:11
Enable pax-web WAR Extender to find and automatically install the web
application into the web container.
jersey-core-1.18.1 suffers from the split-package problem by providing
javax.ws.rs*, which can affect OSGi bundle resolution
This brings brooklyn's REST API online within karaf using jersey.
…ils-rest-swagger

swagger-core and swagger-jaxrs versions 1.5.3 have package
io.swagger.config, which leads to a split-package situation.
The selected workaround is to inline both these dependencies into
brooklyn-utils-rest-swagger and re-export them for higher level client
code. When brooklyn moves to guava-18.0 we can migrate to swagger 1.5.4,
which are properly bundled.
Fix the code detecting if we are running within an OSGi framework.
Allow 3rd party libs to inject the managment context into various
objects by renaming MagementContextInjectable.injectManagementContext()
to setManagementContext().
@CMoH
Copy link
Contributor Author

CMoH commented Nov 19, 2015

Currently brooklyn-rest-server can be properly deployed as a feature in karaf, integrated with pax-web WAR extender. This allows the use of Jersey as a plain servlet for the REST api. However, at this point the contents of REST resources is not initialized with the appropriate management context, and so are the servlet filters. For this reason, launching the rest server within karaf still crashes, but that will be done as soon as we find a clean solution to inject the management context into the servlet context.

brooklyn-jsgui is also deployable as a WAR. For the purpose of separating concerns, having multiple GUIs pluggable into the karaf container we must have the GUI and REST APIs deployable at configurable base URLs (or at least base paths on the same server).

CMoH added 4 commits November 19, 2015 20:44
In order to keep both the monolithic launcher and have the OSGi launcher
working we need a temporary solution that works both ways. This OSGi
antipattern should be removed as soon as client code gets better.
Rename injectManagementContext to setManagementContext
@hzbarcea
Copy link
Contributor

@CMoH I looked into this this weekend, builds ok, tests work, webui works with the old launcher.

However the karaf tests gave the following trace:

org.osgi.service.resolver.ResolutionException: Unable to resolve root: missing requirement [root] osgi.identity; osgi.identity=brooklyn-rest-server; type=karaf.feature; version="[0.9.0.SNAPSHOT,0.9.0.SNAPSHOT]"; filter:="(&(osgi.identity=brooklyn-rest-server)(type=karaf.feature)(version>=0.9.0.SNAPSHOT)(version<=0.9.0.SNAPSHOT))" [caused by: Unable to resolve brooklyn-rest-server/0.9.0.SNAPSHOT: missing requirement [brooklyn-rest-server/0.9.0.SNAPSHOT] osgi.identity; osgi.identity=org.apache.brooklyn.rest-server; type=osgi.bundle; version="[0.9.0.SNAPSHOT,0.9.0.SNAPSHOT]"; resolution:=mandatory [caused by: Unable to resolve org.apache.brooklyn.rest-server/0.9.0.SNAPSHOT: missing requirement [org.apache.brooklyn.rest-server/0.9.0.SNAPSHOT] osgi.wiring.package; filter:="(&(osgi.wiring.package=org.apache.brooklyn.camp.brooklyn)(version>=0.9.0)(!(version>=1.0.0)))" [caused by: Unable to resolve org.apache.brooklyn.camp/0.9.0.SNAPSHOT: missing requirement [org.apache.brooklyn.camp/0.9.0.SNAPSHOT] osgi.wiring.package; filter:="(osgi.wiring.package=org.python.google.common.collect)"]]]
        at org.apache.felix.resolver.ResolutionError.toException(ResolutionError.java:42)[org.apache.felix.framework-5.4.0.jar:]
        at org.apache.felix.resolver.ResolverImpl.resolve(ResolverImpl.java:235)[org.apache.felix.framework-5.4.0.jar:]
        at org.apache.felix.resolver.ResolverImpl.resolve(ResolverImpl.java:158)[org.apache.felix.framework-5.4.0.jar:]
        at org.apache.karaf.features.internal.region.SubsystemResolver.resolve(SubsystemResolver.java:220)[7:org.apache.karaf.features.core:4.0.3]
        at org.apache.karaf.features.internal.service.Deployer.deploy(Deployer.java:263)[7:org.apache.karaf.features.core:4.0.3]
        at org.apache.karaf.features.internal.service.FeaturesServiceImpl.doProvision(FeaturesServiceImpl.java:1079)[7:org.apache.karaf.features.core:4.0.3]
        at org.apache.karaf.features.internal.service.FeaturesServiceImpl$1.call(FeaturesServiceImpl.java:975)[7:org.apache.karaf.features.core:4.0.3]
        at java.util.concurrent.FutureTask.run(FutureTask.java:262)[:1.7.0_80]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)[:1.7.0_80]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)[:1.7.0_80]
        at java.lang.Thread.run(Thread.java:745)[:1.7.0_80]

Is this still WIP?

@CMoH
Copy link
Contributor Author

CMoH commented Nov 23, 2015

Well, before syncing up with master I had the rest-server and the jsgui working. Although there still is a problem for them to work together (see below), the rest-server was completely usable.

However, the new commits from master seem to expose jython classes in the bundle exported packages, so we're back again to the winrm problem. We need to integrate @splatch's work on brooklyn-itest, so other developers can see if their unrelated changes break the karaf assembly project, so this does not happen anymore.

The problem with integrating brooklyn-jsgui and brooklyn-rest-server is that neither of them is deployable as a separate karaf feature. They can only work together in the presence of the highly customized webcontext that is created by the BrooklynWebServer class in brooklyn-launcher (and reimplemented in various other projects for tests). This is another discussion that I want to start on the mailing list though, since it does not affect the progress within this PR.

CMoH added 2 commits November 23, 2015 18:42
Use brooklyn-rest-server and brooklyn-jsgui as main features, since they
pull in the whole tree.
New unrelated code added an unwanted dependency towards jython, that
causes the karaf assembly to break. Ignoring this dependency for now,
until winrm work and its jython dependency gets isolated in a proper
bundle.
@CMoH
Copy link
Contributor Author

CMoH commented Nov 24, 2015

Fixed; brooklyn-rest-server works fine within karaf.

From my perspective this can and should be merged.

@hzbarcea
Copy link
Contributor

Thanks @CMoH , I'll review and test this evening.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaks binary + source compatibility, but I guess it's fine as we've already have another precedent for this release. Worth adding to the release notes.

Out of curiosity, is this needed for OSGi injection, which library will be able to inject the context here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aries blueprints injects using setter methods. I had this working with CXF resources, where the management context gets injected by the framework. I've since gone on the jersey path, and I'm not sure how to achieve the same with jersey, so I made a proposal to migrate to CXF altogether.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's code in Brooklyn using python APIs should migrate it to guava instead of doing this. Worth adding to the maven api check as well, not the first time this has snuck in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far it seems to be because of winrm. Will be dropped once references to winrm are contained in brooklyn-software-winrm, for instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this must be resolved in a follow-up commit.

@neykov
Copy link
Member

neykov commented Nov 24, 2015

Looks good, some minor comments worth addressing.
Couldn't figure out how the ManagementContext sharing works, probably needs more work (as part of a next iteration)?

@CMoH
Copy link
Contributor Author

CMoH commented Nov 24, 2015

I'll address some. Thanks @neykov

CMoH added 2 commits November 24, 2015 23:43
The settings in pom.xml are inherited from the parent and not needed here.
javax-servlet-api.version is already provided by parent pom.
Slipped in while syncing with master
@hzbarcea
Copy link
Contributor

Reviewed, tested, manually tested the launcher and the ui. Looks good, merging. Thanks @CMoH and @neykov .

@asfgit asfgit merged commit b514adb into apache:master Nov 25, 2015
asfgit pushed a commit that referenced this pull request Nov 25, 2015
@CMoH CMoH deleted the karaf-container-2-jersey branch November 26, 2015 09:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants