Bug 789420 - Plugin validation prevents multiple embedded imported servers; e.g. JMX Server
Plugin validation prevents multiple embedded imported servers; e.g. JMX Server
Status: ASSIGNED
Product: RHQ Project
Classification: Other
Component: Plugin Container (Show other bugs)
4.3
Unspecified Unspecified
medium Severity unspecified (vote)
: ---
: ---
Assigned To: John Mazzitelli
Mike Foley
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-10 13:38 EST by Elias Ross
Modified: 2012-03-16 09:26 EDT (History)
2 users (show)

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


Attachments (Terms of Use)
initial cut; allows for duplicate imported resources (30.57 KB, patch)
2012-02-29 15:26 EST, Elias Ross
no flags Details | Diff
Rebased on top of bug/747925 (30.36 KB, patch)
2012-03-01 16:25 EST, Elias Ross
no flags Details | Diff
amended patch (30.37 KB, patch)
2012-03-01 18:37 EST, Elias Ross
no flags Details | Diff

  None (edit)
Description Elias Ross 2012-02-10 13:38:08 EST
This is my plugin configuration (excerpt)

<?xml version="1.0"?>
<plugin name="hadoop"
        displayName="Hadoop Monitoring Plugin"
        description="Monitor various components of Hadoop Clusters; including Hive"
        package="org.rhq.plugins.hadoop"
        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xmlns="urn:xmlns:rhq-plugin"
        xmlns:c="urn:xmlns:rhq-configuration">

    <depends plugin="JMX" useClasses="true"/>

    <server name="SecondaryNameNode" discovery="HadoopDiscovery" class="HadoopComponent" singleton="true">
        <process-scan name="SecondaryNameNode" query="process|basename|match=^java.*,arg|org.apache.hadoop.hdfs.server.namenode.SecondaryNameNode|match=.*"/>
        <server name="SecondaryNameNode JVM" description="JVM of the Secondary NameNode"
              sourcePlugin="JMX" sourceType="JMX Server"
              discovery="org.rhq.plugins.jmx.InternalJMXServerDiscoveryComponent" class="org.rhq.plugins.jmx.JMXServerComponent"
              singleton="true"/>
    </server>

    <server name="NameNode" discovery="HadoopDiscovery" class="HadoopComponent" singleton="true">
        <process-scan name="NameNode" query="process|basename|match=^java.*,arg|org.apache.hadoop.hdfs.server.namenode.NameNode|match=.*"/>
        <server name="NameNode JVM" description="JVM of the NameNode"
              sourcePlugin="JMX" sourceType="JMX Server"
              discovery="org.rhq.plugins.jmx.InternalJMXServerDiscoveryComponent" class="org.rhq.plugins.jmx.JMXServerComponent"
              singleton="true"/>
    </server>


</plugin>

But this fails:

ERROR 10-02 10:29:58,814 (PluginMetadataManager.java:loadPlugin:138)  -Error transforming plugin descriptor [hadoop].
org.rhq.core.clientapi.agent.metadata.InvalidPluginDescriptorException: Type [ResourceType[id=0, category=Service, name=Operating System, plugin=hadoop]] is duplicate for this plugin. This is illegal.
        at org.rhq.core.clientapi.agent.metadata.PluginMetadataManager.loadPlugin(PluginMetadataManager.java:128)
        at org.rhq.plugins.hadoop.ComponentTest.processPluginDescriptor(ComponentTest.java:188)
        at org.rhq.plugins.hadoop.ComponentTest.before(ComponentTest.java:175)
        at org.rhq.plugins.hadoop.HadoopComponentTest.before(HadoopComponentTest.java:38)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)

The "Operating System" <service> is from the JMX plugin.

It doesn't make sense that it would be an issue: If I embed JMX plugin once then there is no issue.
Comment 1 Mike Foley 2012-02-13 11:59:32 EST
per triage 2/13/2012 (asantos, crouch, foley, loleary)
Comment 2 Elias Ross 2012-02-17 21:02:40 EST
I have a patch pending with unit tests etc. Needs company approval first. Needs some more testing as well.

For each service or server embedded:
1. Copies (clones) the ResourceType and adjusts per resource descriptor.
2. Uses the same ResourceType as children as from the imported resource and does not add them to the importer's plugin.

The big change is 2.

The changes may affect some existing deployments, because the child resource types are now reused and not recreated.

It would also make sense to create a change so that if the resource type declaration such as 'JMX' appeared without any 'name' element:

   <server name="Server"
           discovery="Discovery"
           class="Component"
           description="Description">

      <server
           sourcePlugin="JMX"
           sourceType="JMXServer"/> <!-- No name -->

    </server>

