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

Avoid metrics collection workers unless endpoint #7

Merged
merged 1 commit into from May 8, 2017

Conversation

moolitayer
Copy link

@moolitayer moolitayer commented Apr 27, 2017

Starting with k8s since container_manager_mixin is shared with openshift.
We will then need to do one of:

  • copy emses_in_zone to openshift's metrics_collector_worker.rb
    OR
  • create a common component for both metrics collectors

supports_metrics? aims to be replaced by a support feature.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1437138#c0

@moolitayer
Copy link
Author

cc @durandom @yaacov @cben @simon3z

Copy link
Member

@yaacov yaacov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Note:
The check connection_configurations.hawkular != nil will not work on systems where we have a hawkular with nil hostname, which is the way we check for supports_metrics? now.

# Override PerEmsTypeWorkerMixin.emses_in_zone to limit metrics collection
def self.emses_in_zone
super.select do |ems|
(ems.supporty_metrics?).tap do |hawkular|
Copy link
Member

Choose a reason for hiding this comment

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

  • I would not call the arg hawkular, because supports_metrics? just returns a boolean. How about available or supported
  • the ()are not needed
  • Typo: supporty vs supports

@durandom
Copy link
Member

I'm ok with adding the method before adding a feature. @moolitayer just be sure to push adding the feature once you have a full featured and working implementation

@miq-bot
Copy link
Member

miq-bot commented May 1, 2017

Checked commit moolitayer@3d26da2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@cben
Copy link
Contributor

cben commented May 1, 2017

Nice.
@yaacov is right. Backward compatibility is hard, depends on when provider was Added (or last Edited):

  • ≤capablanca was creating single-endpoint providers. their metrics did work on ≥darga if they had it on master port 5000 (or by changing metrics_port setting). though on capablanca itself I don't see this code (no hawkular_client_mixin.rb, no metrics_port).
    Are you OK with such providers no longer getting metrics (requiring manual Edit to fix)?

  • ≥darga always created hawkular endpoints, sometimes with empty hostname.
    I suppose such providers will just continue to try (and fail) getting metrics, we can later do a migration deleting empty-hostname endpoints...

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

👍

@simon3z
Copy link
Contributor

simon3z commented May 8, 2017

@moolitayer @cben any label? (bug? enhancement? backport? etc.)

@simon3z simon3z merged commit e7d6ee1 into ManageIQ:master May 8, 2017
@moolitayer
Copy link
Author

@simon3z should reach 5.8.1
@miq-bot add_label enhancement, fine/yes

@simaishi
Copy link

simaishi commented Jun 13, 2017

@moolitayer @simon3z BZ 1437138 is now for 'master' only. In the BZ, it says it's safe to backport this alone, but not sure what issue this will fix. Should this still be backported?

@moolitayer
Copy link
Author

I was just on my way here... Better not to backport

@miq-bot add_label fine/no

@moolitayer moolitayer modified the milestones: Sprint 61 Ending May 22, 2017, Sprint 60 Ending May 8, 2017 Aug 9, 2017
@simon3z simon3z added the metrics label Sep 5, 2017
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.

None yet

7 participants