Bug 536373 (RHQ-729)

wow, that's an impressive amount of detail jdob - nice leg work.  

*This comment largely talks in terms of the (unfortunately) currently closed source JBoss AS plugin code, however the concepts of what and how to connect to a resource transcend a particular plugin and apply to plugin best practices in general.*

The initial issue that started me down this path was the AS plugin failing on start if the AS instance isn't running. I wonder if I'm looking at the solution in the wrong place. The issue might not be in our architecture of having a start call on a resource component. It might be in the implementation of the AS plugin itself.

If the AS resource component can't connect to a resource when the component is started, is that necessarily a failure?

We currently have code that will attempt to start a stopped resource component each time it's used if it's not already running. That same sort of logic might have a place in the AS plugin. Perhaps we treat the inability to connect to an AS instance as not a failure. Instead, if we are not connected when we go to use the JMX connection, we once again attempt to load it. At that point, if we still can't, we can't perform the requested operation (metrics collection, operation, etc.) and THAT is where the failure lies.

At the same time, we still have a means for showing that connection is not made in the availability check. So even if the start succeeds, the availability indicator of the resource will still show there is an issue.

As much as I thought this was a new idea, it's largely covered in the ResourceComponent documentation:

     * <p>This method typically examines the plugin configuration from {@link ResourceContext#getPluginConfiguration()}
     * and uses that information to connect to the managed resource. If this method finds that the plugin configuration
     * is invalid which causes the connection to the managed resource to fail, then this method should throw
     * {@link InvalidPluginConfigurationException}. This exception typically should not be thrown if the connection
     * failed for some reason other than an invalid plugin configuration (e.g. in the case the managed resource is
     * simply not running now) because usually those conditions can be tracked as part of the resource's
     * {@link #getAvailability() availability} data.</p>

The current incarnation of the start method (redundantly) throws InvalidPluginConfigurationException and Exception. I don't believe the infrastructure properly reacts to the former and I'd like to do some more research into that to make sure we still have the information necessary to inform the user the plugin configuration needs to be updated.

There are a number of issues in play in the current code. Depending on the solution, some of these may disappear, but it's worth noting where we stand now.

InventoryManager.activateResource
------------------------------

1.) When we try to start the resource, there is code to check to see if the activation was specifically due to a plugin configuration change. If it is, we rewrap the exception as an InvalidPluginConfigurationException. There are two potential issues with this:
- The rewrap is often unnecessary; if the plugin is correctly honoring the ResourceComponent contract, it will have already thrown an InvalidPluginConfigurationException as it is.
- We swallow *any* exception from the call to ResourceComponent.start and wrap it as an InvalidPluginConfiguration exception. This prevents us from being able to differentiate between a failure and an invalid config.

As a result of the second point, if the plugin configuration change was successful (but a connection cannot be established), we have no way of knowing that and will mark the change request as failed.


2.) I may be missing something, but if a plugin configuration update fails, we add a new ResourceGotActivatedListener. That listener is notified when a resource is successfully activated. In the listener, the server is sent a message to clear the resource configuration error (DiscoveryServerService.clearResourceConfigError).

The problem as I see it is that a new instance of this listener is added to the listener list each time a plugin configuration fails. The instance itself isn't scoped to a particular resource. So if we fail to update a plugin configuration multiple times, or even for multiple resources before the correct configuration is entered, there will be two results:
- The first time a resource is successfully activated, there will be multiple calls to the server to clear its resource configuration error.
- Each of these instances removes itself from the listener list after it's been triggered the first time. If there were multiple resources that had invalid plugin configurations, when they are resolved the listeners won't be present to make the call to clear the configuration error.

InventoryManager.updatePluginConfiguration
------------------------------

1.) There is a TODO in place for this. After activating the resource whose plugin configuration has just changed, we don't attempt to activate its children. I suspect in the rest of the discovery chaos we accidentally activate them, otherwise we'd have run into this already. Regardless, we should better understand if we need to explicitly add this or if they will be reliably be started elsewhere.

2.) Similarly, we don't deactivate children, just the resource being updated. We should put some thought into whether or not this would be a good idea.

ConfigurationManagerBean.executePluginConfigurationUpdate
------------------------------

