-
Notifications
You must be signed in to change notification settings - Fork 898
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.
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
ensure monitoring manager deletion and creation on endpoint update #16635
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,30 @@ | ||
module HasMonitoringManagerMixin | ||
extend ActiveSupport::Concern | ||
|
||
private | ||
def endpoint_created(role) | ||
if role == "prometheus_alerts" && monitoring_manager.nil? | ||
monitoring_manager = ensure_monitoring_manager | ||
monitoring_manager.save | ||
end | ||
end | ||
|
||
def ensure_monitoring_manager | ||
# monitoring_manager should be defined by child classes. | ||
if try(:monitoring_manager_needed?) | ||
build_monitoring_manager(:parent_manager => self) | ||
monitoring_manager.name = "#{name} Monitoring Manager" | ||
def endpoint_destroyed(role) | ||
if role == "prometheus_alerts" && monitoring_manager.present? | ||
# TODO: if someone deletes the alerts endpoint and then quickly readds it they can end up without a manager. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LGTM, I don't see any easy fix, and hard fixes are out of scope. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure this is a real risk. I think the queued destroy will be targeted to a specific manager id, and not affect the new monitoring manager. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, even less of a risk now that the logic moved to |
||
monitoring_manager.destroy_queue | ||
end | ||
ensure_monitoring_manager_properties | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Theres a slight issue here. If a user tries to recreate the alerts endpoint while the previous manager wasnt deleted yet, he could end up with the endpoint but without a manager. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we could fail the save if |
||
|
||
def ensure_monitoring_manager_properties | ||
if monitoring_manager | ||
monitoring_manager.zone_id = zone_id | ||
monitoring_manager.provider_region = provider_region | ||
private | ||
|
||
def ensure_monitoring_manager | ||
if monitoring_manager.nil? | ||
build_monitoring_manager(:parent_manager => self, | ||
:name => "#{name} Monitoring Manager", | ||
:zone_id => zone_id, | ||
:provider_region => provider_region) | ||
end | ||
|
||
monitoring_manager | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a generic callback that could be used for other things I don't think you need to call out the monitoring manager or prometheus endpoint specifically here and here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, removing.