the same resource type would be reused as-is without creating yet another resource type in the database.
Comment 3 Elias Ross 2012-02-29 15:26:45 EST
Created attachment 566637 [details]
initial cut; allows for duplicate imported resources

Patch. Hasn't been extensively tested yet.
Comment 4 John Mazzitelli 2012-02-29 16:06:11 EST
(In reply to comment #3)
> Created attachment 566637 [details]
> initial cut; allows for duplicate imported resources
> 
> Patch. Hasn't been extensively tested yet.

I haven't looked deep into this patch yet, but there are going to be changes going into master soon that will create conflicts with this attached patch I think (see origin/bug/747925 branch - related to bug #747925). So you may have to resolve those conflicts and repost the patch once those changes get into master (which will probably be in the next day or two).

NOTE: RHQ code conventions require curly braces on all if-statements - whether or not the consequent clause is a single line or not.
    if (boolean) {
        doSomething();
    }
Comment 5 Elias Ross 2012-02-29 17:31:18 EST
Looking at that other issue, is this patch simply redundant?
Comment 6 John Mazzitelli 2012-02-29 20:28:43 EST
(In reply to comment #5)
> Looking at that other issue, is this patch simply redundant?

Good question. I assumed that this issue is different than bug #747925, but maybe not. You could checkout the bug/747925 branch, build it and test your scenario to see if its fixed or not.

This issue appears to me to talk about having MULTIPLE resource types in the same descriptor extend the same type (which is different than 747925 which dealt with *upgrading* already existing types that were extended using sourcePlugin/sourceType).

It is worth investigating if both issues had, at its core, the same bug.
Comment 7 Heiko W. Rupp 2012-03-01 04:06:20 EST
As mazz said, these are different (even if related) use cases.

Especially your case 2  (on top of case 1 )

<server
           sourcePlugin="JMX"
           sourceType="JMXServer"/> <!-- No name -->


would be a big win
Comment 8 John Mazzitelli 2012-03-01 07:23:13 EST
(In reply to comment #7)
> As mazz said, these are different (even if related) use cases.
> 
> Especially your case 2  (on top of case 1 )
> 
> <server
>            sourcePlugin="JMX"
>            sourceType="JMXServer"/> <!-- No name -->
> 
> 
> would be a big win

Well, I will say - the XML Schema makes the name attribute required - so, on the face of it, that was never valid XML anyway and it was always the intention to have the plugin author tell us explicitly the name (in other words, that particular issue is technically a request for an enhancement, not necessarily a bug).

You have to give the name to your resource types. I understand that here (based on that XML above) you just want to pick up the same name as the extended type and not have to repeat it in your XML, but the easy workaround is to just put the name= attribute in there with the same value as the extended type. So, that's not the issue I would think has a major severity. To me, if we can't extend the same type multiple times in the same descriptor, that is of major significance to fix. Not being able to omit the name and fallback to the parent extended type is minor and easily worked around.

For the record, I don't even know how to make the XML Schema validate that. I mean, we want resource types to have their name required under all other conditions - thus we still need the XML Schema to mark name attribute as "required". If you want it to be optional ONLY when sourcePlugin is a specified attribute, I'll have to crack open my XML Schema book to find out what tricks you can play to say, "require name attribute UNLESS attribute "sourcePlugin" is specified". I see in the attached patch there are no changes to the .xsd, so I assume that patch doesn't address this enhancement request (since the current .xsd requires name on resource type elements and thus that XML above is still not valid).
Comment 9 John Mazzitelli 2012-03-01 10:46:57 EST
I'm gonna at least write some unit tests to see if this is still broken on the bug/747925 branch.
Comment 10 John Mazzitelli 2012-03-01 12:01:32 EST
I just wrote a unit test for bug #747925 that has this in a plugin descriptor:

    <server name="OuterServerA">
        <server name="Child1ServerA"
                description="Child 1 Server type that extends Parent Server type"
                discovery="Child1DiscoveryComponent"
                class="Child1Component"
                sourcePlugin="MultiplePluginExtensionSinglePluginDescriptorMetadataParentTestPlugin"
                sourceType="ParentServerA">
        </server>
    </server>

    <server name="OuterServerB">
        <server name="Child2ServerA"
                description="Child 2 Server type that extends Parent Server type"
                discovery="Child2DiscoveryComponent"
                class="Child2Component"
                sourcePlugin="MultiplePluginExtensionSinglePluginDescriptorMetadataParentTestPlugin"
                sourceType="ParentServerA">
        </server>
    </server>

The test passes. You can see there is one plugin descriptor with two main types (OuterServerA and OuterServerB). Each has a child type - both of which are embedded extensions of the ParentServerA type from the parent plugin named "MultiplePluginExtensionSinglePluginDescriptorMetadataParentTestPlugin". As I understand this bugzilla issue, the above scenario is the main problem being reported. And my unit testing is showing that, in the bug/747925 branch at least, this is now working. Once I push the changes over to master and close 747925 I think this bug can be closed.
Comment 11 John Mazzitelli 2012-03-01 12:03:28 EST
one thing I noticed that my tests are NOT looking at, is children of the parent type. I'll write some more tests to make sure that is working at well.
Comment 12 John Mazzitelli 2012-03-01 13:57:19 EST
(In reply to comment #11)
> one thing I noticed that my tests are NOT looking at, is children of the parent
> type. I'll write some more tests to make sure that is working at well.

OK, I think the problem is due to the fact that the embedded type has a child service in it and (in conjunction with that) the plugin descriptor has two types that each embed that. So, this is definitely a problem and I have some test code over here showing it. I'll see if I can hunt it down, perhaps looking at the attached patch for ways to fix.
Comment 13 Elias Ross 2012-03-01 14:38:48 EST
(In reply to comment #8)

> You have to give the name to your resource types. I understand that here (based
> on that XML above) you just want to pick up the same name as the extended type
> and not have to repeat it in your XML, but the easy workaround is to just put
> the name= attribute in there with the same value as the extended type.

I wasn't really thinking leaving off the name in XML was a big win nor how to specify it, but I saw that RHQ was basically creating a (partial) copy of the resource type A, minus its descriptor elements, so any time you created a copy of a resource you would lose its default configuration, metrics, etc. So the big win is avoiding a resource type copy, cutting down on the number of resources added, and also avoids the condition where you simply would have to craft a new name just for the same of preventing conflicts.

I may create a separate issue for doing this.

I'll give my changes a test on top of John Mazzitelli's just to see if they are compatible or possibly improve matters.

Thanks for your consideration.
Comment 14 Elias Ross 2012-03-01 14:39:50 EST
(In reply to comment #12)

> OK, I think the problem is due to the fact that the embedded type has a child
> service in it and (in conjunction with that) the plugin descriptor has two
> types that each embed that. So, this is definitely a problem and I have some
> test code over here showing it. I'll see if I can hunt it down, perhaps looking
> at the attached patch for ways to fix.

Yes, my patch addresses this. I have test cases you can take from the patch, if anything.
Comment 15 John Mazzitelli 2012-03-01 15:28:06 EST
(In reply to comment #13)
> (In reply to comment #8)
> I'll give my changes a test on top of John Mazzitelli's just to see if they are
> compatible or possibly improve matters.
> 
> Thanks for your consideration.

Elias - I just peer reviewed your changes. Things look good, but I couldn't apply the patch to my bug/747925 branch without conflicts (conflicts in just the test stuff though).

Can I ask that you re-apply your fixes, resolving conflicts, so I can commit directly to that bug/747925 branch? Attach that fixed patch, I'll then apply to my branch, run my tests over here (I also have a second test that shows this problem) and that can put this problem to bed along with 747925.

Thanks for the efforts. Looking forward to receiving the fixed patch.
Comment 16 Elias Ross 2012-03-01 16:25:08 EST
Created attachment 566950 [details]
Rebased on top of  bug/747925

Added { }

Note: New testcase seems to break, haven't investigated.
Comment 17 John Mazzitelli 2012-03-01 17:02:45 EST
Elias - I have to kick back the new patch. Doing a full build with it, you will notice that the GWT GUI module fails to compile:

[INFO]       [ERROR] Errors in 'jar:file:/home/mazz/.m2/repository/org/rhq/rhq-core-domain/4.4.0-SNAPSHOT/rhq-core-domain-4.4.0-SNAPSHOT.jar!/org/rhq/core/domain/resource/ResourceType.java'
[INFO]          [ERROR] Line 909: The method clone() of type ResourceType must override or implement a supertype method
[INFO]          [ERROR] Line 911: The method clone() is undefined for the type Object
[INFO]          [ERROR] Line 913: No source code is available for type java.lang.CloneNotSupportedException; did you forget to inherit a required module?
[INFO]    [ERROR] Aborting compile due to errors in some input files

GWT doesn't like clone():

http://groups.google.com/group/google-web-toolkit/browse_thread/thread/d7ab63572bf3f1c2/abb2af3be886ce53

And ResourceType, being a domain object, must be compilable/runnable both in the server and in the GWT client.
Comment 18 Elias Ross 2012-03-01 18:34:27 EST
Okay, I removed @Override. My GWT build doesn't work but I don't know why yet.

Unfortunately, I can't keep amending patches forever. This is definitely a 'nice to have' for me.
Comment 19 Elias Ross 2012-03-01 18:37:24 EST
Created attachment 566969 [details]
amended patch

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