ensure monitoring manager deletion and creation on endpoint update #16635

zeari · 2017-12-11T10:52:37Z

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

Updating the monitoring endpoint on an existing provider did not create or delete the monitoring manager accordingly.

Completed by ManageIQ/manageiq-providers-kubernetes#188

@moolitayer @cben @nimrodshn @yaacov Please review

@miq-bot add_label providers/containers

zeari · 2017-12-11T11:01:50Z

app/models/mixins/has_monitoring_manager_mixin.rb

+
+  def ensure_monitoring_manager_deletion
+    monitoring_manager.try(:delete_queue) unless try(:monitoring_manager_needed?)
+  end


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.

maybe we could fail the save if monitoring_manager.enabled = false to reduce the chance of that?

yaacov

LGTM 👍

moolitayer · 2017-12-11T13:24:52Z

@masayag since you are doing something similar in case this is helpful

cben · 2017-12-13T09:02:02Z

app/models/mixins/has_monitoring_manager_mixin.rb

      monitoring_manager.zone_id = zone_id
      monitoring_manager.provider_region = provider_region
    end
  end
+
+  def ensure_monitoring_manager_deletion
+    monitoring_manager.try(:delete_queue) unless try(:monitoring_manager_needed?)


if try(:monitoring_manager_needed?) fails — if it doesn't respond to such method — is it correct to delete?

I think the openshift provider is the only one who uses the monitoring manager so its up to us. As best as i can tell, yes. Because if you dont have the endpoint\role for monitoring then you dont need the manager.
We wont lose data the collected since the MiqAlerts are associated with the parent ems and not this manager.

yes these should only be called for container managers, and they all implement monitoring_manager_needed? since it's in container manager mixin. I suspect this was only done to allow merging the manageiq PR first with out breaking anything. @zeari can we loose the try?

zeari · 2017-12-14T11:50:55Z

@bazulay @moolitayer gaprindashvili/yes?

zeari · 2017-12-14T12:57:28Z

@cben @moolitayer I unified the functionality of creation\deletion under before_save

bdunne · 2017-12-14T13:08:10Z

app/models/mixins/has_monitoring_manager_mixin.rb

      monitoring_manager.zone_id = zone_id
      monitoring_manager.provider_region = provider_region
+    elsif try(:monitoring_manager_needed?) == false && monitoring_manager.present?
+      monitoring_manager.try(:delete_queue) unless try(:monitoring_manager_needed?)


I don't think you need the unless here based on the condition above, right?

Youre right. I was moving stuff around with @moolitayer

moolitayer · 2017-12-14T13:15:36Z

@bazulay @moolitayer gaprindashvili/yes?

I think so, according to your input this bug would completely prevent monitoring from working if the endpoint isn't added on container manager creation. Please create a bz for it.

moolitayer

LGTM 👍

yaacov · 2017-12-17T07:50:47Z

app/models/mixins/has_monitoring_manager_mixin.rb

      monitoring_manager.zone_id = zone_id
      monitoring_manager.provider_region = provider_region
+    elsif !monitoring_manager_needed? && monitoring_manager.present?
+      # TODO:: if someone deletes the alerts endpoint and then quickly readds it they can end up without a manager.


@zeari 👍 for comments!!

yaacov · 2017-12-17T09:58:07Z

👍 nice!

cben · 2017-12-18T09:09:42Z

app/models/mixins/has_monitoring_manager_mixin.rb

      monitoring_manager.zone_id = zone_id
      monitoring_manager.provider_region = provider_region
+    elsif !monitoring_manager_needed? && monitoring_manager.present?
+      # TODO: if someone deletes the alerts endpoint and then quickly readds it they can end up without a manager.


LGTM, I don't see any easy fix, and hard fixes are out of scope.
I wonder if long-term we'll want API & UI to actually create/delete managers — and deal with manager deletion being async — rather than just creating/deleting endpoints.

moolitayer

Remove ack

zeari · 2017-12-21T16:55:13Z

I moved some code to endpoint.rb

@miq-bot add_label wip

cben · 2017-12-21T17:23:21Z

app/models/endpoint.rb

