[WIP] Cap & U - ems centric cleanup #16099

kbrock · 2017-10-03T14:54:06Z

Relevant PRs:

Metrics Worker capture_timer more ems centric #16004 ems centric capture_targets
Cut up perf_capture into multiple methods #16262 split up perf_capture
[C&U] fix bug in Targets#capture_vm_targets #16373 capture bug
VimConnectMixin#connect - remove unused params #16379 VimConnectMixin#connect cleanup
~~Separate has_required_role from should_start_worker #16505 reduction of connect~~

Overview

Currently perf_capture_timer, perf_capture, and perf_process are not working so well together. They create a large number of objects and a large number of queue entries. They also rely upon MiqQueue#put_or_update which is problematic going to a real queue implementation.

The goal is to reorganize the code to more closely map to how providers best communicate metrics and our bottlenecks.

Before

Determine the objects to be collected in a single zone.
This code is generic.
Message is sent to requesting each object be collected
Each object is captured, processed, and persisted.

Good: Collection can be done in parallel. (The bottleneck is persisting, not collecting)
Bad: This floods the queue and causes a number of issues for our users. This causes multiple requests for collecting the same objects.

determine objects (perf_capture_timer)
    -queue->
        collect + persist (perf_capture, perf_process)

After

Determine the objects to be collected for an ems.
This code is moved to metric_capture.rb and can be extended.
Capture metrics in the main process. Code moved to metric_capture.rb to allow for extensibility.
Send request to process a group of objects.
Group of objects are processed and persisted perf_process.
Callback on those message allows Hosts to be rolled up into an EmsCluster metrics record.

Good: able to extend the collection. Already seen big benefits for VMware.
Bad: collection is not able to run in parallel (as said before, it is not a bottleneck)

determine objects + collect(perf_capture_timer capture_metrics)
    -queue->
        collect + persist (perf_process)

Notes

storage.rb overrode all of perf_capture work. Since it does not communicate with the provider/ems I left it intact and just offloaded to the processor.

agrare · 2017-10-03T15:04:06Z

app/models/metric/capture.rb

@@ -1,4 +1,5 @@
-module Metric::Capture
+class Metric
+class Capture


This so you didn't have to indent the whole rest of the file?

yea, I could do class Metric::Capture

agrare · 2017-10-03T15:05:30Z

app/models/metric/capture.rb

@@ -38,6 +39,12 @@ def self.alert_capture_threshold(target)
                ::Settings.performance.capture_threshold_with_alerts.default)
  end

+  attr_accessor :ems, :interval_name
+  def initialize(ems = nil, interval_name = "realtime")


Is a nil ems a valid default here? Looks like it is required further down (at least for ems.zone and maybe Metric::Targets.all_capture_targets(ems)...not sure what that does with a nil arg yet)

this object is not valid without an ems
But I feel we should always be able to create objects without args for things like specs

kbrock · 2017-10-03T21:34:11Z

@agrare wants this in MetricsCapture

agrare · 2017-10-05T18:44:35Z

app/models/metric/capture.rb

-    _log.info("Queueing performance capture...Complete")
+  def self.perf_collect_all_metrics(ems, interval_name = "realtime", start_time = nil, end_time = nil, options = nil)
+    ems = ExtManagementSystem.find(ems) unless ems.kind_of?(ExtManagementSystem)
+    klass = ems.class::MetricsCapture rescue ManageIQ::Providers::BaseManager::MetricsCapture


I don't think we need to worry about this, if an EMS doesn't have a MetricsCapture we will just blowup later in just_perf_capture when calling perf_collect_metrics

was thinking this gave us compatibility for all providers while we rolled out new code

Since this is going into older branch, thought we'd go for minimal change to each of those repos

agrare · 2017-10-09T15:03:26Z

app/models/manageiq/providers/base_manager/metrics_capture.rb

+      start_time ||= ems.last_metrics_success_date
+      # For hourly on the first capture, we don't want to get all of the
+      #   historical data, so we shorten the query
+      start_time ||= 4.hours.ago.utc if interval_name == 'hourly'


We should probably move 4.hours.ago.utc as the "realtime cutoff" to a constant since we use it here too

agrare · 2017-10-09T15:06:57Z

So this isn't using each target's last_perf_capture_on to drive the start_time instead using the ems's last_metrics_success_date right?

agrare · 2017-10-09T15:07:14Z

@Fryguy can you take a look?

miq-bot · 2017-10-16T21:56:49Z

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

jameswnl · 2017-11-03T22:09:37Z

app/models/metric/capture.rb

+  # called by ui
+  # run a perf capture zone for a zone or ems
+  def self.perf_capture_gap_queue(start_time, end_time, zone = nil)
+    emses = if zone.kind_of?(ExtManagementSystem)


Can we refactor this method's signature? The way this param zone can be an ems is against intuition.

jameswnl · 2017-11-03T22:10:22Z

app/models/metric/capture.rb

+    emses = if zone.kind_of?(ExtManagementSystem)
+              zone
+            else
+              zone = Zone.find(zone) if zone && !zone.kind_of?(Zone)


