Ems event groups - allow provider settings #13942

durandom · 2017-02-16T14:47:49Z

Allow provider plugins to participate in setting up `EmsEvent.event_groups`

In order to provide settings for event_handling.event_groups every provider had to include its event names into settings.yml - which grew massively to beyond recognition.

To split these into settings.yml in the provider repos I have to hook into the event_groups method (see [1] for reasons).
This patch iterates over all registered provider plugins and looks for the same event_handling.event_groups structure under the provider name key.

Eg.

:amazon:
  :event_handling:
    :event_groups:
      :addition:
        :critical:
        - AWS_EC2_Instance_CREATE
        - AWS_EC2_Instance_UPDATE
        - AWS_EC2_Instance_DELETE
      :power:
        :critical:
        - AWS_EC2_Instance_running
        - AWS_EC2_Instance_shutting-down
        - AWS_EC2_Instance_stopped

Then it merges the hash and more important merges the arrays of events.

see inline comments for questions

Includes #13941
Follows the suggestion in [1]

[1] ManageIQ/manageiq-providers-amazon#138

durandom · 2017-02-16T14:49:41Z

app/models/ems_event.rb

@@ -24,8 +24,22 @@ def self.task_final_events
 ::Settings.event_handling.task_final_events.to_hash
 end

+ def self.provider_plugin_keys


could this method go into Vmdb::Plugins for re-use?

Sounds good to me, maybe something like @provider_plugins to be populated on initialize.

yeah, makes sense. Then I can also skip the begin / rescue dance below

durandom · 2017-02-16T14:56:39Z

app/models/ems_event.rb

+ Vmdb::Plugins.instance.vmdb_plugins.collect do |plugin|
+ begin
+ ManageIQ::Providers::Inflector.provider_name(plugin).downcase.to_sym
+ rescue ManageIQ::Providers::Inflector::ObjectNotNamespacedError


Do not suppress exceptions.

is it ok here?

I would probably compare the class names instead...

plugin.class.name.starts_with?("ManageIQ::Providers")

durandom · 2017-02-16T14:57:10Z

app/models/ems_event.rb

 def self.event_groups
- ::Settings.event_handling.event_groups.to_hash
+ provider_plugin_keys.each_with_object(::Settings.event_handling.event_groups.to_hash) do |provider_key, event_groups|
+ next unless provider_event_group = ::Settings.fetch_path(provider_key, :event_handling, :event_groups)


Assignment in condition - you probably meant to use ==.

nope, I mean it. Should I split into two lines?

durandom · 2017-02-16T14:57:42Z

app/models/ems_event.rb

+ end
+ end.compact
+ end
+
 def self.event_groups


If this is ok, I probably want to do this for bottleneck_event_groups too

durandom · 2017-02-16T14:58:18Z

app/models/ems_event.rb

+ if event_groups_val.kind_of?(Array) && provider_groups_val.kind_of?(Array)
+ event_groups_val + provider_groups_val
+ else
+ event_groups_val


this is meant to preserve String values like :name:

durandom · 2017-02-16T14:59:02Z

@miq-bot add_labels pluggable providers, enhancement
@miq-bot assign @Fryguy

@Fryguy @bdunne please review

durandom · 2017-02-17T11:40:27Z

@bdunne refactored to move the provider_plugins to Vmdb::Plugins. While there I made some imho internal methods private.

durandom · 2017-02-17T11:41:03Z

If you guys are ok with it, I'll add some tests

bdunne · 2017-02-17T16:36:57Z

lib/vmdb/plugins.rb

 @vmdb_plugins = []
+
+ Rails.application.railties.each do |railtie|
+ next unless railtie.class.name.start_with?("ManageIQ::Providers::") || railtie.try(:vmdb_plugin?)


@durandom Would you mind adding this to the engine.rb on the plugin generator? It would simplify this code.

def vmdb_plugin? true end

Sure, I can. It's on my TODO 😄

Can the registered_provider_plugins logic be split into a stand alone PR, independent of EmsEvents and moving the initializer logic?

bdunne · 2017-02-17T16:59:35Z

lib/vmdb/plugins.rb

 @vmdb_plugins = []
+
+ Rails.application.railties.each do |railtie|
+ next unless railtie.class.name.start_with?("ManageIQ::Providers::") || railtie.try(:vmdb_plugin?)


Can the registered_provider_plugins logic be split into a stand alone PR, independent of EmsEvents and moving the initializer logic?

durandom · 2017-02-17T17:19:57Z

I'll extract the Vmdb::Plugins changes to #13941

bdunne · 2017-02-17T19:19:05Z

@durandom I feel like thee are three different things. Listing Provider plugins, EmsEvents, and Plugin initialization

durandom · 2017-02-20T11:49:14Z

@bdunne

Listing Provider plugins -> moved to Add registered_provider_plugins to Vmdb::Plugins #13983 (merged)
Plugin initialization -> already at move plugin initializer code into Vmdb::Plugins #13941

