Add monitoring manager #15354

moolitayer · 2017-06-12T12:30:57Z

This is the first item of ManageIQ/manageiq-providers-kubernetes#39

moolitayer · 2017-06-13T17:27:18Z

@simon3z @blomquisg @Ladas please take a look

moolitayer · 2017-06-13T17:28:21Z

app/models/manageiq/providers/monitoring_manager.rb

+  class MonitoringManager < BaseManager
+    class << model_name
+      def route_key
+        "ems_monitoring"


Actually a routing isn't needed here as this provider should not have separate screens

Ladas · 2017-06-14T10:09:20Z

app/models/manageiq/providers/monitoring_manager.rb

+
+      def singular_route_key
+        "ems_monitoring"
+      end


you will need def self.ems_type to pass the specs

hm, strange you have it in the next PR. It should be really only in the leaf class, which might be the problem here, since this is a leaf class, until you define the leaf class.

To pass the specs you will need to ignore this manager in the spec, then merge the leaf class, then re-enable it in the spec :-)

Ladas · 2017-06-14T10:09:49Z

@miq-bot assign @blomquisg

looks good in overall, just need the specs fixed

moolitayer · 2017-06-14T14:49:18Z

Thanks @Ladas I'll probably have a few more changes to this PR once I finish the event collection that uses the new managers.

miq-bot · 2017-06-19T18:36:55Z

This pull request is not mergeable. Please rebase and repush.

moolitayer · 2017-06-27T10:04:12Z

@Ladas @cben I think this is ready can you please review this?

moolitayer · 2017-06-28T13:09:45Z

@Ladas @cben @simon3z please review

cben · 2017-06-28T13:17:24Z

app/models/mixins/has_monitoring_manager_mixin.rb

+  extend ActiveSupport::Concern
+
+  included do
+    # TODO: why is this relation needed ? why is the inheritors relation no enough?


I believe AR class methods like has_one don't create something inheritable (like defining a method).
That's why mixins do this under included to execute in each class instead of in the mixin.

It obviously does work with class->class STI inheritance — dunno how exactly — but not mixins I think.
I may be wrong.

I don't want anything inherited in any case. I was wondering why other secondary managers (NetworkManager I think) had this in the parent class. Why shouldn't I remove this relation and just have the relation in the ~~base~~ child class? (e.g ManageIQ::Providers::Openshift::ContainerManager)

cc @durandom

removing for now

simon3z · 2017-07-03T10:09:59Z

config/permissions.tmpl.yml

@@ -35,6 +35,7 @@
 - ems-type:gce_network
 - ems-type:hawkular
 - ems-type:hawkular_datawarehouse
+- ems-type:monitoring


@moolitayer looking at the other types they're technology-specific (ansible, azure, cinder, ec2, s3, foreman, gce, etc.).

I don't see anything generic (e.g. network, storage, cloud, infra).

Then why "monitoring" instead of probably "prometheus"?

Question is if we will ever want to move existing hawkular stuff under the new manager, like metrics, or introduce a new type of alerting ~~provider~~ implementation