1.) The call is made to DiscoveryAgentService.updatePluginConfiguration, which is fielded by the InventoryManager to actually do the update. The agent service method can throw one of two exceptions: InvalidPluginConfigurationException and PluginContainerException.

In the ConfigurationManagerBean, however, we simply catch Exception. Thus even when the InventoryManager is fixed to not mask the indication that a plugin configuration update was invalid, the server won't treat it differently anyway.

JBossASServerComponent.start
------------------------------
1.) This should not throw an exception if we cannot connect to the AS resource it if it not running, as per the documentation in ResourceComponent.start

Given that the proposed split of start and connect would affect more than just plugin configuration updates, the following summarizes the usage of the InventoryManager.activateResource call.

AvailabilityExecutor
- Checking the availability of a resource that's not currently started

InventoryManager
- Starting a resource component after it is loaded from disk
- Starting the resource component of a manually inventoried resource
- "Merging resource from discovery" when in the embedded console
- In refreshResourceComponentState - ???  (I'm not fully sure what this does, but once again there is a flag in this method that does different handling if a plugin configuration was updated).
- After a plugin configuration is updated

This reply only scratches the surface of all the various issues you addressed:

there doesn't need to be a state distinction between started/stopped, connected/disconnected. to me, this is all just "state", and as such a single state diagram could comprise the entire thing.

* started->connected,disconnected // so, a resource is started by the container, and then depending on whether the connect() succeeds can either move to connected or disconnected
* disconnected<-->connected // the resource will move back and forth between these two states depending on whether the plugin configuration entries allow it to connect or not
* disconnected->stopped // if a resource wants to be stopped, it must first be in the disconnected state, so disconnect() might first have to be called before stop()

then, once you have this single, unified state model, plugin configuration updates don't have to worry about putting the resource in an inconsistent stopped/connected state tuple. instead, plugin config updates look quite simple:

* if resource is connected, disconnect()
* update plugin config
* connect()

if connect throws plugin configuration exception, then the plugin container assumes that the entries are not valid to be able to connect to the resource. the resource would remain in the disconnected state, but the new plugin configuration is kept locally on that agent -- because there's no reason to NOT update the plugin config, it's just data (that in this case has special meaning for resource connectivity)

-----

i think the interface idea is interesting. but to make it easier for the plugin developer i think we can provide reasonable defaults. for instance, if you don't override connect(), the default impl is to return true, meaning that the container will always succeed in putting your resource into the connected state.

-----

i think container services need to be directly linked to the state of the resource. essentially, i only care about availability reports, measurement reports, events, etc when my resource is in the connected state. if it's not in the connected state, then it is a feature of the plugin container to always return an "unknown" availability state for that resource...but this might not be something we want to handle.

basically, do we really want to differentiate between: 1) i can't connect to the resource, so i have no idea what state it's in, and 2) i can connect to the resource, but it's in a downed state. we have to consider how often case #2 can really occur. i'd think it would be relatively rare. more often, if the resource is down, then the plugin won't be able to connect to it either. so the inability for the plugin to reach the connected state is the same thing as red availability.

perhaps, from a plugin developer's perspective, this should be the container-provided, default implementation for the availability report. but users are free to override the availability facet and provide more complicated mechanism (essentially, one of the rare scenarios that case #2 embodies).

-----

so, similar to availability, once we have a solid notion of state (connected/disconnected), we can come up with what a container-level service failure is. basically, i feel that we should ONLY be starting/running container services if the resource is in the connected state. if it's not connected, then everything else is suppressed. no data is polled for, no listeners are registered, no files are parsed, etc etc. this way, there is a clean separation / abstraction. services that logically should depend on the ability to talk to the resource don't themselves execute unless that line of communication is open. if that line goes down, it should be the responsibility of the container to shut down those services until some time in the future when that connection is re-established.

@  "then, once you have this single, unified state model, plugin configuration updates don't have to worry about putting the resource in an inconsistent stopped/connected state tuple. instead, plugin config updates look quite simple"

I agree that if we were to do the start/connect split, it'd make sense to do it in a unified state model. Look at my original comment under Other Tasks. The issue is that it'd be a pretty significant change to our domain model. I believe the server uses the Resource.connected flag, so it couldn't be outright removed. Nor does it make sense to expose the ResourceContainer's state to the server. So at best, we could add the unified state model and maintain the Resource.connected flag. It's just ugly.

