Add alerts on container nodes #13323
Conversation
|
@miq-bot add_label providers/containers, enhancement |
|
@simon3z please review, this was not mentioned but is also required for next week's goal |
|
@moolitayer I assume we'll have more of these depending on what entities we can attach alerts to. Do we support nodes at this point? |
| @@ -19,6 +19,7 @@ class ContainerManager < BaseManager | |||
| has_many :container_build_pods, :foreign_key => :ems_id, :dependent => :destroy | |||
| has_many :container_templates, :foreign_key => :ems_id, :dependent => :destroy | |||
| has_one :container_deployment, :foreign_key => :deployed_ems_id, :inverse_of => :deployed_ems | |||
| has_many :miq_alert_statuses, :through => :container_nodes | |||
There was a problem hiding this comment.
@moolitayer I don't think this is optimal as we grow in number of entities supporting alerts. You should probably take all the MiqAlertStatuses that are related to this provider (not just the nodes ones).
There was a problem hiding this comment.
I think we can handle that when we add more entities?
What I was thinking of doing when we add more container entities that have miq_alert_statuses is writing custom code for the join that will look at multiple tables (I think conceptually join is the best solution here).
@kbrock is there a better solution? we will need a way to has_many through multiple tables basically.
(=== has_many :miq_alert_statuses, :through => [:container_nodes, :container_groups ...])
There was a problem hiding this comment.
Yea, I've got nothing here.
Either associate all alert statuses directly to the container (or container_node) or modify the table to add a manager_id - even if you could define this in an active record way, the performance would be terrible.
Right we will have more, see response to your code comment. We will support nodes after this pr (the actual collection bringing in miq_alert_statuses on nodes as well as code allowing to define miq_alerts on them is in #12773) |
moolitayer
force-pushed
the
add_node_alerts
branch
from
74fe046 to
fb24cc7
Compare
|
@simon3z This is only to add the link now after suggested changes PTAL |
|
Checked commit moolitayer@fb24cc7 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
|
@moolitayer I have the feeling we don't want to lose the alerts statuses when a resource disappears ( |
|
@simon3z note this is how miq_alerts_statuses are attached to all other objects in the system. @blomquisg can you please point me to somewhere where we purge data that is not dependent destroy? |
@moolitayer if we end up dropping the history when entities disappear (not suggesting to go that way, just saying) then we need it (at the very least) to be very well documented and understood by the end user. |
|
LGTM 👍 cc @chessbyte @miq-bot assign blomquisg |
No description provided.