Miq Alert Status model changes for resolved alerts #14295

moolitayer · 2017-03-13T16:42:26Z

ancestry was added in Model changes for alerts #13233 to model resolved alerts
the intention was to link an open alert with a resolved one and figure out using ancestry which alerts to show in the screen (only opening ones, the have no resolving mas). It became apparent it would be more straight forward to represent that using one MAS with a new field(see next commit) and that would also make retrival more performant (save an N+1 issue).
resolved means the problem represented by the alerts has gone away
ems_ref is a reference in the provider to the event that created the alert and allows to find the opening alert when the resolving one comes into the system

moolitayer · 2017-03-13T16:48:04Z

@miq-bot add_label enhancement, providers/containers

simon3z · 2017-03-14T08:03:43Z

@moolitayer it seems that there are Travis failures to fix.
@miq-bot assign moolitayer

moolitayer · 2017-03-14T15:34:22Z

db/migrate/20170309125642_add_alert_ems_ref.rb

+class AddAlertEmsRef < ActiveRecord::Migration[5.0]
+  def change
+    add_column :miq_alert_statuses, :ems_ref, :string
+    add_index :miq_alert_statuses, :ems_ref


@kbrock I'm pretty sure this isn't the index I need, here is the query using it:
https://github.com/moolitayer/manageiq/blob/f423f61dbc0485eee72f2bb153ce1303e70a96dd/app/models/miq_alert.rb#L217
An explain of that query:

QUERY PLAN ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ---------------------------------------- Limit (cost=0.00..1.04 rows=1 width=239) -> Seq Scan on miq_alert_statuses (cost=0.00..1.04 rows=1 width=239) Filter: ((miq_alert_id = '1000000000028'::bigint) AND ((resource_type)::text = 'ContainerNode'::text) AND (resource_id = '1000000000001'::bigint) AND ((ems_ref)::text = 'ops-example-1489487734309-58f1aa 1c-dafe-4285-bbae-ec7a788de50d'::text)) (3 rows)

@kbrock I think a good solution might be to have an index on event_ems_ref and remove the resource part from the query. WDYT?

https://github.com/moolitayer/manageiq/blob/f423f61dbc0485eee72f2bb153ce1303e70a96dd/app/models/miq_alert.rb#L217

moolitayer · 2017-03-14T15:41:19Z

@simon3z please review, this is the most pressing PR for me ATM, as many others depend on it.

moolitayer · 2017-03-15T09:50:37Z

@simon3z This is the most urgent PR for me ATM, most others are dependent on it

simon3z · 2017-03-15T15:07:56Z

@simon3z This is the most urgent PR for me ATM, most others are dependent on it

@moolitayer then I think you should seek some code reviews; who is familiar with this part?
When you introduced this in #13233 @cben @zeari and @Fryguy reviewed/commented (not sure but you may have also mentioned some help from @kbrock).
Involving them would be a good starting point.

cben · 2017-03-15T19:32:14Z

The PR itself is straightforward 👍 but I'd like to better understand what ems_ref means here.
If I understand #14300 right:

MAS ems_ref will be set from the EmsEvent ems_ref set from the hawkular alert id.
when an alert gets resolved in hawkular, it's modified in-place, retaining same id, and we'll get a second EmsEvent with same ems_ref, and second MAS with same ems_ref
and then you'd be able to relate resolved MAS to previous MAS by looking for same ems_ref

Assuming that's right: Having several records with same ems_ref feels weird, normally this field name is a unique identifier. I don't have a better suggestion for EmsEvent — we do want 2 events for one hawkular alert — but for MAS what do you think of calling it event_ems_ref?
(Although the event is just a transport detail. Conceptually the interesting relation is between MASes and a hawkular alert.)

miq-bot · 2017-03-16T09:48:07Z

Checked commits moolitayer/manageiq@6851d68~...5ef9392 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🏆

moolitayer · 2017-03-16T09:49:44Z

@cben a nicer solution model wise would be to link the MAS to the EmsEvent that caused it and use that to do the join. Unfortunately that would produce complex queries on the join for an incoming EmsEvent. I chose this way for practicality.

I've changed the name to event_ems_ref as suggested

moolitayer · 2017-03-16T09:54:17Z

@miq-bot assign @simon3z

moolitayer · 2017-03-16T12:36:10Z

@cben please review

moolitayer · 2017-03-16T13:01:39Z

Thanks @cben @zeari. @simon3z Please review.

simon3z · 2017-03-16T13:18:16Z

LGTM 👍

@miq-bot assign Fryguy

kbrock

This is great!
Can feel the performance improvements already ;)

Think the only thing missing here is the data migration

kbrock · 2017-03-16T14:04:02Z

app/models/miq_alert_status.rb

 class MiqAlertStatus < ApplicationRecord
  SEVERITY_LEVELS = %w(error warning info).freeze

-  has_ancestry


kbrock · 2017-03-16T14:04:49Z

db/migrate/20170309111313_add_resolved_to_alerts.rb

@@ -0,0 +1,5 @@
+class AddResolvedToAlerts < ActiveRecord::Migration[5.0]
+  def change
+    add_column :miq_alert_statuses, :resolved, :boolean