Once those are merged, I'll rebase this on top of them

miq-bot · 2017-02-20T18:56:43Z

This pull request is not mergeable. Please rebase and repush.

bdunne · 2017-02-20T19:18:43Z

lib/vmdb/plugins.rb

@@ -4,11 +4,17 @@ class Plugins

 attr_reader :vmdb_plugins
 attr_reader :registered_automate_domains
+ attr_reader :registered_provider_plugins


I think this should have been removed during rebase.

durandom · 2017-02-20T20:46:33Z

@miq-bot remove_label wip

@bdunne refactored a bit and squashed commits

durandom · 2017-02-21T11:47:14Z

@bdunne added specs for the change, please re-review

bdunne

Not too familiar with eventing, but 👍 LGTM
@Fryguy Any concerns?

Fryguy

I'm not a fan of the toplevel :amazon key given that we already have a toplevel :ems key that is designed for pluggable ems settings. I think the events can be nested under the respective key under the toplevel :ems key, and then there is no need to have any knowledge about plugins at all. The Settings constant will just contain everything in a known location and we can walk it. In this way there is no need to couple EmsEvent to Vmdb::Plugins at all.

So, for example, the Settings will look like:

:ems:
  :ems_amazon:
    :event_handling:
      :event_groups:
        :addition:
          :critical:
          - AWS_EC2_Instance_CREATE
          - AWS_EC2_Instance_UPDATE
          - AWS_EC2_Instance_DELETE
        :power:
          :critical:
          - AWS_EC2_Instance_running
          - AWS_EC2_Instance_shutting-down
          - AWS_EC2_Instance_stopped

Then, since we know that there's a bunch of provider specific keys under :ems, we just walk the keys, and look for ones with event_groups within them.

core_event_groups = ::Settings.event_handling.event_groups.dup

Settings.ems.each_with_object(core_event_groups) do |(_provider_type, provider_settings), event_groups|
  next unless provider_settings.event_groups
  # Existing deep_merge code
end

To take this a little further, the deep_merge method comes from the deep_merge gem, so you can probably replace the entirety of your merge code, with code that works exactly like how Config::Options works under the covers, but with merging of arrays enabled (or overwrite disabled, as it were):

event_groups.deep_merge!(provider_settings.event_groups.to_hash, :preserve_unmergeables => false, :overwrite_arrays => false)

durandom · 2017-02-22T21:05:20Z

@Fryguy thanks for the review

we already have a toplevel :ems key that is designed for pluggable ems settings.

I saw that and was about to put it right there, but then I thought it's actually a good chance to move away from :ems, as we want to pivot toward talking about Providers and Managers.

But you have a point in decoupling from Vmdb::Plugins that we can just walk whatever is under :ems. (Maybe we can replace that by :provider sometime 😄

How about ditching the ems_ prefix then? So Settings.ems.amazon? Just say no and I'll put it at Settings.ems.ems_amazon (and put up a PR to change it, hehe)

deep_merge method comes from the deep_merge gem

oh, nice. Can I use this, although it's a dependency from the config gem? So it might go away if config drops the dependency. But for this I have the test...

durandom · 2017-02-22T21:28:11Z

Wether it be :ems.ems_amazon or :ems.amazon doesnt need to be part of this PR, because this will now consume everyhing under :ems

I chose not to use deep_merge from the deep_merge gem, because a) it adds another dependency and b) it will allow to overwrite string values (the :name key - see here )

miq-bot · 2017-02-23T15:47:27Z

Checked commits durandom/manageiq@80c8cb7~...e686839 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🍪

durandom · 2017-02-23T16:27:33Z

@Fryguy please see my last 2 comments, ok now?

durandom · 2017-02-28T07:44:02Z

@Fryguy pling pling, are you good with this?

Fryguy · 2017-03-06T18:38:28Z

Closed in favor of #13942

miq-bot · 2017-03-06T18:41:35Z

This pull request is not mergeable. Please rebase and repush.

NickLaMuro · 2017-06-20T23:52:26Z

@Fryguy Was following the changes and additions to Vmdb::Plugins, and came across this PR, and noticed that in your comment from above:

Closed in favor of #13942

That it is this PR that you are referencing instead of some other one. Did you mean #14177 perhaps? Based on the PR description, seems like that is the case, but wanted to confirm.

durandom · 2017-06-21T08:38:07Z

@NickLaMuro that is correct.

self referencing PR links should throw a null pointer exception 8/

durandom mentioned this pull request Feb 16, 2017

Add amazon events to settings.yml #13927

Closed

durandom commented Feb 16, 2017

View reviewed changes

miq-bot added the wip label Feb 16, 2017

durandom force-pushed the ems_event_allow_provider_settings branch from 8360059 to 63318ae Compare February 16, 2017 14:55

durandom commented Feb 16, 2017

View reviewed changes

miq-bot added enhancement pluggability/providers labels Feb 16, 2017

miq-bot assigned Fryguy Feb 16, 2017

