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

Quadlet: Add support for --sysctl #18785

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Quadlet: Add support for --sysctl #18785

merged 1 commit into from
Jun 9, 2023

Conversation

LauKr
Copy link
Contributor

@LauKr LauKr commented Jun 2, 2023 •
edited by ygalblum

AddSysctl allows to set namespaced kernel parameters for containers.
Closes #18727.

Does this PR introduce a user-facing change?

Yes

Quadlet - Add support for --sysctl via Sysctl flag for containers.

Signed-off-by: Laurenz Kruty git@laurenzkruty.de

Luap99
Luap99 reviewed Jun 5, 2023
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.

Thanks for the contribution, please add a test for it. See #18788 for an example on how to add quadlet tests.

pkg/systemd/quadlet/quadlet.go Outdated Show resolved Hide resolved
@Luap99
Copy link
Member

Luap99 commented Jun 5, 2023

Also please squash the commits into one, thanks.

TomSweeneyRedHat
TomSweeneyRedHat reviewed Jun 5, 2023
View reviewed changes
docs/source/markdown/podman-systemd.unit.5.md Outdated

For example:
```
AddSysctl=net.ipv6.conf.all.disable_ipv6=1 net.ipv6.conf.all.use_tempaddr=1
Copy link
Member

Choose a reason for hiding this comment

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

Just checking, not comma separated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To my knowledge both is possible.
When using a comma separated list podman inspect returns

--sysctl= AddSysctl=net.ipv6.conf.all.disable_ipv6=1,net.ipv6.conf.all.use_tempaddr=1

In contrast, using the space separated list leads to

sysctl=net.ipv6.conf.all.disable_ipv6=1 
sysctl=net.ipv6.conf.all.use_tempaddr=1

I personally would prefer the latter, however the documentation can be changed if wanted.

@TomSweeneyRedHat
Copy link
Member

Tests are unhappy.

@LauKr
Copy link
Contributor Author

LauKr commented Jun 8, 2023

Thanks for the contribution, please add a test for it. See #18788 for an example on how to add quadlet tests.

The linked PR has an ## assert-podman-final-args localhost/imagename, is this also necessary here? I saw several other tests without it though.

@Luap99
Copy link
Member

Luap99 commented Jun 8, 2023

Thanks for the contribution, please add a test for it. See #18788 for an example on how to add quadlet tests.

The linked PR has an ## assert-podman-final-args localhost/imagename, is this also necessary here? I saw several other tests without it though.

As long as you make sure you have the correct --sysctl argument in there is should be fine.

@mheon
Copy link
Member

mheon commented Jun 8, 2023

Code LGTM

@LauKr
Copy link
Contributor Author

LauKr commented Jun 8, 2023

Sorry, forgot to change the alphabetical sorting on one occasion (supportedContainerKeys). I will fix that, as it's only the order of Supported keys this will be a minor change. However, the checks have to rerun.

@LauKr
Quadlet: Add support for --sysctl flag
b37f74b 
The Sysctl=name=value entry can be used to set --sysctl=name=value
directly without the need to use PodmanArgs=--sysctl=name=value.

Signed-off-by: Laurenz Kruty <git@laurenzkruty.de>
@TomSweeneyRedHat
Copy link
Member

LGTM
Thanks @LauKr !

Luap99
Luap99 approved these changes Jun 9, 2023
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 bot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LauKr, 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 Jun 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit 8107957 into containers:main Jun 9, 2023
87 checks passed
@LauKr
Copy link
Contributor Author

LauKr commented Jun 9, 2023

Thanks everybody

@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 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rhatdan rhatdan rhatdan left review comments

@ygalblum ygalblum ygalblum left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@Luap99 Luap99 Luap99 approved these changes

Assignees

@Luap99 Luap99

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.

[Quadlet]: Add support for --sysctl
7 participants
@LauKr @Luap99 @TomSweeneyRedHat @mheon @rhatdan @ygalblum @openshift-merge-robot