+    monitoring_manager = ems.try(:monitoring_manager)
+
+    # Make sure monitoring manager is deleted\created for the prometheus endpoint
+    if role == "prometheus_alerts" && monitoring_manager.nil?


Nice.
To keep Endpoint class mostly provider-agnostic, consider keeping the logic in HasMonitoringManagerMixin method(s), with the hooks here reduced to something like

ems.endpoint_created(role) if ems.responds_to?(:endpoint_created)

zeari · 2017-12-24T10:25:20Z

@miq-bot remove_label wip

zeari · 2017-12-24T11:45:45Z

Still doesnt work with the API

@miq-bot add_label wip

moolitayer · 2017-12-24T13:38:12Z

This DOES work with the API.
@moolitayer ManageIQ/manageiq-providers-kubernetes#188 (comment)
The role is prometheus_alerts instead of prometheus-alerts (underscore and not dash)

Nice catch, updated!

zeari · 2017-12-25T10:23:00Z

This PR and ManageIQ/manageiq-providers-kubernetes#188 need to be merged together.
I ran the failed travis tests locally against ManageIQ/manageiq-providers-kubernetes#188 and didnt get any failures.

cben

👍

cben · 2017-12-25T12:06:45Z

app/models/mixins/has_monitoring_manager_mixin.rb

-      monitoring_manager.provider_region = provider_region
+  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.


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.

Ah, even less of a risk now that the logic moved to endpoint.rb.

zeari · 2017-12-25T12:36:35Z

@bazulay @moolitayer gaprindashvili/yes?

I think so, according to your input this bug would completely prevent monitoring from working if the endpoint isn't added on container manager creation. Please create a bz for it.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1528955
@miq-bot add_label gaprindashvili/yes

moolitayer

LGTM 👍

moolitayer · 2017-12-25T20:50:46Z

app/models/mixins/has_monitoring_manager_mixin.rb

@@ -1,21 +1,28 @@
 module HasMonitoringManagerMixin
  extend ActiveSupport::Concern

-  private
+  def ensure_monitoring_manager_with_params


I liked the old name better. also why not keep private?

we can keep the old name

cant be private since we access this through Endpoint

I think ensure_monitoring_manager_with_params should be private and endpoint_created/endpoint_destroyed should be public, right?

zeari · 2017-12-28T12:00:54Z

I think ensure_monitoring_manager_with_params should be private and endpoint_created/endpoint_destroyed should be public, right?

@moolitayer keeping ensure_monitoring_manager made the tests pass 😄

zeari · 2018-01-04T14:08:41Z

@agrare PTAL

agrare · 2018-01-04T14:49:09Z

app/models/endpoint.rb

+  after_destroy :endpoint_destroyed
+
+  def endpoint_created
+    # Make sure monitoring manager is created for the prometheus endpoint


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

Right, removing.

agrare

👍 nice change @zeari

agrare · 2018-01-04T14:55:25Z

Thanks @zeari will merge when green

miq-bot · 2018-01-04T14:56:20Z

Checked commits zeari/manageiq@474010f~...8f771b9 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

ensure monitoring manager deletion and creation on endpoint update (cherry picked from commit 272f1e6) https://bugzilla.redhat.com/show_bug.cgi?id=1531293

simaishi · 2018-01-04T23:00:05Z

Gaprindashvili backport details:

$ git log -1
commit 47636cae5e62b274fd7987f23b5dab884eb7f0e1
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Jan 4 10:23:26 2018 -0500

    Merge pull request #16635 from zeari/monitoring_manager_deletion
    
    ensure monitoring manager deletion and creation on endpoint update
    (cherry picked from commit 272f1e638b984a9620b51293f9379c16ca9c31ee)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1531293

miq-bot added the providers/containers label Dec 11, 2017

zeari mentioned this pull request Dec 11, 2017

ensure monitoring manager is created or deleted on provider update ManageIQ/manageiq-providers-kubernetes#188

Merged

zeari commented Dec 11, 2017

View reviewed changes

yaacov approved these changes Dec 11, 2017

View reviewed changes

cben reviewed Dec 13, 2017

View reviewed changes

zeari force-pushed the monitoring_manager_deletion branch from de77506 to 25ab150 Compare December 14, 2017 12:55

