Enable 'podman run --memory-swappiness=0' #12272
Conversation
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hshiina, rhatdan 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 |
openshift-ci
bot
added
the
approved
|
@hshiina is there a reported issue against Podman for this? I just want to make sure it gets closed after this merges. |
hshiina
force-pushed
the
memory-swappiness
branch
from
419e9ee to
19689bf
Compare
|
Thank you for pointing out. |
hshiina
force-pushed
the
memory-swappiness
branch
from
19689bf to
8273580
Compare
openshift-ci
bot
added
the
needs-rebase
'--memory-swappiness=0' used to work. This patch fixes the regression issue, which was caused by the change of infra container creation process. Signed-off-by: Hironori Shiina <shiina.hironori@jp.fujitsu.com>
hshiina
force-pushed
the
memory-swappiness
branch
from
8273580 to
9226ccb
Compare
openshift-ci
bot
removed
the
needs-rebase
|
LGTM, but I'd like a final look from @cdoern |
| Expect(session).Should(Exit(0)) | ||
| Expect(session.OutputToString()).To(Equal("15")) | ||
| }) | ||
| for _, limit := range []string{"0", "15", "100"} { |
There was a problem hiding this comment.
is there no way to reverse this syntax? I feel like its better to declare the test once and run the same code with different values inside of the single test
There was a problem hiding this comment.
I think separated tests makes it easy to find which case fails.
| @@ -266,6 +266,15 @@ type ContainerCreateOptions struct { | |||
| CgroupConf []string | |||
| } | |||
|
|
|||
| func NewInfraContainerCreateOptions() ContainerCreateOptions { | |||
There was a problem hiding this comment.
I like the idea of this, but would it make more sense to make this a struct rather than a function? A struct would more closely resemble our other create options while a function more closely represents our specgenerator process. either way this is a good way to differentiate infra.
There was a problem hiding this comment.
A new struct for an infra container would be helpful to clarify what parameter is required for creating an infra container. But, I'm not sure how much change is necessary to introduce the new struct. I chose a function to fix this issue with the smaller change.
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Signed-off-by: Hironori Shiina shiina.hironori@jp.fujitsu.com
What this PR does / why we need it:
--memory-swappiness=0used to work. This patch fixes the regression issue.How to verify it
Run the added integration test with cgroup v1.
Which issue(s) this PR fixes:
None
Special notes for your reviewer: