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

Fix CPU usage limitation in play kube for non integer values #15728

Merged
merged 1 commit into from
Sep 10, 2022
Merged

Fix CPU usage limitation in play kube for non integer values #15728

merged 1 commit into from
Sep 10, 2022

Conversation

tyler92
Copy link
Contributor

@tyler92 tyler92 commented Sep 9, 2022 •
edited
Loading

Signed-off-by: Mikhail Khachayants [email protected]

Does this PR introduce a user-facing change?

Fix CPU usage limitation in play kube for non integer values

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Sep 9, 2022
@tyler92 tyler92 force-pushed the fix-cpu-millis-limit branch from e235e98 to 6855060 Compare September 9, 2022 19:13
@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2022

Wh is this a WIP?
@umohnani8 @haircommander @saschagrunert @mrunalp PTAL

@tyler92
Copy link
Contributor Author

tyler92 commented Sep 9, 2022 •
edited
Loading

Just wanted to add a test next day

@tyler92
Copy link
Contributor Author

tyler92 commented Sep 10, 2022

I have no idea why test "podman play kube allows setting resource limits" works in CI and doesn't work locally. I mean without my change. This test should fail without my changes but it doesn't with or without my changes.

@tyler92 tyler92 mentioned this pull request Sep 10, 2022
Closed
@tyler92 tyler92 force-pushed the fix-cpu-millis-limit branch from 6855060 to 5dae966 Compare September 10, 2022 07:38
@tyler92 tyler92 changed the title WIP: Fix CPU usage limitation in play kube for non integer values Fix CPU usage limitation in play kube for non integer values Sep 10, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 10, 2022
@giuseppe
Copy link
Member

Code LGTM, thanks! Please add a test and a reference in the commit message to the previous commit that caused this bug

@tyler92
Fix CPU usage limitation in play kube for non integer values
b8108d0 
This logic has been broken by commit 9c6c981
(kube: fix conversion from milliCPU to period/quota).

[NO NEW TESTS NEEDED]
Fixes: containers#15726

Signed-off-by: Mikhail Khachayants <[email protected]>
@tyler92 tyler92 force-pushed the fix-cpu-millis-limit branch from 5dae966 to b8108d0 Compare September 10, 2022 07:56
@tyler92
Copy link
Contributor Author

tyler92 commented Sep 10, 2022 •
edited
Loading

a reference in the commit message to the previous commit that caused this bug

Done. Not sure about format, didn't find any related to this in the doc.

Please add a test

I wanted to do it. But I'm surprised that the necessary test already exists - "podman play kube allows setting resource limits". It fails without my changes and is green with my PR on my laptop. But in CI this test is always green. Do you have any idea why?

@rhatdan
Copy link
Member

rhatdan commented Sep 10, 2022

/approved
I would guess that the tests is not being run or is being run on a cgroup V1 system?

@rhatdan
Copy link
Member

rhatdan commented Sep 10, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2022
@giuseppe
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2022
@giuseppe
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, tyler92

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit b9cbc0c into containers:main Sep 10, 2022
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@giuseppe giuseppe giuseppe approved these changes

Assignees

@giuseppe giuseppe

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tyler92 @rhatdan @giuseppe @openshift-merge-robot