Use settings for filtered events #23320

agrare · 2025-01-29T19:04:04Z

Dependents:

#19707

Fryguy · 2025-01-29T19:13:38Z

app/models/manageiq/providers/base_manager/event_catcher/runner.rb

@@ -95,16 +95,16 @@ def before_exit(message, _exit_code)
  def sync_blacklisted_events


I think this can go away entirely - we already sync_settings

Yes I'm testing that the @filtered_events ivar is also reloaded when settings change (ref vs copy) when pointing to the Config object

Yup confirmed that @filtered_events had the old copy of the array but after sync_config @ems.class.default_blacklisted_event_names has the updated list so that's perfect.

Fryguy · 2025-01-29T19:14:41Z

app/models/manageiq/providers/base_manager.rb

@@ -57,7 +57,7 @@ def self.http_proxy
    end

    def self.default_blacklisted_event_names


probably want to rename this to blacklisted_event_names, since it's not a default anymore. If we really need the default there is a way to load the settings without the user's changes, but that feels unnecessary.

Yes this is the goal, will need to sync with a number of providers at the same time hence wip

Fryguy · 2025-01-29T19:15:11Z

I realize this is WIP, but MUCH EXCITE

agrare · 2025-01-29T19:18:13Z

app/models/manageiq/providers/base_manager.rb

-      Array(::Settings.ems["ems_#{provider_name.underscore}"].try(:blacklisted_event_names))
+      Array(::Settings.ems["ems_#{ems_type}"].try(:blacklisted_event_names))


NOTE this will let us delete most of the overrides in the providers, I think Vmware::InfraManager is the only one where ems_type != the settings key

agrare · 2025-01-29T19:44:05Z

app/models/manageiq/providers/base_manager/event_catcher/runner.rb

+  def filtered_events
+    @ems.class.default_blacklisted_event_names
  end


NOTE we probably don't even need this method since the providers could use their Settings.ems.ems_type directly

agrare · 2025-01-29T19:45:36Z

app/models/miq_worker/runner.rb

@@ -266,7 +266,6 @@ def sync_config
    Vmdb::Settings.reload!
    @my_zone ||= MiqServer.my_zone
    sync_worker_settings
-    sync_blacklisted_events


Wild that this was in the base MiqWorker::Runner class 🤷

Not sure why the EventCatcher didn't just do this in after_sync_config

wow, this must be old... I wonder what non-event workers would need to sync blacklisted events?

I think it was just implemented globally, because reasons - I don't recall it being for anything but event catchers in practice.

Yeah I don't see any rationale in the original PR for why it was in the base base worker. The sync_blacklisted_events method was added to the BaseManager::EventCatcher::Runner in an earlier commit in the same PR then added to the base worker towards the end (guessing they saw it wasn't being called so just added it to the base class)

Fryguy · 2025-01-29T20:06:38Z

Obligatory @-mention of @jrafanie for this wonderful 🔥 ✂️ 🗑️

agrare · 2025-01-30T17:15:22Z

@miq-bot cross-repo-tests /providers, ManageIQ/manageiq-providers-amazon#883, ManageIQ/manageiq-providers-azure#567, ManageIQ/manageiq-providers-openstack#903, ManageIQ/manageiq-providers-ovirt#686, ManageIQ/manageiq-providers-vmware#937,

From Pull Request: ManageIQ/manageiq#23320

agrare · 2025-01-30T18:07:35Z

There are two sets of changes that will involve dependent provider prs, the first is what I have linked right now which just changes the providers which had overridden the .default_blacklisted_event_names method and is only a handfull of them. The second will be to change the Settings blacklisted_event_names to match the new filtered_event_names name and will hit way more providers.

jrafanie · 2025-01-31T16:41:55Z

Obligatory @-mention of @jrafanie for this wonderful 🔥 ✂️ 🗑️

It's definitely has the adds/deletes ratio done correctly!

haha, I can't wait to see the migration ... how do we migrate from what we have now in some things using black listed events model to settings?

Fryguy · 2025-01-31T19:39:00Z

how do we migrate from what we have now in some things using black listed events model to settings?

I believe there is a column for "system" blacklisted events, so we can filter on that for "user" events. Then, we'd have to stick it into an existing provider-specific payload, and I'm actually not sure how to do that without picking either a random provider, or just putting it under all providers. Vmdb::Settings has a way to get the "base" configuration before applying the user overrides, so you could grab the provider's out of the box list of filtered events, then tack on the extra event(s) and write it directly to the SettingsChange table.

agrare · 2025-01-31T20:56:20Z

I believe there is a column for "system" blacklisted events

Any events added via Advanced Settings and then seeded would be marked as system, the only ones which wouldn't would be if someone manually created a BlacklistedEvent record via the rails console.

I think the only guaranteed way is going to be to have in the migration a map of provider_model=>settings_key and a full list of default blacklisted settings per settings_key.

Fryguy · 2025-02-14T14:31:39Z

@agrare Is there a data migration for the existing table to drop it and convert the values to settings? If not, should I wait on this set of PRs?

Fryguy

Just waiting on the data migration question

agrare · 2025-02-17T21:46:34Z

