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.

Sign up for GitHub

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

Jump to bottom

bug 1506073. Lower cpu request for logging when it exceeds limit #5897

Merged
merged 1 commit into from
Oct 27, 2017
Merged

bug 1506073. Lower cpu request for logging when it exceeds limit #5897

merged 1 commit into from
Oct 27, 2017

Conversation

jcantrill
Copy link
Contributor

This PR fixes https://bugzilla.redhat.com/show_bug.cgi?id=1506073 by:

  • Lowering the CPU request to match the limit when the request is greater then a specified limit

I have an open question on if this is an acceptable change of if it makes the outcome unexpected. Should we prefer to exit during the deployment and advise the operator to correct their inventory?

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 26, 2017
portante
portante approved these changes Oct 26, 2017
View reviewed changes
Copy link
Contributor

@portante portante left a comment

Choose a reason for hiding this comment

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

/lgtm, but wondering why we need to use ansible facts?

@jcantrill
Copy link
Contributor Author

@portante We are not using facts in this context. We are using set_fact to simply set a variable to be evaluated later

@jcantrill jcantrill force-pushed the 1506073_cpu_request_match_limit branch from d62c359 to 2e82450 Compare October 26, 2017 14:57
@portante
Copy link
Contributor

Why do you have to use set_fact? Can you just put that expression used in the set_fact where the fact is referenced?

ewolinetz
ewolinetz suggested changes Oct 26, 2017
View reviewed changes
Copy link
Contributor

@ewolinetz ewolinetz left a comment

Choose a reason for hiding this comment

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

I agree with @portante there isn't a real reason to add set_fact for a lot of these, we can just use the filter plugin in line where we would pass in the var. It cuts down on calls a little...

@jcantrill jcantrill force-pushed the 1506073_cpu_request_match_limit branch from 2e82450 to 8934b85 Compare October 26, 2017 20:17
@jcantrill jcantrill force-pushed the 1506073_cpu_request_match_limit branch from 8934b85 to 205d034 Compare October 27, 2017 13:02
sdodson
sdodson approved these changes Oct 27, 2017
View reviewed changes
Copy link
Member

@sdodson sdodson left a comment

Choose a reason for hiding this comment

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

lgtm, will let eric tag the pr for merge if he's happy too

ewolinetz
ewolinetz approved these changes Oct 27, 2017
View reviewed changes
Copy link
Contributor

@ewolinetz ewolinetz left a comment

Choose a reason for hiding this comment

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

misread how we're using the filter in my comment...
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2017
@ewolinetz
Copy link
Contributor

/retest

@jcantrill
Copy link
Contributor Author

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 27, 2017
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 2c81304 into openshift:master Oct 27, 2017
@jcantrill jcantrill deleted the 1506073_cpu_request_match_limit branch October 30, 2017 16:28
@jcantrill jcantrill mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdodson sdodson sdodson approved these changes

@portante portante portante approved these changes

@ewolinetz ewolinetz ewolinetz approved these changes

Assignees

@ewolinetz ewolinetz

Labels
affects_3.6 affects_3.7 kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jcantrill @portante @ewolinetz @openshift-merge-robot @sdodson @openshift-ci-robot