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

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

Merged
merged 1 commit into from
Sep 4, 2023
Merged

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

merged 1 commit into from
Sep 4, 2023
+42 −1

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

@giuseppe giuseppe mentioned this pull request Sep 4, 2023
Closed
@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
vrothberg
vrothberg approved these changes Sep 4, 2023
View reviewed changes
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 giuseppe force-pushed the clamp-oom-score-adj branch from c4999fd to 91d5e01 Compare September 4, 2023 08:28
@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

vrothberg
vrothberg approved these changes Sep 4, 2023
View reviewed changes
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

@giuseppe giuseppe force-pushed the clamp-oom-score-adj branch from 91d5e01 to abad970 Compare September 4, 2023 09:46
flouthoc
flouthoc approved these changes Sep 4, 2023
View reviewed changes
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

@giuseppe
linux, rootless: clamp oom_score_adj if it is too low
8b4a79a 
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>
@giuseppe giuseppe force-pushed the clamp-oom-score-adj branch from abad970 to 8b4a79a Compare September 4, 2023 12:44
@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
@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 giuseppe mentioned this pull request Sep 11, 2023
Closed
@dustymabe dustymabe mentioned this pull request Sep 11, 2023
Closed
@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

This was referenced Sep 12, 2023
Closed
Closed
@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).

@damian01w damian01w mentioned this pull request Sep 14, 2023
Closed
@rhafer rhafer mentioned this pull request Sep 18, 2023
Closed
@Gyromancer Gyromancer mentioned this pull request Sep 20, 2023
Open
@Cellebyte Cellebyte mentioned this pull request Sep 29, 2023
Closed
@poettering poettering mentioned this pull request Oct 30, 2023
Closed
@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 Dec 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg approved these changes

@flouthoc flouthoc flouthoc approved these changes

Assignees

@rhatdan rhatdan

@flouthoc flouthoc

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.

Cannot start rootless containers via docker-compose
7 participants
@giuseppe @rhatdan @Luap99 @snazy @flouthoc @vrothberg @openshift-merge-robot