@miq-bot cross-repo-tests /providers, ManageIQ/manageiq-providers-amazon#883, ManageIQ/manageiq-providers-azure#567, ManageIQ/manageiq-providers-openstack#903, ManageIQ/manageiq-providers-ovirt#686, ManageIQ/manageiq-providers-vmware#937, ManageIQ/manageiq-schema#783

From Pull Request: ManageIQ/manageiq#23320

agrare · 2025-02-17T21:58:15Z

So because of how we seed blacklisted events from settings, if a user added an event-name to the blacklisted events setting and ran BlacklistedEvent.seed from a rails console we don't need to do anything because the SettingsChange is already there. The only time we'd miss a blacklisted event would be if they manually created a BlacklistedEvent record not via Advanced Settings which I'm not sure we've ever instructed anyone to do but it is possible.

ManageIQ/manageiq-schema#783

agrare added 2 commits January 28, 2025 16:53

Use default_blacklisted_event_names for filtered_events

170262a

Drop BlacklistedEvent model

e32f387

agrare requested review from Fryguy and kbrock as code owners January 29, 2025 19:04

Fryguy reviewed Jan 29, 2025

View reviewed changes

Fryguy self-assigned this Jan 29, 2025

Fryguy added the technical debt label Jan 29, 2025

agrare commented Jan 29, 2025

View reviewed changes

Drop sync_blacklisted_events and @filtered_events

defb560

agrare requested a review from jrafanie as a code owner January 29, 2025 19:42

agrare commented Jan 29, 2025

View reviewed changes

Rename default_blacklisted_event_names

ed1b8f3

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jan 30, 2025

Running tests for agrare

8eef18f

From Pull Request: ManageIQ/manageiq#23320

miq-bot mentioned this pull request Jan 30, 2025

[BOT] Cross repo test for ManageIQ/manageiq#23320 ManageIQ/manageiq-cross_repo-tests#945

Closed

agrare changed the title ~~[WIP] Use settings for filtered events~~ Use settings for filtered events Feb 5, 2025

Fryguy approved these changes Feb 14, 2025

View reviewed changes

agrare mentioned this pull request Feb 17, 2025

Create SettingsChanges from BlacklistedEvents ManageIQ/manageiq-schema#783

Merged

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Feb 17, 2025

Running tests for agrare

ba43a61

From Pull Request: ManageIQ/manageiq#23320

miq-bot mentioned this pull request Feb 17, 2025

[BOT] Cross repo test for ManageIQ/manageiq#23320 ManageIQ/manageiq-cross_repo-tests#949

Closed

Fryguy merged commit 99489ca into ManageIQ:master Feb 19, 2025
8 checks passed

agrare deleted the use_settings_for_filtered_events branch February 19, 2025 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use settings for filtered events #23320

Use settings for filtered events #23320

agrare commented Jan 29, 2025 •

edited by Fryguy

Loading

Fryguy Jan 29, 2025

agrare Jan 29, 2025

agrare Jan 29, 2025

Fryguy Jan 29, 2025

agrare Jan 29, 2025

Fryguy commented Jan 29, 2025

agrare Jan 29, 2025

agrare Jan 29, 2025

agrare Jan 29, 2025 •

edited

Loading

jrafanie Jan 31, 2025

Fryguy Jan 31, 2025

agrare Jan 31, 2025 •

edited

Loading

Fryguy commented Jan 29, 2025 •

edited

Loading

agrare commented Jan 30, 2025

agrare commented Jan 30, 2025

jrafanie commented Jan 31, 2025

Fryguy commented Jan 31, 2025 •

edited

Loading

agrare commented Jan 31, 2025

Fryguy commented Feb 14, 2025

Fryguy left a comment

agrare commented Feb 17, 2025

agrare commented Feb 17, 2025

		@@ -95,16 +95,16 @@ def before_exit(message, _exit_code)
		def sync_blacklisted_events

		@@ -57,7 +57,7 @@ def self.http_proxy
		end

		def self.default_blacklisted_event_names

		Array(::Settings.ems["ems_#{provider_name.underscore}"].try(:blacklisted_event_names))
		Array(::Settings.ems["ems_#{ems_type}"].try(:blacklisted_event_names))

Use settings for filtered events #23320

Use settings for filtered events #23320

Conversation

agrare commented Jan 29, 2025 • edited by Fryguy Loading

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

Fryguy commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Fryguy commented Jan 29, 2025 • edited Loading

agrare commented Jan 30, 2025

agrare commented Jan 30, 2025

jrafanie commented Jan 31, 2025

Fryguy commented Jan 31, 2025 • edited Loading

agrare commented Jan 31, 2025

Fryguy commented Feb 14, 2025

Fryguy left a comment

Choose a reason for hiding this comment

agrare commented Feb 17, 2025

agrare commented Feb 17, 2025

agrare commented Jan 29, 2025 •

edited by Fryguy

Loading

agrare Jan 29, 2025 •

edited

Loading

agrare Jan 31, 2025 •

edited

Loading

Fryguy commented Jan 29, 2025 •

edited

Loading

Fryguy commented Jan 31, 2025 •

edited

Loading