Bug 988735 - Allow layered plugins to do their own type detection
Allow layered plugins to do their own type detection
Status: CLOSED CURRENTRELEASE
Product: RHQ Project
Classification: Other
Component: Plugins (Show other bugs)
4.8
Unspecified Unspecified
unspecified Severity high (vote)
: ---
: RHQ 4.9
Assigned To: John Mazzitelli
Mike Foley
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-26 05:30 EDT by Heiko W. Rupp
Modified: 2014-03-26 04:32 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-03-26 04:32:18 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Heiko W. Rupp 2013-07-26 05:30:34 EDT
Description of problem:

Right now the as7/eap6 plugin has some logic in 
org.rhq.modules.plugins.jbossas7.JBossProductType#determineJBossProductTypeViaProductConfFile
to determine what the layered type is. When a layered product wants to differentiate, then we have to patch the as7 plugin to update the JBossProdctType class.

It should be possible that the layered product is delivering that value itself, so that they can mix and match as they need without touching the base as7-plugin.
Comment 1 Simeon Pinder 2013-07-29 10:44:55 EDT
@Heiko, I agree(see below), but should this be a JON bug since rebranding is usually productization/rebranding/non-community?

Other than that, agree with descriptions. We should modify the logic and build process to instead retrieve the information from the EAP rebranding contents(https://docspace.corp.redhat.com/docs/DOC-124664),  files and determine product in that way and then only fall back on the JBossProductType logic for legacy plugins and managed resource discovery.  

I believe the JBossProductType.* logic was done because branding steps were not clearly defined.
Comment 2 Heiko W. Rupp 2013-08-20 09:44:15 EDT
See also Bug 997606, Bug 998973 and Bug 988894
Comment 3 John Mazzitelli 2013-08-20 10:36:22 EDT
Just off the top of my head, based on a call with thomas, heiko - perhaps we can do something like this in the XML descriptor

<callbacks>

   <!-- this tells the plugin container that whenever it runs a discovery for a resource type called "AS7 Standalone Server", it should pass the discovery details returned by that type's discovery component to MyDiscoveryCallback. MyDiscoveryCallback can then massage the results, tweeking it as necessary, and returning the discovery details. The MyDiscoveryCallback can opt to leave the discovered resource details alone and return them as-is -->

   <callback type="AS7 Standalone Server" callback="MyDiscoveryCallback" />

   <!-- this is like above, accept it is not type-specific. Instead, it is saying, anytime you are trying to discovery a type defined in my plugin descriptor (or one of my runs-inside types) using this particular discovery component class, then use MyDiscoveryCallback to again possibly massage that discovery class's returned details -->

   <callback discovery="jbossas7.SubsystemDiscovery" callback="MyDiscoveryCallback" />

</callbacks>
Comment 4 Heiko W. Rupp 2013-08-21 15:51:07 EDT
I think we may need 2 upcalls

- discovery helper: we pass in the process we found + an untyped map of properties we found (server base dir, server config, .... ) so that the embedded plugin can go in e.g. take the server config and deduct special directories to search for clues (like trying to find specific jars to inspect)

- post-discovery helper, that gets the DiscoveredResource(s) object(s) and can then say "yes that is me" and rewrite it if needed (e.g. set a different resource name) or return null to say "this is not me", where the PC would just take the original value(s).

Both callbacks do not need to be present and we need to make sure that we can grok multiple embedded plugins having such callback(s). Here we need to have rules what happens if more than one plugin claims that it is the chosen one.
Comment 5 John Mazzitelli 2013-08-26 14:49:42 EDT
(In reply to Heiko W. Rupp from comment #4)
> can grok multiple embedded plugins having such callback(s). Here we need to
> have rules what happens if more than one plugin claims that it is the chosen
> one.

This is going to be difficult because I'm not sure how we guarantee ordering of who gets called first. I think the embedded plugins should only mess with the discovered details if it ensures its only discovering unique details that it, itself, would only know. But I don't know how feasible that is or if there are any edge cases that should allow for us to accept multiple plugins returning different results. I don't even know what the semantics should be - is it that the first embedded plugin wins? The last one? And if the ordering isn't guaranteed, I'm not sure how that isn't arbitrary either (since the first may be last during different runs of the agent).
Comment 6 Heiko W. Rupp 2013-08-26 15:16:46 EDT
(In reply to John Mazzitelli from comment #5)
> This is going to be difficult because I'm not sure how we guarantee ordering
> of who gets called first. I think the embedded plugins should only mess with

We could call all the embedded plugins and check if more than one returns non-null and then abort.
It may be overkill, but on the other hand would expect that this could be quickly checked by deploying all plugins and then testing against the respective resources.

I agree that the "modify result" callback should only be offered to that plugin that says "this is my type".
Comment 7 John Mazzitelli 2013-08-30 15:56:25 EDT
git commit af523918b7e179c15bcae948cb8e7102e5072732

after talking to heiko, we might want to change the callback interface a bit. right now its:

void discoveredResources(Set<DiscoveredResourceDetails> discoveredDetails)

But it might be nicer to have the PC loop over all the details and pass them in one at a time:

boolean discoveredResources(DiscoveredResourceDetails details)

this would allow each callback to handle them one at a time and be able to return "true" if the callback wants to tell the PC "This is me so I handled this, you can stop calling other callbacks" or the PC could even keep going and if more than one callback wants to handle it, the PC could abort.

So I'm leaving this in the ON_DEV state.
Comment 8 John Mazzitelli 2013-09-03 11:01:05 EDT
So, I'm in the process of changing the code I commited earlier so that the callback method only gets one resource details passed into it (as opposed to the whole set of discovered resources) - this is so each callback has an opportunity to look at each one individually and can tell us about each individual resource if "it's him" or not.

Heiko and I discussed this and thought it a good thing if we abort the discovery if two or more callbacks return true in their callback method because this would be more than one callback claiming that a discovered resource belongs to them.

But I don't think we want to abort the entire discovery - because if more than one resource is discovered at the same time (that is, the set of details returned by the original discovery component has more than one resource) we will still want to keep the other resources that were discovered.

Heiko - we need to think about this some more. What should happen if more than one callback says the resource details belong to them?

As least we should not abort ALL the discovery - we should just remove the offending resource details from the discovered set of resources but continue with the rest normally.

Are we sure we should abort? Or should we just log a warning and continue, thus allowing more than one callback to change the details? I don't know what to do - I'm leaning towards removing (i.e. aborting) those offending details, but I'm questioning if there may actually be a use case where we could have two or more callbacks that want to adjust discovered details? What if we had a callback implementation whose job is to just modify all resource descriptions, but another callback whose job is to modify all plugin configuration properties? Are we going to shoot ourselves in the foot if we force an abort but later find out there is a use case where it is valid to have more than one callback adjust the discovered details?
Comment 9 John Mazzitelli 2013-09-03 17:25:46 EDT
git commit to master : b763d74
Comment 10 Thomas Segismont 2013-09-05 07:53:02 EDT
IMHO, logging a warning instead of aborting discovery is a better option because it doesn't narrow the possibilities for plugin developers and still allows our primary use case.

Otherwise I looked at the changes and they look good.

We still miss one feature though, because we need to allow plugin developers to veto some resources: for example the Infinispan plugin needs to disable the Infinispan subsystem included in the as7 plugin. Should this be addressed in another BZ?
Comment 11 John Mazzitelli 2013-09-05 09:21:40 EDT
(In reply to Thomas Segismont from comment #10)
> IMHO, logging a warning instead of aborting discovery is a better option
> because it doesn't narrow the possibilities for plugin developers and still
> allows our primary use case.

Yeah, that's what I questioned. And in fact, my uncertainty was so strong that I added a "backdoor" to turn that off. If you start the agent with that sysprop rhq.agent.discovery-callbacks.never-abort set to true, we do what you suggest - just log something but continue.

> We still miss one feature though, because we need to allow plugin developers
> to veto some resources: for example the Infinispan plugin needs to disable
> the Infinispan subsystem included in the as7 plugin. Should this be
> addressed in another BZ?

This could be easily addressed. We could add a new exception to the plugin API and if a callback determined some resource should be vetoed, the callback should throw that exception. The PC will catch that exception and immediately throw away the discovered details (it would be the same thing that is done when multiple callbacks process the details - but this would be a way for a callback to explicitly tell the PC to throw away the details).

I'll put this back ON_DEV and implement that. Very easy to do.
Comment 12 Thomas Segismont 2013-09-05 09:34:46 EDT
(In reply to John Mazzitelli from comment #11)
> This could be easily addressed. We could add a new exception to the plugin
> API and if a callback determined some resource should be vetoed, the
> callback should throw that exception. The PC will catch that exception and
> immediately throw away the discovered details (it would be the same thing
> that is done when multiple callbacks process the details - but this would be
> a way for a callback to explicitly tell the PC to throw away the details).

How about a new enum constant 'VETOED'  in addition to 'PROCESSED' and 'UNPROCESSED' instead of an exception?
Comment 13 John Mazzitelli 2013-09-05 14:55:52 EDT
added VETO enum. added integration test to confirm it works.

git commit to master: 3606bc232276f0a41ded2d646b7651f614fe8283

NOTE: I disabled the test DiscoveryCallbackVetoTest.testDiscoveryCallbacks because something is causing our arquillian setup to deploy another plugin from another test and its breaking this one. I'm trying to find the problem, but for now, disabling the test just so I can get this committed to master so its in the next GA. the test passes if I enable it and run it individually, so the test is functional. It just can't work in the full test suite.
Comment 14 John Mazzitelli 2013-09-05 18:09:01 EDT
git commit: 2b7ee84

i fixed the test, setting the priority seemed to magically get the other tests' plugins to not get deployed.
Comment 15 Heiko W. Rupp 2013-09-06 03:27:24 EDT
Changing the title, as this now applies to all plugins.

Initial wiki page is here https://docs.jboss.org/author/display/RHQ/Discovery+Callbacks
Comment 16 Heiko W. Rupp 2014-03-26 04:32:18 EDT
Bulk closing now that 4.10 is out.

If you think an issue is not resolved, please open a new BZ and link to the existing one.

Note You need to log in before you can comment on or make changes to this bug.