Notify users of killing workers when exceed memory #17673

kbrock · 2018-07-06T00:09:40Z

Overview

Notifications provide information about events to users.
The workflows for larger customers rely more upon email than logging into the console.

manageiq can send out emails for these events using alerts. These emails do not have the rich information that the notifications provide.

In our particular case, when a workers is killed, only the server information is provided, not the worker itself.

Before

Alert Triggered
Alert 'aKB Worker killed', triggered

Event:	Alert condition met
Entity:	(MiqServer) local

After

Alert Triggered
Alert 'aKB Worker killed', triggered

Event:	Alert condition met
Entity:	(MiqServer) local
Details:	Killing worker MiqPriorityWorker due to excessive memory usage. 144.6 MB used memory exceeds limit of 100 MB.

This PR adds a details section to the Alert emails.

Improves information for out of memory events
Add notification text for out of memory errors
Introduce ability to disable notification while keeping the notification template
Include notification#message in alert emails

Followup:

Put gook into Email Event Details for notification emails manageiq-ui-classic#4291

https://bugzilla.redhat.com/show_bug.cgi?id=1535177

carbonin · 2018-08-01T14:08:17Z

app/models/notification_type.rb

    end
  end

+  # this disables notifications, but allows the notification to still exist
+  # this notification template can be used for emails
+  def none?


I don't really like the name. At first glance it feels like Enumerable#none? which is completely unrelated.

Maybe has_audience? or even something like enabled? (or the negative of either of those)?

carbonin · 2018-08-01T14:12:58Z

If we're not raising the notification to the user (audience type "none") why are we creating it? Is is just for the string interpolation based on type? That's only there for translations. Is there no other way to get this particular string translated for the email?

I'm okay with this approach if there's a reason behind it, but I don't think I understand that reason just yet.

gmcculloug

Two minor comments. @gtanzillo Do you have thoughts on the Notification side of things?

gmcculloug · 2018-08-01T16:14:58Z

app/models/notification.rb

+    Notification.create(:notification_type => type,
+                        :options           => event.full_data,
+                        :subject_id        => event.target_id,
+                        :subject_type      => event.target_type)


Do subject id and type need to be broken out or can it remain as it was before :subject => event.target

I liked the idea of not looking up the target object, but reverting back to original is fine

gmcculloug · 2018-08-01T16:21:19Z

app/models/notification.rb

@@ -41,6 +45,12 @@ def seen_by_all_recipients?
    notification_recipients.unseen.empty?
  end

+  def self.notification_text(event_type, full_data)
+    return unless NotificationType.names.include?(event_type) && full_data


Can you avoid the NotificationType.names lookup when full_data is nil by swapping the order of this check?

kbrock · 2018-08-01T20:38:40Z

@carbonin The idea is to use NotificationType as a template for the messages.
That way the existing alerts (that also have notification types) will be enhanced.

audience: none allows admins to disable a notification types without deleting it from the database. But I suppose our seed process doesn't make that very practical.

Is there another place to put email message templates that work similar to notification templates?

carbonin · 2018-08-01T21:05:20Z

That way the existing alerts (that also have notification types) will be enhanced.

Ah, that makes sense.

Is there another place to put email message templates that work similar to notification templates?

No that I know of, I was just curious.

gtanzillo · 2018-08-09T21:44:54Z

@gtanzillo Do you have thoughts on the Notification side of things?

I'm good with this change to notifications. I like the fact that the messages can be normalized for both notifications and email actions 👍

gtanzillo

I'm good with this 👍

Fryguy · 2018-08-10T15:18:30Z

app/models/miq_server/worker_management/monitor/validation.rb

@@ -25,7 +25,16 @@ def validate_worker(w)
    if MiqWorker::STATUSES_CURRENT.include?(w.status) && usage_exceeds_threshold?(usage, memory_threshold)
      msg = "#{w.format_full_log_msg} process memory usage [#{usage}] exceeded limit [#{memory_threshold}], requesting worker to exit"
      _log.warn(msg)
-      MiqEvent.raise_evm_event_queue(w.miq_server, "evm_worker_memory_exceeded", :event_details => msg, :type => w.class.name)
+      helper = ApplicationController.helpers


@jrafanie Does/Will this cause an issue with respect to your work to run core without the UI code?

@kbrock, @jrafanie I'm not saying my comment should hold up the merge, but we may want to take note of it somewhere as a dependency on some UI functionality that may need to be shared if the UI plugin is removed.

I'm more than happy removing the helpers clause - I put that in to get the pretty megabytes thing displayed (that you suggested ;) )

yeah, we shouldn't depend on ApplicationController here. I'm all for keeping raw bytes and changing the presentation to make it pretty.

There is currently no place for the formatting.

I'll move over to ActiveSupport - that should be included in all gems (I assume)

Fryguy · 2018-08-10T15:22:09Z

app/models/notification.rb

@@ -41,6 +44,12 @@ def seen_by_all_recipients?
    notification_recipients.unseen.empty?
  end

+  def self.notification_text(event_type, full_data)