@moolitayer if we do (and we're not planning to) why wouldn't it be a different manager with an hawkular name?
It would be an OpenShift provider with an Hawkular alert manager instead of a Prometheus manager.

Using a generic name here seems out of place.

OK Thanks, turns out this was a mistake, and should only be done for the leaf classes:

#15506

@simon3z turns out I do need it until monitoring manager has children[1]
I will remove it in #15506

[1]

manageiq/app/models/ext_management_system.rb

Lines 10 to 12 in f2d7380

def self.leaf_subclasses

descendants.select { |d| d.subclasses.empty? }

end

moolitayer · 2017-07-04T12:17:53Z

There is a test failure[1] that does not seem related, restarting...

[1] ./spec/models/miq_report_spec.rb:151

moolitayer · 2017-07-05T05:48:08Z

@simon3z please take a look

simon3z · 2017-07-05T09:22:31Z

LGTM 👍 cc @blomquisg

moolitayer · 2017-07-05T10:20:14Z

I'm seeing spec/models/miq_report_spec.rb:151 failing on master locally.

blomquisg · 2017-07-05T14:59:38Z

lib/vmdb/loggers.rb

@@ -26,6 +26,7 @@ def self.apply_config(config)
      apply_config_value(config, $kube_log,          :level_kube)
      apply_config_value(config, $mw_log,            :level_mw)
      apply_config_value(config, $datawarehouse_log, :level_datawarehouse)
+      apply_config_value(config, $monitoring_log,    :level_monitoring)


This seems a little too broad for a logging category (same comment for the logger definition below).

If we're introducing the concept of a Monitor, I could easily see monitoring as a concept spanning multiple providers (OpenStack Gnocci/Ceilometer or AWS CloudWatch for instance). If we refactor some of the other providers to have "monitoring" instead of just "metrics", then I wouldn't want to have a single logger category for that.

In the same way I wouldn't want to have a single "inventory.log" file.

Maybe a better idea here would be "container_monitoring"? I can see why getting more specific with "prometheus_monitoring" or "hawkular_monitoring" is less desirable, since the possibility of reusing those specific services in different providers.

/cc @Fryguy @jrafanie @gtanzillo

Thoughts?

Maybe a better idea here would be "container_monitoring"?

👍

I went with container_monitoring for the log and cn_monitoring_log for the logger, so it won't be super long.

durandom · 2017-07-06T12:21:36Z

app/models/mixins/has_monitoring_manager_mixin.rb

@@ -0,0 +1,21 @@
+module HasMonitoringManagerMixin


prepare for troubles with this approach.

validations and creation time Add managers in before_create callback instead of before_validation manageiq-providers-azure#13

accessing associations through the parent manager [WIP] add hosts and vms as an association #12884

Also what happens if you include HasMonitoringManagerMixin and HasNetworkingManagerMixin ?
It wont chain ensure_managers. I was experimenting with HasParentManager, which would be included into the parent manager, but it also adds some confusion...
So, in the end, I think this linking of managers via parent_manager is flawed, or at least how we set it up.

Unfortunately there is no better way to do this for now 😢

Thanks for the heads up @durandom. I experience some weird stuff (that was solved passing parent_manager to build). We should be good with the first issue and the second one (we use on_create, and Monitoring manager always has a parent_manager).

I should probably change ensure_manager to ensure_X_manager.

I would appreciate it if you can take a look on the manageiq-providers-kubernetes PR adding the concrete manager.

moolitayer · 2017-07-06T13:15:50Z

I should probably change ensure_manager to ensure_X_manager.

Done

miq-bot · 2017-07-06T13:17:53Z

Checked commits moolitayer/manageiq@62f873e~...302b25d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 0 offenses detected
Everything looks fine. ⭐

miq-bot added the wip label Jun 12, 2017

moolitayer force-pushed the add_monitoring_manager branch from 1d8b26d to d6f6a81 Compare June 13, 2017 15:33

moolitayer changed the title ~~[WIP] Add monitoring manager~~ Add monitoring manager Jun 13, 2017

miq-bot removed the wip label Jun 13, 2017

moolitayer mentioned this pull request Jun 13, 2017

Multiple managers for Kubernetes ManageIQ/manageiq-providers-kubernetes#39

Closed

9 tasks

moolitayer changed the title ~~Add monitoring manager~~ [WIP] Add monitoring manager Jun 13, 2017

miq-bot added the wip label Jun 13, 2017

moolitayer force-pushed the add_monitoring_manager branch from d6f6a81 to c9ed7a3 Compare June 13, 2017 17:19

moolitayer changed the title ~~[WIP] Add monitoring manager~~ Add monitoring manager Jun 13, 2017

miq-bot removed the wip label Jun 13, 2017

moolitayer commented Jun 13, 2017

View reviewed changes

Ladas reviewed Jun 14, 2017

View reviewed changes

miq-bot assigned blomquisg Jun 14, 2017

miq-bot added the unmergeable label Jun 19, 2017

moolitayer force-pushed the add_monitoring_manager branch from c9ed7a3 to 14fc6b1 Compare June 27, 2017 10:03

miq-bot removed the unmergeable label Jun 27, 2017

moolitayer force-pushed the add_monitoring_manager branch 6 times, most recently from 03fd6d6 to e600b4d Compare June 28, 2017 13:08

moolitayer force-pushed the add_monitoring_manager branch from e600b4d to 76d1251 Compare June 28, 2017 13:14

cben reviewed Jun 28, 2017

View reviewed changes

simon3z reviewed Jul 3, 2017

View reviewed changes

This was referenced Jul 3, 2017

Add kubernetes monitoring manager ManageIQ/manageiq-providers-kubernetes#38

Merged

Add openshift monitoring manager ManageIQ/manageiq-providers-openshift#21

Merged

moolitayer force-pushed the add_monitoring_manager branch 2 times, most recently from 776aed7 to eb5e1c3 Compare July 4, 2017 11:30

moolitayer closed this Jul 4, 2017

moolitayer reopened this Jul 4, 2017

moolitayer mentioned this pull request Jul 4, 2017

Adapt manageiq to new managers #15506

Merged

moolitayer closed this Jul 5, 2017

moolitayer reopened this Jul 5, 2017

simon3z approved these changes Jul 5, 2017

View reviewed changes

blomquisg reviewed Jul 5, 2017

View reviewed changes

moolitayer force-pushed the add_monitoring_manager branch 3 times, most recently from 767a270 to e7eac86 Compare July 6, 2017 07:13

durandom reviewed Jul 6, 2017

View reviewed changes

Mooli Tayer added 2 commits July 6, 2017 16:14

Add Monitoring manager base

62f873e

Container managers have a Monitoring manager

302b25d

moolitayer force-pushed the add_monitoring_manager branch from e7eac86 to 302b25d Compare July 6, 2017 13:14

blomquisg merged commit fa18025 into ManageIQ:master Jul 6, 2017

blomquisg added this to the Sprint 64 Ending Jul 10, 2017 milestone Jul 6, 2017

blomquisg added providers enhancement labels Jul 10, 2017

moolitayer mentioned this pull request Aug 8, 2017

[WIP] add prometheus from moolitayer/manageiq-providers-prometheus #15139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add monitoring manager #15354

Add monitoring manager #15354

moolitayer commented Jun 12, 2017 •

edited

Loading

moolitayer commented Jun 13, 2017

moolitayer Jun 13, 2017

Ladas Jun 14, 2017 •

edited

Loading

Ladas commented Jun 14, 2017

moolitayer commented Jun 14, 2017

miq-bot commented Jun 19, 2017

moolitayer commented Jun 27, 2017

moolitayer commented Jun 28, 2017

cben Jun 28, 2017

moolitayer Jun 29, 2017 •

edited

Loading

moolitayer Jul 2, 2017

simon3z Jul 3, 2017

moolitayer Jul 3, 2017 •

edited

Loading

simon3z Jul 3, 2017 •

edited

Loading

moolitayer Jul 4, 2017

moolitayer Jul 4, 2017 •

edited

Loading

moolitayer commented Jul 4, 2017

moolitayer commented Jul 5, 2017

simon3z commented Jul 5, 2017

moolitayer commented Jul 5, 2017

blomquisg Jul 5, 2017

moolitayer Jul 5, 2017

moolitayer Jul 5, 2017

durandom Jul 6, 2017

moolitayer Jul 6, 2017

moolitayer commented Jul 6, 2017

miq-bot commented Jul 6, 2017

	def self.leaf_subclasses
	descendants.select { \|d\| d.subclasses.empty? }
	end

Add monitoring manager #15354

Add monitoring manager #15354

Conversation

moolitayer commented Jun 12, 2017 • edited Loading

moolitayer commented Jun 13, 2017

Choose a reason for hiding this comment

Ladas Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Ladas commented Jun 14, 2017

moolitayer commented Jun 14, 2017

miq-bot commented Jun 19, 2017

moolitayer commented Jun 27, 2017

moolitayer commented Jun 28, 2017

Choose a reason for hiding this comment

moolitayer Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

simon3z Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer commented Jul 4, 2017

moolitayer commented Jul 5, 2017

simon3z commented Jul 5, 2017

moolitayer commented Jul 5, 2017

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

moolitayer commented Jul 6, 2017

miq-bot commented Jul 6, 2017

moolitayer commented Jun 12, 2017 •

edited

Loading

Ladas Jun 14, 2017 •

edited

Loading

moolitayer Jun 29, 2017 •

edited

Loading

moolitayer Jul 3, 2017 •

edited

Loading

simon3z Jul 3, 2017 •

edited

Loading

moolitayer Jul 4, 2017 •

edited

Loading