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

Use Prometheus timeouts from settings #167

Merged
merged 1 commit into from
Nov 12, 2017

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Nov 9, 2017

Description

Metrics collection from OCP 3.7 using Prometheus may hit timeout while collecting metrics.

a. the default timeouts are too small
b. users can not set new timeout values to fit their system

This PR uses the settings needed to adjust the timeouts.

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

@yaacov
Copy link
Member Author

yaacov commented Nov 9, 2017

@miq-bot add_label bug

@simon3z @moolitayer @ilackarms @joelddiaz please review

@miq-bot miq-bot added the bug label Nov 9, 2017
@miq-bot
Copy link
Member

miq-bot commented Nov 9, 2017

Checked commit yaacov@d8f14a4 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@cben
Copy link
Contributor

cben commented Nov 9, 2017

These are new settings right?
Please add them to config/settings.yml, instead of defaults in code.
It's hard to track what is "settable" if one has to infer it from the code...

@yaacov
Copy link
Member Author

yaacov commented Nov 9, 2017

Please add them to config/settings.yml, instead of defaults in code.

ManageIQ/manageiq#16429 ?

@yaacov
Copy link
Member Author

yaacov commented Nov 9, 2017

@cben 👍

closed ManageIQ/manageiq#16429 , will move the settings to local setting.yml file

@moolitayer moolitayer requested a review from cben November 9, 2017 14:15
@moolitayer moolitayer self-assigned this Nov 9, 2017
@joelddiaz
Copy link

lgtm

@yaacov yaacov force-pushed the use-prometheus-timeouts branch from d8f14a4 to da6da55 Compare November 12, 2017 10:00
Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@moolitayer moolitayer merged commit 17825b2 into ManageIQ:master Nov 12, 2017
@moolitayer moolitayer added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 12, 2017
@simaishi
Copy link

@yaacov gaprindashvili/yes ?

@yaacov
Copy link
Member Author

yaacov commented Nov 13, 2017

@yaacov gaprindashvili/yes ?

yes

@ilackarms
Copy link

@yaacov wondering how this will be configurable from openshift in the manageiq-pod

@joelddiaz
Copy link

IMO, ideally we want some way for the automation in openshift-ansible to be able to accept configuring these settings

@yaacov
Copy link
Member Author

yaacov commented Nov 13, 2017

IMO, ideally we want some way for the automation in openshift-ansible to be able to accept configuring these settings

We will have to ask ( the api guys ? ) if we have an api for setting the settings ? @moolitayer ?

@cben suggested adding that to the new tags/labels @enoodle added for the provider, do we have api for those ?

@cben
Copy link
Contributor

cben commented Nov 13, 2017 via email

@yaacov
Copy link
Member Author

yaacov commented Nov 13, 2017

There should be API (and then) ansible for settings.

@cben Do you know what is the api path for that ?

p.s.
I suggested the tags in case their is no api for settings, and their is one for tags ...

@cben
Copy link
Contributor

cben commented Nov 13, 2017 via email

@moolitayer
Copy link

moolitayer commented Nov 14, 2017

We will have to ask ( the api guys ? ) if we have an api for setting the settings ? @moolitayer ?

  • We have an API for selected settings, we must actively include what needs to be exposed[1]
  • I agree with @cben about the orthogonality of per provider setting - we should add those if they make sense and not because we can API them. I think they do make sense (e.g one provider is behind a slower network) I seem to recall a api PR for that, @enoodle can you confirm we have an API for per provider settings (options right?)?

[1] ManageIQ/manageiq-api@cb85286

@moolitayer
Copy link

We have an API for selected settings, we must actively include what needs to be exposed[1]

I see @cben already commented about that

@enoodle
Copy link

enoodle commented Nov 14, 2017

@cben @yaacov can you take a look at [1] and if it is what you are looking for then the PRs [2][3] it links to see how to configure per provider settings.

[1] ManageIQ/manageiq-ui-classic#1652
[2] #45
[3] ManageIQ/manageiq-providers-openshift#32

@moolitayer
Copy link

@enoodle @joelddiaz is looking for an API

@enoodle
Copy link

enoodle commented Nov 14, 2017

@moolitayer Currently that API for per-provider options that I created is not returning the values, but only the available options per provider. I think it should be changed to also return the values, or another one should be created for that.

@moolitayer
Copy link

@enoodle what about setting them?

@enoodle
Copy link

enoodle commented Nov 14, 2017

@moolitayer Yes, I will make a PR to add getting/setting the options per provider in another endpoint (something like /api/provider//settings )

@agrare agrare changed the title Use Proetheus timeouts form settings Use Proetheus timeouts from settings Nov 14, 2017
simaishi pushed a commit that referenced this pull request Nov 14, 2017
@simaishi
Copy link

Gaprindashvili backport details:

$ git log -1
commit 62f6d243910e2cba0dda09628af25e786cb3fb5f
Author: Mooli Tayer <[email protected]>
Date:   Sun Nov 12 15:21:15 2017 +0200

    Merge pull request #167 from yaacov/use-prometheus-timeouts
    
    Use Proetheus timeouts form settings
    (cherry picked from commit 17825b2fdefaffd8b81a1fddf0818c33fb5a4435)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1513116

@enoodle
Copy link

enoodle commented Nov 15, 2017

I created ManageIQ/manageiq-api#211 to allow setting the options field per provider. For example:

curl --user admin:smartvm -X POST -d '{"action":"edit","options":{"image_inspector_options":{"image_tag":"2.1"}}}' http://localhost:3000/api/providers/1

@moolitayer moolitayer changed the title Use Proetheus timeouts from settings Use Prometheus timeouts from settings Nov 30, 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.

8 participants