bdunne reviewed Dec 14, 2017

View reviewed changes

zeari force-pushed the monitoring_manager_deletion branch from 25ab150 to 84505b7 Compare December 14, 2017 13:09

moolitayer approved these changes Dec 14, 2017

View reviewed changes

yaacov reviewed Dec 17, 2017

View reviewed changes

zeari force-pushed the monitoring_manager_deletion branch from 84505b7 to a0a6cb0 Compare December 17, 2017 09:39

cben approved these changes Dec 18, 2017

View reviewed changes

moolitayer suggested changes Dec 19, 2017

View reviewed changes

ensure monitoring manager deletion and creation on provider update

474010f

zeari force-pushed the monitoring_manager_deletion branch from a0a6cb0 to cd86fb3 Compare December 21, 2017 16:50

miq-bot changed the title ~~ensure monitoring manager deletion and creation on provider update~~ [WIP] ensure monitoring manager deletion and creation on provider update Dec 21, 2017

miq-bot added the wip label Dec 21, 2017

cben reviewed Dec 21, 2017

View reviewed changes

zeari force-pushed the monitoring_manager_deletion branch from cd86fb3 to b38e8e7 Compare December 24, 2017 10:24

miq-bot changed the title ~~[WIP] ensure monitoring manager deletion and creation on provider update~~ ensure monitoring manager deletion and creation on provider update Dec 24, 2017

miq-bot removed the wip label Dec 24, 2017

miq-bot changed the title ~~ensure monitoring manager deletion and creation on provider update~~ [WIP] ensure monitoring manager deletion and creation on provider update Dec 24, 2017

zeari force-pushed the monitoring_manager_deletion branch from b38e8e7 to 6d60e97 Compare December 24, 2017 13:36

zeari force-pushed the monitoring_manager_deletion branch from 6d60e97 to bb88857 Compare December 24, 2017 13:51

cben approved these changes Dec 25, 2017

View reviewed changes

miq-bot added the gaprindashvili/yes label Dec 25, 2017

monitoring manager building to endpoint

79c12d5

zeari force-pushed the monitoring_manager_deletion branch from bb88857 to 79c12d5 Compare December 25, 2017 13:51

moolitayer approved these changes Dec 25, 2017

View reviewed changes

moolitayer reviewed Dec 25, 2017

View reviewed changes

shorten to ensure_monitoring_manager

565916c

masayag mentioned this pull request Dec 31, 2017

Support KubeVirt provider ManageIQ/manageiq-providers-kubernetes#197

Merged

moolitayer mentioned this pull request Jan 2, 2018

Introduce virtualization manager #16721

Merged

zeari changed the title ~~ensure monitoring manager deletion and creation on provider update~~ ensure monitoring manager deletion and creation on endpoint update Jan 4, 2018

agrare reviewed Jan 4, 2018

View reviewed changes

agrare self-assigned this Jan 4, 2018

agrare approved these changes Jan 4, 2018

View reviewed changes

remove monitoring manager specific comments from endpoint.rb

8f771b9

agrare merged commit 272f1e6 into ManageIQ:master Jan 4, 2018

agrare added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 4, 2018

agrare added the bug label Jan 4, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 4, 2018

ensure monitoring manager deletion and creation on endpoint update #16635

ensure monitoring manager deletion and creation on endpoint update #16635

Conversation

zeari commented Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov left a comment

Choose a reason for hiding this comment

moolitayer commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari commented Dec 14, 2017

zeari commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Dec 14, 2017

moolitayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Dec 17, 2017

Choose a reason for hiding this comment

moolitayer left a comment

Choose a reason for hiding this comment

zeari commented Dec 21, 2017

Choose a reason for hiding this comment

zeari commented Dec 24, 2017

zeari commented Dec 24, 2017

moolitayer commented Dec 24, 2017

zeari commented Dec 25, 2017

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari commented Dec 25, 2017

moolitayer 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

zeari commented Dec 28, 2017

zeari commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

agrare commented Jan 4, 2018

miq-bot commented Jan 4, 2018

simaishi commented Jan 4, 2018

zeari commented Dec 11, 2017 •

edited

Loading