Skip to content

Add support for --pids-limit in podman kube play. #25645

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
Merged
merged 1 commit into from
Mar 26, 2025
Merged

Add support for --pids-limit in podman kube play. #25645

merged 1 commit into from
Mar 26, 2025

Conversation

jankaluza
Copy link
Member

@jankaluza jankaluza commented Mar 21, 2025 •
edited
Loading

This commit adds new annotation called:

io.podman.annotations.pids-limit/$ctrname

This annotation is used to define the PIDsLimit for a particular pod. It is also automatically defined when newly added --pids-limit option is used.

Fixes: #24418

Does this PR introduce a user-facing change?

The `podman kube play` now supports the `--pids-limit` command line option to define the PIDsLimit for pod.

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Mar 21, 2025
@mheon
Copy link
Member

mheon commented Mar 21, 2025

@Luap99 Had a checkpoint flake on F41 remote root, this time, though only one of the tests failed. Doesn't seem nearly as serious as the Debian flake yet, but it is happening outside of that OS.

@mheon
Copy link
Member

mheon commented Mar 21, 2025

LGTM

Luap99
Luap99 reviewed Mar 21, 2025
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Is there a reason why we need to support both a annotation and a cli option?
Seems simpler to just support one thing instead?

@Luap99
Copy link
Member

Luap99 commented Mar 21, 2025

@Luap99 Had a checkpoint flake on F41 remote root, this time, though only one of the tests failed. Doesn't seem nearly as serious as the Debian flake yet, but it is happening outside of that OS.

If you mean this #24571 then this is happening for a long time and I agree that it is unrelated from the debian hangs.

@jankaluza
Copy link
Member Author

jankaluza commented Mar 21, 2025 •
edited
Loading

Is there a reason why we need to support both a annotation and a cli option? Seems simpler to just support one thing instead?

I do not have enough experience with the project to decide that. If you think we should support just the annotation, I can remove the cli option.

Update: I originally added both annotation and cli option just because that was suggested in the podman issue I'm trying to fix here.

@Luap99
Copy link
Member

Luap99 commented Mar 21, 2025

Just the annotation would make sense to me but I am interested in other opinions
@containers/podman-maintainers WDYT?

@mheon
Copy link
Member

mheon commented Mar 21, 2025

It's not unprecedented, we have CLI options for other things (e.g. --mac-address, --no-hosts) in play kube. I think it's fine to include it but won't object if others feel strongly we should remove.

@Luap99
Copy link
Member

Luap99 commented Mar 21, 2025

It's not unprecedented, we have CLI options for other things (e.g. --mac-address, --no-hosts) in play kube. I think it's fine to include it but won't object if others feel strongly we should remove.

Right but those have no corresponding yaml setting, do they? I am just looking at this purely from a support perspective, if we have two ways for the same thing it means double the code, documentation and tests for the most part. To me that is just extra load we don't have to take on. I would assume the users are happy with just one, the yaml annotation seems nicer to me as it can be added in generate kube.

@mheon
Copy link
Member

mheon commented Mar 21, 2025

I kind of assumed they did have corresponding annotations, so that generate kube could put containers in the right network? But I have not checked that

@jankaluza
Copy link
Member Author

Just annotation would also simplify the code a lot (no need to change the bindings and extra code for remote commands). I think less code to maintain is better, so if users are happy with just annotation, I would do it that way.

@aquirdTurtle
Copy link

Thanks everyone for jumping on this so quick 🥳

@baude
Copy link
Member

baude commented Mar 25, 2025

/lgtm

thanks for the contribution and welcome to the team @jankaluza

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2025
@Luap99
Copy link
Member

Luap99 commented Mar 25, 2025

/hold

@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 Mar 25, 2025
@Luap99
Copy link
Member

Luap99 commented Mar 25, 2025

The annotation vs cli question is still open and you don't seem to have answered on that @baude? Do you think we should support both? Just the annotation seems simpler.

@baude baude removed the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2025
@baude
Copy link
Member

baude commented Mar 25, 2025

I would agree, that if the annotation gives what users need, simply go with that. Adding CLI is always an additional burden.

@jankaluza
Copy link
Member Author

@baude , So, should I remove the CLI? I think the annotation should as well work. I'm asking to prevent extra work :-).

Add support for pids-limit annotation for podman kube play.
f15b088 
This commit adds new annotation called:

io.podman.annotations.pids-limit/$ctrname

This annotation is used to define the PIDsLimit for
a particular pod. It is also automatically defined
when newly added --pids-limit option is used.

Fixes: containers#24418

Signed-off-by: Jan Kaluza <[email protected]>
@jankaluza
Copy link
Member Author

I've removed the cli option and kept just the annotation.

Luap99
Luap99 approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 26, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jankaluza, Luap99

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2025
@mheon
Copy link
Member

mheon commented Mar 26, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2025
@ygalblum
Copy link
Contributor

LGTM @Luap99 since you put the hold label, I'm leaving it to you to remove.

@Luap99
Copy link
Member

Luap99 commented Mar 26, 2025

/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 Mar 26, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit cb24660 into containers:main Mar 26, 2025
80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@mheon mheon

@baude baude

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for --pids-limit in kube
6 participants
@jankaluza @mheon @Luap99 @aquirdTurtle @baude @ygalblum