Does this Zone.find take in any thing and return the zone that the thing is in?

it takes integers or strings - typically from the queue

jameswnl · 2017-11-07T18:49:21Z

spec/models/metric_spec.rb

@@ -62,111 +53,12 @@
          }
        end

-        it "should queue up enabled targets" do
-          expect(MiqQueue.group(:class_name, :method_name).count).to eq(expected_queue_items)


This let block defining this now deleted expected_queue_items should be removed as well

jameswnl · 2017-11-07T18:55:39Z

spec/models/metric_spec.rb

@@ -34,15 +34,6 @@
      end

      context "executing capture_targets" do
-        it "should find enabled targets" do
-          targets = Metric::Targets.capture_targets
-          assert_infra_targets_enabled targets, %w(ManageIQ::Providers::Vmware::InfraManager::Vm ManageIQ::Providers::Vmware::InfraManager::Host ManageIQ::Providers::Vmware::InfraManager::Host ManageIQ::Providers::Vmware::InfraManager::Vm ManageIQ::Providers::Vmware::InfraManager::Host Storage)


This assert_infra_targets_enabled seems obsoleted as well.

Thanks - I'll delete Metric::Targets.capture_targets as well

jameswnl · 2017-11-07T19:12:00Z

spec/models/metric/capture_spec.rb

+        :class_name  => "Metric::Capture",
+        :method_name => "perf_collect_all_metrics",
+        :args        => [ems.id, "realtime", nil, nil, :exclude_storage => true],
+        :zone        => ems.zone_name


👍 so this should be able to catch the zone_name vs zone.id issue

provided that the test was written correctly in the first place :( - thanks for the catch

jameswnl · 2017-11-07T19:14:07Z

app/models/metric/capture.rb

-  end
-
+  # legacy messages on the queue
+  # went away in 4.6


this #went away in 4.6 still true?
we need this for 4.6 coz 4.5 could have left behind queued item for this deprecated method.

Well, the callers have all gone away.
I put this note in there to let people know that it is just around for legacy messages.

Are you thinking the scheduler should be paired down and still generate this message?
So all the intelligence will be in this process instead of the Scheduler::Runner?

Alternatively, we could delete the contents of this method since there will only ever be one message in the queue and the contents will just get picked up a few minutes later when the scheduler submits it again.

jameswnl · 2017-11-07T19:24:13Z

app/models/metric/capture.rb

+    perf_collect_all_metrics_queue(zone.ext_management_systems, "realtime")
+  end
+
+  def self.perf_collect_all_metrics_queue(ems, interval_name = "realtime", start_time = nil, end_time = nil, options = {})


question: why not make the param ems as emses and we have no need for the Array.wrap?
(Is this ruby style to make the caller happier?)

I had not realized that everything coming in is an array.

My initial intent was for us to queue a single metrics collect.
Sad to see the Array.wrap go, but think you are right - making edit

jameswnl · 2017-11-07T19:27:35Z

spec/models/metric/capture_spec.rb

+    it "triggers better capture" do
+      EvmSpecHelper.local_miq_server.zone
+      ems  # default zone
+      ems2 # other zone


and ems3 as well

miq-bot · 2017-11-14T19:21:54Z

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

miq-bot · 2018-02-16T21:02:32Z

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

TODO: move from capture to metrics_capture.rb THEN refactor

miq-bot · 2018-02-27T02:46:45Z

Some comments on commits kbrock/manageiq@45d685c~...fc76af2

spec/models/miq_vim_broker_worker_spec.rb

⚠️ - 41 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 7 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2018-02-27T02:47:51Z

Checked commits kbrock/manageiq@45d685c~...fc76af2 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
14 files checked, 4 offenses detected

app/models/manageiq/providers/base_manager/metrics_capture.rb

❗ - Line 127, Col 14 - Style/ParenthesesAroundCondition - Don't use parentheses around the condition of an if.
❗ - Line 143, Col 7 - Style/RescueStandardError - Avoid rescuing without specifying an error class.
❗ - Line 162, Col 5 - Style/MultipleComparison - Avoid comparing a variable with multiple items in a conditional, use Array#include? instead.

app/models/vim_performance_state.rb

❗ - Line 249, Col 7 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

miq-bot · 2018-03-20T15:01:33Z

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

miq-bot · 2018-09-24T04:00:20Z

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

kbrock added enhancement providers/metrics labels Oct 3, 2017

kbrock assigned agrare Oct 3, 2017

kbrock added the wip label Oct 3, 2017

agrare reviewed Oct 3, 2017

View reviewed changes

kbrock force-pushed the fetch_collector_collect branch 4 times, most recently from 981452f to 62431a8 Compare October 5, 2017 18:31

agrare reviewed Oct 5, 2017

View reviewed changes

kbrock force-pushed the fetch_collector_collect branch 6 times, most recently from 52574b7 to 2f9e4af Compare October 6, 2017 21:14

agrare reviewed Oct 9, 2017

View reviewed changes

agrare requested a review from Fryguy October 9, 2017 15:05

miq-bot added the unmergeable label Oct 16, 2017

kbrock mentioned this pull request Oct 23, 2017

Cut up perf_capture into multiple methods #16262

Merged

kbrock force-pushed the fetch_collector_collect branch from 2f9e4af to 13a83a4 Compare October 23, 2017 13:53

miq-bot removed the unmergeable label Oct 23, 2017

kbrock force-pushed the fetch_collector_collect branch from 13a83a4 to 54c8809 Compare October 25, 2017 13:30

kbrock changed the title ~~[WIP] Cap & U - ems centric~~ [WIP] Cap & U - ems centric cleanup Oct 27, 2017

kbrock force-pushed the fetch_collector_collect branch from 54c8809 to 9babbae Compare October 27, 2017 15:37

jameswnl reviewed Nov 3, 2017

View reviewed changes

kbrock force-pushed the fetch_collector_collect branch from 3eb8d47 to c128c36 Compare November 6, 2017 15:45

jameswnl reviewed Nov 7, 2017

View reviewed changes

kbrock force-pushed the fetch_collector_collect branch from c128c36 to 26c10fe Compare November 8, 2017 23:48

kbrock changed the title ~~Cap & U - ems centric cleanup~~ [WIP] Cap & U - ems centric cleanup Nov 14, 2017

kbrock added the wip label Nov 14, 2017

kbrock force-pushed the fetch_collector_collect branch 2 times, most recently from 872c419 to c58df51 Compare November 14, 2017 15:07

miq-bot added the unmergeable label Nov 14, 2017

kbrock force-pushed the fetch_collector_collect branch from c58df51 to a65edb1 Compare November 22, 2017 16:58

miq-bot removed the unmergeable label Nov 22, 2017

kbrock force-pushed the fetch_collector_collect branch 2 times, most recently from e09afa4 to 1cde354 Compare January 25, 2018 22:29

miq-bot added the unmergeable label Feb 16, 2018

kbrock and others added 5 commits February 26, 2018 21:34

minor speed increases for remove_snapshot

45d685c

MiqVimBrokerWorker#has_required_role_configured

ac0ee57

vim_performance_state resources

9fa5428

remove MiqAlert#alert_capture_threshold

f720463

WIP Add ems centric capture to metrics_capture

fc76af2

TODO: move from capture to metrics_capture.rb THEN refactor

kbrock force-pushed the fetch_collector_collect branch from b1d6402 to fc76af2 Compare February 27, 2018 02:39

miq-bot removed the unmergeable label Feb 27, 2018

miq-bot added the unmergeable label Mar 20, 2018

miq-bot closed this Sep 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Cap & U - ems centric cleanup #16099

[WIP] Cap & U - ems centric cleanup #16099

kbrock commented Oct 3, 2017 •

edited

Loading

agrare Oct 3, 2017

kbrock Oct 3, 2017

kbrock Oct 27, 2017

agrare Oct 3, 2017

kbrock Oct 3, 2017

kbrock Oct 27, 2017

kbrock commented Oct 3, 2017

agrare Oct 5, 2017

kbrock Oct 5, 2017

kbrock Oct 27, 2017

agrare Oct 9, 2017

kbrock Oct 27, 2017

agrare commented Oct 9, 2017

agrare commented Oct 9, 2017

miq-bot commented Oct 16, 2017

jameswnl Nov 3, 2017

jameswnl Nov 3, 2017

kbrock Nov 6, 2017

jameswnl Nov 7, 2017 •

edited

Loading

jameswnl Nov 7, 2017

kbrock Nov 8, 2017

jameswnl Nov 7, 2017

kbrock Nov 8, 2017

jameswnl Nov 7, 2017

kbrock Nov 8, 2017 •

edited

Loading

jameswnl Nov 7, 2017

kbrock Nov 8, 2017 •

edited

Loading

jameswnl Nov 7, 2017

miq-bot commented Nov 14, 2017

miq-bot commented Feb 16, 2018

miq-bot commented Feb 27, 2018

miq-bot commented Feb 27, 2018

miq-bot commented Mar 20, 2018

miq-bot commented Sep 24, 2018

[WIP] Cap & U - ems centric cleanup #16099

[WIP] Cap & U - ems centric cleanup #16099

Conversation

kbrock commented Oct 3, 2017 • edited Loading

Overview

Before

After

Notes

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

Choose a reason for hiding this comment

kbrock commented Oct 3, 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

agrare commented Oct 9, 2017

agrare commented Oct 9, 2017

miq-bot commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameswnl Nov 7, 2017 • edited Loading

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

Choose a reason for hiding this comment

kbrock Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Nov 14, 2017

miq-bot commented Feb 16, 2018

miq-bot commented Feb 27, 2018

miq-bot commented Feb 27, 2018

miq-bot commented Mar 20, 2018

miq-bot commented Sep 24, 2018

kbrock commented Oct 3, 2017 •

edited

Loading

jameswnl Nov 7, 2017 •

edited

Loading

kbrock Nov 8, 2017 •

edited

Loading

kbrock Nov 8, 2017 •

edited

Loading