Skip to content

Conversation

@samuelkarp
Copy link
Member

Testing: unit tests including in the default validator. I have not yet built a new containerd binary with these changes and built a plugin to set sysctls. Let me know if you want to see that before merging.

Another note: right now the default validator configuration is RejectSysctlAdjustment and off by default, so either we can change that to a positive "AllowSysctlAdjustment" or we can turn it on by default in containerd when we integrate this change.

Signed-off-by: Samuel Karp <samuelkarp@google.com>
Signed-off-by: Samuel Karp <samuelkarp@google.com>
Signed-off-by: Samuel Karp <samuelkarp@google.com>
@klihub
Copy link
Member

klihub commented Dec 2, 2025

Testing: unit tests including in the default validator. I have not yet built a new containerd binary with these changes and built a plugin to set sysctls. Let me know if you want to see that before merging.

This looks straightforward enough to not warrant insisting on that. Then again, before a new NRI version gets eventually tagged with this included, we need to test this with the runtimes and make sure it works.

Another note: right now the default validator configuration is RejectSysctlAdjustment and off by default, so either we can change that to a positive "AllowSysctlAdjustment" or we can turn it on by default in containerd when we integrate this change.

I think the current naming and semantics ('enabled by default via a reject-setting defaulting to false') is in line with how the other similar validator settings are configured. So unless there is some other reason for it, I don't think it'd need to be flipped the other way around.

Copy link
Member

@klihub klihub left a comment

Choose a reason for hiding this comment

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

LGTM.

@samuelkarp samuelkarp merged commit 96f0f20 into containerd:main Dec 2, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants