Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cu schedule collector by ems #19420

Merged
merged 4 commits into from
Oct 29, 2019
Merged

Cu schedule collector by ems #19420

merged 4 commits into from
Oct 29, 2019

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Oct 21, 2019

We suggest that customers should configure one ems per zone.
So running collection by zone is typically just running collection by ems.

before

scheduler sends no parameter for collector.
collector defaults to the current zone
collector gets all vms/hosts for the zone and sends off a message to collect each.

after

scheduler sends an ems for collector
collector gets all vms/hosts for the ems and sends off a message to collect each.

old messages for collectors still default to the current zone.

/thanks @d-m-u

@kbrock kbrock force-pushed the cu_schedule_ems branch 2 times, most recently from 31908f1 to 3f58c9a Compare October 21, 2019 20:33
@kbrock kbrock changed the title [WIP] Cu schedule ems [WIP] Cu schedule by ems Oct 21, 2019
@kbrock kbrock marked this pull request as ready for review October 23, 2019 13:41
@kbrock kbrock requested a review from agrare October 23, 2019 13:42
@kbrock kbrock changed the title [WIP] Cu schedule by ems Cu schedule collector by ems Oct 23, 2019
@kbrock kbrock removed the wip label Oct 23, 2019
@kbrock kbrock force-pushed the cu_schedule_ems branch 2 times, most recently from 1d6a3f2 to f0ea15e Compare October 24, 2019 20:11
@agrare agrare self-assigned this Oct 28, 2019
Copy link
Member Author

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the one main thing

app/models/metric/capture.rb Outdated Show resolved Hide resolved
spec/models/metric_spec.rb Outdated Show resolved Hide resolved
schedules_for_ems_metrics_coordinator_role checks for
role enabled with ems_metrics_coordinator before creating repeating
timer metric_capture_perf_capture_timer

No need to also check for the the role in the timer
app/models/zone.rb Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Oct 28, 2019

One issue from me on the terminology, but otherwise looks good. I'll leave it to @agrare to merge

@@ -68,11 +68,11 @@ def storage_scan_timer
end

def metric_capture_perf_capture_timer
zone = MiqServer.my_server(true).zone
if zone.role_active?("ems_metrics_coordinator")
MiqServer.my_server.zone.ems_collectable.each do |ems|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed updating this one to ems_metrics_collectable

Up until now, perf_capture_timer is never passed a parameter
So changing the parameter from zone to ems_id will not break anything.
@miq-bot
Copy link
Member

miq-bot commented Oct 29, 2019

Checked commits kbrock/manageiq@16aec34~...6af789d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🏆

@agrare agrare merged commit 909e8d9 into ManageIQ:master Oct 29, 2019
@agrare agrare added this to the Sprint 124 Ending Nov 11, 2019 milestone Oct 29, 2019
@kbrock kbrock deleted the cu_schedule_ems branch October 30, 2019 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants