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

linux, rootless: clamp oom_score_adj if it is too low #19843

Merged

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Sep 4, 2023

when running rootless, if the specified oom_score_adj for the container process is lower than the current value, clamp it to the current value and print a warning.

Closes: #19829

Does this PR introduce a user-facing change?

When running rootless, if the oom-score-adj value is too low, then it is clamped to the current value

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

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Code LGTM

Can you add a note to the docs as well?

@giuseppe
Copy link
Member Author

giuseppe commented Sep 4, 2023

Can you add a note to the docs as well?

sure, added a note to the docs

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
@containers/podman-maintainers PTAL

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM
/hold
/lgtm

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, vrothberg

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:
  • OWNERS [flouthoc,giuseppe,vrothberg]

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

when running rootless, if the specified oom_score_adj for the
container process is lower than the current value, clamp it to the
current value and print a warning.

Closes: containers#19829

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2023
@giuseppe
Copy link
Member Author

giuseppe commented Sep 4, 2023

needs again the lgtm label

@rhatdan
Copy link
Member

rhatdan commented Sep 4, 2023

/lgtm
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2023
@openshift-merge-robot openshift-merge-robot merged commit 8914caf into containers:main Sep 4, 2023
93 checks passed
@Luap99
Copy link
Member

Luap99 commented Sep 11, 2023

Late comment but doesn't this run into the same problem similar to #18721?

You know set the limit on container creation permanently. Which means a long living container cannot be started if the oom_score_adj limit for the current user is set higher.
Instead we should check this value on startup time based on the real current value and not the calue at creation time.

@giuseppe
Copy link
Member Author

Late comment but doesn't this run into the same problem similar to #18721?

You know set the limit on container creation permanently. Which means a long living container cannot be started if the oom_score_adj limit for the current user is set higher. Instead we should check this value on startup time based on the real current value and not the calue at creation time.

That is a good idea, taking a look right now

@giuseppe
Copy link
Member Author

@Luap99 PR here: #19927

@snazy
Copy link

snazy commented Sep 14, 2023

Maybe not the right place to ask, but could you push a new 4.6.3 patch release with this fix?
I'm kind of locked into SuSE's unstable repo and the workaround to downgrade crun doesn't work there (switching to the "stable" one doesn't work for other reasons).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

Cannot start rootless containers via docker-compose
7 participants