durandom mentioned this pull request Feb 16, 2017

Filter out events in Settings instead of CloudManager ManageIQ/manageiq-providers-amazon#141

Merged

bdunne reviewed Feb 17, 2017

View reviewed changes

bdunne requested changes Feb 17, 2017

View reviewed changes

durandom mentioned this pull request Feb 20, 2017

Add registered_provider_plugins to Vmdb::Plugins #13983

Merged

miq-bot added the unmergeable label Feb 20, 2017

durandom force-pushed the ems_event_allow_provider_settings branch from 5f90769 to c95e71b Compare February 20, 2017 19:09

miq-bot removed the unmergeable label Feb 20, 2017

bdunne requested changes Feb 20, 2017

View reviewed changes

durandom force-pushed the ems_event_allow_provider_settings branch from c95e71b to 4113773 Compare February 20, 2017 20:42

durandom changed the title ~~[WIP] Ems event groups - allow provider settings~~ Ems event groups - allow provider settings Feb 20, 2017

miq-bot removed the wip label Feb 20, 2017

durandom force-pushed the ems_event_allow_provider_settings branch from 1c469fd to 70ca0a8 Compare February 21, 2017 11:49

bdunne approved these changes Feb 21, 2017

View reviewed changes

Fryguy requested changes Feb 22, 2017

View reviewed changes

durandom added 4 commits February 23, 2017 16:45

allow provider plugins to provide their own settings for events

80c8cb7

allow stubbing a subset of Settings while keeping the rest

c950c4c

add specs for EmsEvent.event_groups

e444bb3

remove coupling to Vmdb::Plugins

e686839

durandom force-pushed the ems_event_allow_provider_settings branch from 23f277b to e686839 Compare February 23, 2017 15:45

durandom mentioned this pull request Mar 3, 2017

Ems event groups - allow provider settings (deeper_merge edition) #14177

Merged

Fryguy closed this Mar 6, 2017

miq-bot added the unmergeable label Mar 6, 2017

durandom deleted the ems_event_allow_provider_settings branch March 6, 2017 20:57

durandom mentioned this pull request Mar 6, 2017

add amazon events to settings to display them in timelines ManageIQ/manageiq-providers-amazon#163

Merged

durandom mentioned this pull request Jun 6, 2017

fix indents so we don't overwrite the main settings.yml ManageIQ/manageiq-providers-openstack#53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ems event groups - allow provider settings #13942

Ems event groups - allow provider settings #13942

durandom commented Feb 16, 2017

durandom Feb 16, 2017

bdunne Feb 16, 2017

durandom Feb 16, 2017

durandom Feb 16, 2017

bdunne Feb 16, 2017

durandom Feb 16, 2017

durandom Feb 16, 2017

durandom Feb 16, 2017

durandom commented Feb 16, 2017

durandom commented Feb 17, 2017

durandom commented Feb 17, 2017

bdunne Feb 17, 2017

durandom Feb 17, 2017

bdunne Feb 17, 2017

bdunne Feb 17, 2017

durandom commented Feb 17, 2017

bdunne commented Feb 17, 2017

durandom commented Feb 20, 2017 •

edited

Loading

miq-bot commented Feb 20, 2017

bdunne Feb 20, 2017

durandom commented Feb 20, 2017

durandom commented Feb 21, 2017

bdunne left a comment

Fryguy left a comment •

edited

Loading

durandom commented Feb 22, 2017

durandom commented Feb 22, 2017

miq-bot commented Feb 23, 2017

durandom commented Feb 23, 2017

durandom commented Feb 28, 2017

Fryguy commented Mar 6, 2017

miq-bot commented Mar 6, 2017

NickLaMuro commented Jun 20, 2017

durandom commented Jun 21, 2017

Ems event groups - allow provider settings #13942

Ems event groups - allow provider settings #13942

Conversation

durandom commented Feb 16, 2017

Allow provider plugins to participate in setting up EmsEvent.event_groups

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Feb 16, 2017

durandom commented Feb 17, 2017

durandom commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Feb 17, 2017

bdunne commented Feb 17, 2017

durandom commented Feb 20, 2017 • edited Loading

miq-bot commented Feb 20, 2017

Choose a reason for hiding this comment

durandom commented Feb 20, 2017

durandom commented Feb 21, 2017

bdunne left a comment

Choose a reason for hiding this comment

Fryguy left a comment • edited Loading

Choose a reason for hiding this comment

durandom commented Feb 22, 2017

durandom commented Feb 22, 2017

miq-bot commented Feb 23, 2017

durandom commented Feb 23, 2017

durandom commented Feb 28, 2017

Fryguy commented Mar 6, 2017

miq-bot commented Mar 6, 2017

NickLaMuro commented Jun 20, 2017

durandom commented Jun 21, 2017

Allow provider plugins to participate in setting up `EmsEvent.event_groups`

durandom commented Feb 20, 2017 •

edited

Loading

Fryguy left a comment •

edited

Loading