This all still falls victim to the much bigger question of whether or not it even makes sense to split them. I'd like to see some more thought into what that would get us. Or put another way, how having the two would affect plugin development.

@ "if connect throws plugin configuration exception ... but the new plugin configuration is kept locally on that agent -- because there's no reason to NOT update the plugin config"

There is a reason to not update it, it's invalid. If the plugin rejects a configuration as invalid, there's no reason to keep it around in the agent; the plugin indicated that it's not going to use it.

@ "for instance, if you don't override connect()"

This could be an enhancement later once we clean up InventoryManager and better understand how it should work, but for now I'm fine with letting the plugin writer stub it out themselves if necessary.

@ "this should be the container-provided, default implementation for the availability report. but users are free to override the availability facet"

FYI, availability is not a facet. It's one of the three method in ResourceComponent. I don't think we give them much at all by providing simple stubs, and it we just couldn't achieve the plugin flexibility we have now without using interfaces.

In fact, I'd argue we go in the other direction. If we don't run with connect, we use the getAvailability call to provide the connected logic for us. They aren't necessarily the same thing, but again, I'm not sure what we gain from explicit "is this connected" logic.

@ "basically, do we really want to differentiate between: 1) i can't connect to the resource, so i have no idea what state it's in, and 2) i can connect to the resource, but it's in a downed state"

I think we do want this differentiation. If I update the plugin connection properties, I want to know if I specified a bad value. Even if the resource is down, that didn't stop me from updating the configuration. The issue is, I'm not sure how possible it will be in most cases to determine that a given set of properties is valid, however the resource isn't running.

@ "i think container services need to be directly linked to the state of the resource."   and   "if that line goes down, it should be the responsibility of the container to shut down those services until some time in the future when that connection is re-established."

Agreed, and they are now to a point. The problem is, some do lazy connecting if the connection failed, so it's tricky to see what is going on.

Thanks for the feedback.  Just one thing...

@@ "if connect throws plugin configuration exception ... but the new plugin configuration is kept locally on that agent -- because there's no reason to NOT update the plugin config"

@ "There is a reason to not update it, it's invalid. If the plugin rejects a configuration as invalid, there's no reason to keep it around in the agent; the plugin indicated that it's not going to use it. "

Well, how do we know for sure it's NOT a good configuration?  What if the managed resource was down (not started), but the user wanted to update the plugin configuration anyway.

In my opinion, the server-side initiated update of a plugin config should always succeed.  Things should not fail because an indirect hook or processing side-effect failed.  A plugin config update though should fail if we literally couldn't update the plugin config (problems communicating with the agent, there was a timeout because we sat waiting for some lock too long [indicating we have a concurrent bug somewhere], component was not in the started sate [indicating we might have a life-cycle bug in the plugin container logic], etc).

This way, if we update the plugin config, and always keep that last updated record on the agent, if the managed resource is ever restarted the plugin will naturally reconnect to it during it's next availability poll.  And, from an aggregate standpoint, if i'm updating the plugin config for 100 JBAS servers, I *want* that update to succeed.  Each agent should get the new plugin config, even if they can't connect.  Then I go to the aggregate control / operations section and execute a start operation against the same group.  Because each agent has that most recent plugin config I pushed to them, they should all startup because they have the correct data.  If the agent didn't keep this update, it might cache an older / stale plugin config, which would cause the start operation to fail later.

Ahh, ok. This is a fundamental question.

What was called "plugin configuration exception" is actually two different exceptions thrown from start. To me, if the plugin throws InvalidPluginConfigurationException, it means we know it's not a good configuration. Perhaps it failed some plugin-only validation (as compared to the static validation, i.e. is port an integer, which we can do on the server). For instance, if the resource type's plugin configuration takes a directory on the file system, it might be considered an invalid configuration if that directory doesn't exist.

What this gets us is the ability to show the user in the UI that property X was deemed invalid by the plugin and prompt them to enter a new value. However, this isn't exactly working properly due to a few of the various bugs I listed earlier.

