Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Delete Prometheus Alerts MonitoringManager #518

Merged
merged 3 commits into from
Mar 4, 2024
Merged

Delete Prometheus Alerts MonitoringManager #518

merged 3 commits into from
Mar 4, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Feb 29, 2024 •
edited
Loading

@agrare agrare requested review from cben and Fryguy as code owners February 29, 2024 19:17
@kbrock
Copy link
Member

kbrock commented Mar 1, 2024

think this was a valid failure

Failure/Error: parser.get_additional_attributes_graph(inventory)

@agrare
Copy link
Member Author

agrare commented Mar 1, 2024

Interesting, I can't imagine how...

@agrare agrare force-pushed the remove_prometheus_alert_buffer_gem branch from 9c89854 to 03d86c3 Compare March 1, 2024 16:50
@agrare
Copy link
Member Author

agrare commented Mar 1, 2024

@kbrock those are pending specs, the real failure is that we still have the PrometheusAlerts MonitoringManager here. This will take some more work to untangle as we'll need a data migration to remove the records.

I did find an opportunity to remove some tech debt here though as we're including HasInfraManagerMixin and HasMonitoringManagerMixin in the base ContainerManager class which isn't right. Opened ManageIQ/manageiq#22926 to undo this.

@agrare agrare mentioned this pull request Mar 1, 2024
Merged
4 tasks
Fryguy
Fryguy reviewed Mar 1, 2024
View reviewed changes
config/settings.yml
:ems_monitoring:
:alerts_collection:
:open_timeout: 10.seconds
:timeout: 20.seconds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a database migration to delete any changes in SettingsChanges?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, for that matter, any existing workers or queue items?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and/or the EMS itself?

Copy link
Member Author

@agrare agrare Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fryguy ☝️ #518 (comment)

This will take some more work to untangle as we'll need a data migration to remove the records.

@kbrock
Copy link
Member

kbrock commented Mar 1, 2024 •
edited
Loading

ok. I reproduced the error locally. (and it does not show up in master for me)

rspec spec/manageiq/spec/models/miq_worker/systemd_common_spec.rb:5
       the missing elements were:
["manageiq-providers-kubernetes_monitoring_manager_event_catcher.target",
"manageiq-providers-kuberne...ger_event_catcher.target",
"manageiq-providers-openshift_monitoring_manager_event_catcher@.service"]

kbrock
kbrock reviewed Mar 1, 2024
View reviewed changes
app/models/manageiq/providers/kubernetes/container_manager.rb
Comment on lines -25 to -29
def monitoring_manager_needed?
connection_configurations.roles.include?(
ManageIQ::Providers::Kubernetes::MonitoringManagerMixin::ENDPOINT_ROLE.to_s
)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if this is causing the issue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I just need to delete the systemd units here

@agrare
Copy link
Member Author

agrare commented Mar 1, 2024 •
edited
Loading

Haha slow down guys I'm working on it, okay removed the systemd units for the monitoring manager.

@agrare agrare force-pushed the remove_prometheus_alert_buffer_gem branch from 03d86c3 to e466992 Compare March 1, 2024 17:14
@agrare agrare mentioned this pull request Mar 1, 2024
Merged
@agrare agrare mentioned this pull request Mar 1, 2024
Merged
@miq-bot
Copy link
Member

miq-bot commented Mar 1, 2024

Checked commits agrare/manageiq-providers-kubernetes@e005f4d~...f7b87fe with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. ⭐

@agrare agrare mentioned this pull request Mar 1, 2024
Closed
@agrare agrare changed the title Remove the prometheus-alert-buffer-client gem Delete Prometheus Alerts MonitoringManager Mar 1, 2024
@Fryguy
Copy link
Member

Fryguy commented Mar 1, 2024 •
edited
Loading

Haha slow down guys I'm working on it,

🤣 We're all so excited for the 🔥 🗑️ 💣 ✂️

(I suggest WIP it 😛)

@agrare
Copy link
Member Author

agrare commented Mar 1, 2024

(I suggest WIP it 😛)

No need now, was just making a flurry of changes earlier and some things were broken but it is good now

@Fryguy Fryguy closed this Mar 4, 2024
@Fryguy Fryguy reopened this Mar 4, 2024
@Fryguy Fryguy self-assigned this Mar 4, 2024
@Fryguy Fryguy merged commit 02858d8 into ManageIQ:master Mar 4, 2024
3 of 6 checks passed
@agrare agrare deleted the remove_prometheus_alert_buffer_gem branch March 4, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbrock kbrock kbrock left review comments

@Fryguy Fryguy Fryguy approved these changes

@cben cben Awaiting requested review from cben cben is a code owner

Assignees

@Fryguy Fryguy

Labels
dependencies technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agrare @kbrock @miq-bot @Fryguy