This seems very event specific in it's wording, but the functionality seems general. Perhaps just rename event_type to name and full_data to message_params or something? Same goes for the emit_for_event method, but at least that method has event in the name (I question whether an event specific thing belongs in Notification as opposed to EventStream)

Fryguy · 2018-08-10T15:24:11Z

This looks pretty good to me. I question coding event specific things into Notification class, but that's just organization problem. Overall, this is good stuff 👍

- Provide more complete information for worker out of memory errors - Provide notification messages for out of memory errors - Introduce ability to disable notification - Include notification.message in alert emails https://bugzilla.redhat.com/show_bug.cgi?id=1535177

miq-bot · 2018-08-10T16:07:10Z

Checked commit kbrock@ebd613a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. ⭐

jrafanie

I'm 👍 now that it's 100% not dependent on ActionController.

kbrock added the enhancement label Jul 6, 2018

kbrock force-pushed the monitor_notifier_v3 branch from 544b88e to f861c69 Compare July 6, 2018 17:51

kbrock mentioned this pull request Jul 13, 2018

Event Details for notification emails ManageIQ/manageiq-ui-classic#4291

Merged

kbrock changed the title ~~notify users of killing workers when exceed memory~~ [WIP] notify users of killing workers when exceed memory Jul 26, 2018

kbrock added wip core/events labels Jul 26, 2018

kbrock force-pushed the monitor_notifier_v3 branch 3 times, most recently from 5bd0b97 to 6bfd073 Compare July 31, 2018 19:17

kbrock removed the wip label Jul 31, 2018

kbrock changed the title ~~[WIP] notify users of killing workers when exceed memory~~ Notify users of killing workers when exceed memory Jul 31, 2018

kbrock force-pushed the monitor_notifier_v3 branch 3 times, most recently from cd6c4f4 to a32c1fd Compare August 1, 2018 05:30

kbrock added the bug label Aug 1, 2018

carbonin reviewed Aug 1, 2018

View reviewed changes

gmcculloug assigned gmcculloug and unassigned gmcculloug Aug 1, 2018

gmcculloug reviewed Aug 1, 2018

View reviewed changes

kbrock force-pushed the monitor_notifier_v3 branch from a32c1fd to 4ffa807 Compare August 1, 2018 20:43

kbrock force-pushed the monitor_notifier_v3 branch from 4ffa807 to 82c3d3f Compare August 2, 2018 15:46

gtanzillo approved these changes Aug 10, 2018

View reviewed changes

Fryguy reviewed Aug 10, 2018

View reviewed changes

kbrock force-pushed the monitor_notifier_v3 branch from 82c3d3f to 4c5de3c Compare August 10, 2018 16:02

kbrock force-pushed the monitor_notifier_v3 branch from 4c5de3c to ebd613a Compare August 10, 2018 16:05

jrafanie approved these changes Aug 10, 2018

View reviewed changes

gmcculloug approved these changes Aug 13, 2018

View reviewed changes

kbrock mentioned this pull request Aug 13, 2018

[wip] notify users of killing workers when exceed memory #17640

Closed

gtanzillo added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 13, 2018

gtanzillo merged commit 68e9e3b into ManageIQ:master Aug 13, 2018

kbrock deleted the monitor_notifier_v3 branch August 13, 2018 18:05

kbrock mentioned this pull request Sep 28, 2018

Fix Notifications None #18035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify users of killing workers when exceed memory #17673

Notify users of killing workers when exceed memory #17673

kbrock commented Jul 6, 2018 •

edited

Loading

carbonin Aug 1, 2018

carbonin commented Aug 1, 2018

gmcculloug left a comment

gmcculloug Aug 1, 2018

kbrock Aug 1, 2018 •

edited

Loading

gmcculloug Aug 1, 2018

kbrock commented Aug 1, 2018 •

edited

Loading

carbonin commented Aug 1, 2018

gtanzillo commented Aug 9, 2018

gtanzillo left a comment

Fryguy Aug 10, 2018

Fryguy Aug 10, 2018

kbrock Aug 10, 2018

jrafanie Aug 10, 2018

kbrock Aug 10, 2018 •

edited

Loading

Fryguy Aug 10, 2018

Fryguy commented Aug 10, 2018

miq-bot commented Aug 10, 2018

jrafanie left a comment

Notify users of killing workers when exceed memory #17673

Notify users of killing workers when exceed memory #17673

Conversation

kbrock commented Jul 6, 2018 • edited Loading

Overview

Before

After

Choose a reason for hiding this comment

carbonin commented Aug 1, 2018

gmcculloug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Aug 1, 2018 • edited Loading

carbonin commented Aug 1, 2018

gtanzillo commented Aug 9, 2018

gtanzillo left a 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

Choose a reason for hiding this comment

kbrock Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Aug 10, 2018

miq-bot commented Aug 10, 2018

jrafanie left a comment

Choose a reason for hiding this comment

kbrock commented Jul 6, 2018 •

edited

Loading

kbrock Aug 1, 2018 •

edited

Loading

kbrock commented Aug 1, 2018 •

edited

Loading

kbrock Aug 10, 2018 •

edited

Loading