think we want to populate in a data migration

We do not need that since resolved alerts are only used in the datawarehouse provider which is prototype and hidden in the product. Also resolved alerts are not yet collected (that pr is being reviewed as well #14299)

Fryguy · 2017-03-16T17:28:30Z

db/migrate/20170309125642_add_alert_ems_ref.rb

+class AddAlertEmsRef < ActiveRecord::Migration[5.0]
+  def change
+    add_column :miq_alert_statuses, :event_ems_ref, :string
+    add_index :miq_alert_statuses, :event_ems_ref


Is this ems_ref related to the manageiq events table at all? If not, then this name might be confusing.

Or I can just read back in the thread 😄

Fryguy · 2017-03-16T17:41:50Z

Having several records with same ems_ref feels weird

It's worse than weird, it's a potential problem. In the future we are considering making ems_id, ems_ref a unique index to help with some refresh issues, so that will be a problem in the future if there are duplicates.

miq-bot added enhancement providers/containers sql migration labels Mar 13, 2017

moolitayer mentioned this pull request Mar 13, 2017

Pass additional metadata from alert to event #14301

Merged

chessbyte assigned simon3z and unassigned simon3z Mar 13, 2017

miq-bot assigned moolitayer and unassigned simon3z Mar 14, 2017

This was referenced Mar 14, 2017

[WIP] Resolved alerts #14121

Closed

Datawarehouse: allow more then one alert per resource #14300

Merged

Add resolved to MAS

6851d68

moolitayer force-pushed the mas_model_changes branch from 7d45443 to 1b6821a Compare March 14, 2017 15:00

moolitayer commented Mar 14, 2017

View reviewed changes

moolitayer mentioned this pull request Mar 14, 2017

Retrieve resolved alerts from hawkular #14299

Closed

Mooli Tayer added 2 commits March 16, 2017 11:42

Add event_ems_ref to MAS

3a69b3a

Remove ancestry from MAS

5ef9392

moolitayer force-pushed the mas_model_changes branch from 1b6821a to 5ef9392 Compare March 16, 2017 09:43

miq-bot assigned simon3z and unassigned moolitayer Mar 16, 2017

cben approved these changes Mar 16, 2017

View reviewed changes

zeari approved these changes Mar 16, 2017

View reviewed changes

miq-bot assigned Fryguy and unassigned simon3z Mar 16, 2017

kbrock reviewed Mar 16, 2017

View reviewed changes

Fryguy reviewed Mar 16, 2017

View reviewed changes

Fryguy merged commit a9e013d into ManageIQ:master Mar 16, 2017

Fryguy added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miq Alert Status model changes for resolved alerts #14295

Miq Alert Status model changes for resolved alerts #14295

moolitayer commented Mar 13, 2017 •

edited

Loading

moolitayer commented Mar 13, 2017

simon3z commented Mar 14, 2017

moolitayer Mar 14, 2017 •

edited

Loading

moolitayer Mar 16, 2017

moolitayer commented Mar 14, 2017 •

edited

Loading

moolitayer commented Mar 15, 2017

simon3z commented Mar 15, 2017

cben commented Mar 15, 2017

miq-bot commented Mar 16, 2017

moolitayer commented Mar 16, 2017

moolitayer commented Mar 16, 2017

moolitayer commented Mar 16, 2017

moolitayer commented Mar 16, 2017

simon3z commented Mar 16, 2017

kbrock left a comment

kbrock Mar 16, 2017

kbrock Mar 16, 2017

moolitayer Mar 16, 2017

Fryguy Mar 16, 2017

Fryguy Mar 16, 2017

Fryguy commented Mar 16, 2017

Miq Alert Status model changes for resolved alerts #14295

Miq Alert Status model changes for resolved alerts #14295

Conversation

moolitayer commented Mar 13, 2017 • edited Loading

moolitayer commented Mar 13, 2017

simon3z commented Mar 14, 2017

moolitayer Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer Mar 16, 2017

Choose a reason for hiding this comment

moolitayer commented Mar 14, 2017 • edited Loading

moolitayer commented Mar 15, 2017

simon3z commented Mar 15, 2017

cben commented Mar 15, 2017

miq-bot commented Mar 16, 2017

moolitayer commented Mar 16, 2017

moolitayer commented Mar 16, 2017

moolitayer commented Mar 16, 2017

moolitayer commented Mar 16, 2017

simon3z commented Mar 16, 2017

kbrock left a comment

Choose a reason for hiding this comment

kbrock Mar 16, 2017

Choose a reason for hiding this comment

kbrock Mar 16, 2017

Choose a reason for hiding this comment

moolitayer Mar 16, 2017

Choose a reason for hiding this comment

Fryguy Mar 16, 2017

Choose a reason for hiding this comment

Fryguy Mar 16, 2017

Choose a reason for hiding this comment

Fryguy commented Mar 16, 2017

moolitayer commented Mar 13, 2017 •

edited

Loading

moolitayer Mar 14, 2017 •

edited

Loading

moolitayer commented Mar 14, 2017 •

edited

Loading