There may be a use case for being able to say "update the configuration anyway, validation be damned." But otherwise, I treat that exception as the plugin saying the configuration values will not be enforced, and thus the agent shouldn't care to hold on to them and the server UI should reflect that.

The big issue that led me here is the case of us knowing if a plugin configuration was set (i.e. passed plugin-side validation) however we still can't connect to the resource. For example, if we specified bad login credentials for connecting to JMX.

That's a tricky use case. If we attempt to connect and cannot with those credentials, we could reject the plugin configuration update as invalid. But if the AS instance isn't running, we can't actually test those credentials. In the latter, is that still a failed plugin configuration update? What happens when the AS instance comes online and the credentials are bad?

@ "What was called "plugin configuration exception" is actually two different exceptions thrown from start. To me, if the plugin throws InvalidPluginConfigurationException, it means we know it's not a good configuration. Perhaps it failed some plugin-only validation (as compared to the static validation, i.e. is port an integer, which we can do on the server). For instance, if the resource type's plugin configuration takes a directory on the file system, it might be considered an invalid configuration if that directory doesn't exist."

Agreed.  If there is plugin-only validation, we could fail-fast as we know this config never has a chance of being correct.

@ "What this gets us is the ability to show the user in the UI that property X was deemed invalid by the plugin and prompt them to enter a new value. However, this isn't exactly working properly due to a few of the various bugs I listed earlier."

True, which would work for individual config updates, but maybe not for aggregate plugin config updates...or maybe it would.  An aggregate update, in the end, is just a bunch of clones of some config update, so we should be able to validate the aggegrate update ahead of time.  Basically, we could have the aggregate pushed down to any (it doesn't matter which) agent.  If validation fails, don't submit the job and don't create the individual plugin config update children (because presumably if the aggregate failed validation, so too will the children plugin config updates).

@ "There may be a use case for being able to say "update the configuration anyway, validation be damned." But otherwise, I treat that exception as the plugin saying the configuration values will not be enforced, and thus the agent shouldn't care to hold on to them and the server UI should reflect that."

I'd say that validation failures are the primary (only?) ones we should be concerned with.  If validation passes, then the config may or may not be correct.  (BTW, if we want to be able to do the aggregate validation ahead of time, I'd say that we should separate the validation method from the update method on the agent side.)

@ "The big issue that led me here is the case of us knowing if a plugin configuration was set (i.e. passed plugin-side validation) however we still can't connect to the resource. For example, if we specified bad login credentials for connecting to JMX."

@ "That's a tricky use case. If we attempt to connect and cannot with those credentials, we could reject the plugin configuration update as invalid. But if the AS instance isn't running, we can't actually test those credentials. In the latter, is that still a failed plugin configuration update? What happens when the AS instance comes online and the credentials are bad?"

Agreed, unless we draw a very clear distinction between errors that occur when we can't find the running process versus errors that occur when we can ping the process but don't get back the data we're expecting, this is going to be tough to track down.  Perhaps we could use the events subsystem to notify of these things?  This way, the user will see that the resource is red in the web console, and they can go to the events tab to see why it can't connect.  The event details will either be that the instance could not be found (case when the server isn't started), or that the instance could not be connected up (case when the server was up, but creds are bad).

@ "I'd say that we should separate the validation method from the update method on the agent side"

I was thinking the same thing based on your first comment in that last reply. That'd also save us the unnecessary stop/disconnect if we know the plugin config won't work anyway.

I want to do some looking at other plugins a bit before I continue. I think I've gotten too into the mentality of the AS plugin and its EMS connection. I wonder how much that carries over into other domains. We may be debating adding a connect method that will be unused in 90% of the cases.

One other comment at this time, I'm currently leaning towards not adding connect and fixing the AS usage of start. I don't really view not being able to connect as a reason for start to fail. Availability might be usable for that, although isn't exactly the same thing (there could be two interpretations... the resource is down or we simply can't get to it to check). But again, I wonder if looking at other plugins will make the generic case a little easier to see.

This was punted on for RHQ 1.1. At this time, there doesn't seem to be a compelling use case for changing the semantics of start. To work around the JBoss AS issue that prompted this, the AS plugin was changed to correctly honor the ResourceComponent API as it already stood.

This bug was previously known as http://jira.rhq-project.org/browse/RHQ-729