Feat: Add support for updating event capabilities on save - #929
Feat: Add support for updating event capabilities on save#929niccoloalfredo wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
after_save might be expensive, because the manager gets updated rather frequently when we update the last_refreshed_at column. I'm not sure I understand why we wouldn't do it immediately after verification (I thought that's where we set capabilities for other providers). However @agrare may have other opinions.
|
Yeah This should be run from verify_credentials, I need to dig into how that is called and why the auth_type guard was added in the first place. |
|
Oh good point - I forgot that method needs to run on particular roles, which means it can't be in an after_save which could be run from anywhere. |
|
|
| # Update only if the value changed | ||
| if capabilities["events"] != expected_value | ||
| capabilities["events"] = expected_value | ||
| save! if changed? | ||
| end |
There was a problem hiding this comment.
I think you can set it anyway, and Rails will just do the right thing. That is, I think this is effectively equivalent:
| # Update only if the value changed | |
| if capabilities["events"] != expected_value | |
| capabilities["events"] = expected_value | |
| save! if changed? | |
| end | |
| capabilities["events"] = expected_value | |
| save! if changed? |
|
|
||
| expected_value = begin | ||
| opts = event_monitor_options | ||
| opts[:events_monitor].present? ? event_monitor_available? : false |
There was a problem hiding this comment.
@agrare Is event_monitor_available? the right method for capabilities? That is, if the event monitor goes down for some reason, say network issues, that doesn't me the provider no longer has that capability - it's just currently not working. Maybe I'm just confused on what the purpose of the capabilities is.
There was a problem hiding this comment.
The more I think about it, we should set capabilities[:events] = opts[:events_monitor].present?. That is, if the user defined the events creds (regardless of if they are currently working on not), that is a statement of "yes, I expect events to be supported on this provider", and thus it has the capability (in lieu of us actually being able to detect the capability, which would be a better approach if possible)
There was a problem hiding this comment.
Thank you for your invaluable feedback and for highlighting the architectural issues with after_save.
I confirm your concerns: after_save is triggered on every provider refresh, making the event_monitor_available? check far too expensive and unsuitable for production.
Plan & Architecture Changes
I am now fully committed to moving this logic into verify_credentials or a similar appropriate hook to ensure it runs in the correct ems_operations context. I still need to refine the integration logic, though.
1. Capability Check Revision & Context
I want to clarify that I initially included the event_monitor_available? check because it was the logic used in the existing verify_credentials method within manageiq/providers/openstack/cloud_manager.rb, where it's the only place this capability is cited for OpenStack:
def verify_credentials(auth_type = nil, options = {})
options[:service] ||= "Compute"
ret = super
return ret unless auth_type.nil?
capabilities["events"] = !!event_monitor_available?
save! if changed?
true
endI moved the implementation to manager_mixin.rb hoping to generalize it. Regarding the check itself: I am currently evaluating whether to eliminate this check entirely, which is tied to the availability verification performed by OpenstackEventMonitor.available?(event_monitor_options). I agree that capability should reflect configuration, not runtime availability, and the Event Catcher workers should ideally handle connection failures gracefully.
2. verify_credentials Coverage Challenge
I'm currently working to integrate the updated logic into verify_credentials. The most insidious case is reliably updating the capability to false when the user removes all event configurations (e.g., removing AMQP). Current placement attempts within verify_credentials aren't reliably covering this removal scenario, so I still need to refine this logic. I will also apply the suggested simplification to the save! logic.
3. Automated Spec Failures
Finally, I see that the automated specs have failed, particularly in the areas of AMQP credential validation and EventCatcher eligibility. I will review these failed tests and ensure the new logic, once moved from after_save into verify_credentials, properly integrates with and passes these existing expectations.
Related PR for Review
While I work on the re-implementation here, I would appreciate it if you could also take a look at PR #930, which fixes the network manager `event_target_parser.rb
There was a problem hiding this comment.
I see that the automated specs have failed, particularly in the areas of AMQP credential validation and EventCatcher eligibility. I will review these failed tests and ensure the new logic, once moved from after_save into verify_credentials, properly integrates with and passes these existing expectations.
There was a problem hiding this comment.
We need something to indicate if the event endpoint is available not just configured. This is used by the EventCatcher worker to check if it is available to start.
We can't use the authentication status, because most of the event types re-use the default authentication record so marking the authentication invalid if ceilometer went down would stop refresh as well.
I agree capabilities["events"] should be if events have been configured not just if they are currently available. Maybe a new column on the Endpoint record to track if the "service" is available (this could conflict with authentication_status so we have to be careful)
There was a problem hiding this comment.
We can't use the authentication status, because most of the event types re-use the default authentication record
Oh interesting I didn't realize the auth status was on the auth as opposed to the endpoint. It probably makes more sense in the endpoint because an auth is only valid or invalid with respect to each endpoint it's used on. Even so that's probably an invasive change. 🤔
24b3f5a to
c2e5306
Compare
|
Hello @Fryguy and @agrare, PR Update – Automatic Event Capabilities for OpenStack ProvidersSummaryThis PR introduces automatic management of Implementation
Advantages
Notes & Next Steps
Final note: I’m looking forward to contributing to a more complete version in the future, so that with the next update the full flow works seamlessly and this work can benefit the entire community. |
|
Checked commit sourcesense@c2e5306 with ruby 3.1.7, rubocop 1.56.3, haml-lint 0.64.0, and yamllint app/models/manageiq/providers/openstack/cloud_manager.rb
app/models/manageiq/providers/openstack/manager_mixin.rb
|
|
@agrare Please review. I think we missed the recent update on this one. |
|
Closing in favor of #941 |
|
This pull request is not mergeable. Please rebase and repush. |
Pull Request: Automatic event capabilities management for OpenStack providers
References
Closes #925
Note: This PR is submitted from my personal account (@niccoloalfredo). The original issue #925 was opened and discussed from my company account (@Niccolo-Alfredo).
Summary
This PR implements the callback-based solution discussed in #925 to automatically manage
capabilities["events"]for OpenStack providers. The implementation adds anafter_savecallback that dynamically sets the event capability based on the actual configuration and availability of event brokers (AMQP, Ceilometer, or STF).Problem
As detailed in #925, the
verify_credentialsmethod contains an early exit that prevented automatic setting ofcapabilities["events"]when credentials were verified with an explicitauth_type. This required manual intervention via Rails console to enable event catchers:Solution
Instead of modifying the
verify_credentialsinterface (which has architectural complexity as discussed with @kbrock), this PR implements anafter_savecallback inManagerMixinthat:event_monitor_available?to confirm the broker is reachablecapabilities["events"]based on actual configuration stateTesting
Thoroughly tested all scenarios with AMQP (the other brokers follow the same code path via
event_monitor_optionsand should behave identically):capabilities["events"]=true, EventCatchers start automaticallycapabilities["events"]=falseornilcapabilities["events"]becomestruecapabilities["events"]becomesfalsecapabilities["events"]set tofalse, logged as warningImplementation Details
The callback:
event_monitor_optionswhich detects configured broker type (AMQP/Ceilometer/STF)event_monitor_available?which performs actual connection testcapabilities["events"]value actually changedfalsewith warning logBroker Support
While testing was performed with AMQP, the implementation supports all three OpenStack event brokers:
All three brokers are handled uniformly through
event_monitor_options→event_monitor_available?→OpenstackEventMonitor.available?.Additional Notes
This solution was preferred over modifying
verify_credentialsbecause:Ready for review. Please let me know if any adjustments are needed or if additional testing would be valuable before merge.