device-injector: implement sysctl adjustment annotations. #262
Conversation
mikebrow
left a comment
mikebrow
left a comment
There was a problem hiding this comment.
just a minor change requested
| as the network interface `netdev0`, and the host network interface `ens2.101` into | ||
| container `c1` as the network interface `netdev1`. | ||
|
|
||
| ### Sysctl Annotations |
There was a problem hiding this comment.
need to add sysctl to the top of the readme..
| as the network interface `netdev0`, and the host network interface `ens2.101` into | ||
| container `c1` as the network interface `netdev1`. | ||
|
|
||
| ### Sysctl Annotations |
There was a problem hiding this comment.
It seems a little odd to me to put sysctls in the device injector. This plugin has turned into a bit of a grab-bag. Should we rename this so it's more generic of a plugin?
There was a problem hiding this comment.
Yes, we should. I was thinking the same, basically to 1) rename the plugin as container-adjuster, 2) add command line options (and probably env. vars) to allow selectively disabling unwanted types of adjustments.
There was a problem hiding this comment.
Do you prefer doing it as part of this PR, or as a separate one ?
There was a problem hiding this comment.
I'm fine with it being separate, but we should do it soon.
- add command line options (and probably env. vars) to allow selectively disabling unwanted types of adjustments
Is our goal here mostly to demo different NRI adjustments or to provide a plugin we expect users to actually run? For a demo I don't think the command-line options are really necessary, but for a production-ready plugin we probably need more than just those changes.
There was a problem hiding this comment.
Agreed, but test/demo are the same category to me.
There was a problem hiding this comment.
as with ctr.. test/demo/debug tools tend to get used beyond our recommendations
There was a problem hiding this comment.
Understood. My question is still about intent though. If it is our intent for this to be a more broadly-useful plugin, we have additional work to do. If not, then I think the command-line options are unnecessary.
There was a problem hiding this comment.
@mikebrow @samuelkarp We can limit our changes for now to those that we think are necessary regardless of whether we intend it as a test/demo tool or as a more generally useful plugin for folks to consume and use. I think those would include at least this PR, a few fixes to the docs for undocumented existing features I spotted, and the renaming of the plugins to something more closely reflecting reality (maybe container-adjuster).
Then we can decide later if we intend this as a more generally useful plugin that folks could consume, and do the remaining necessary work if decide to do so.
klihub
force-pushed
the
devel/device-injector/sysctl-adjustment
branch
4 times, most recently
from
bbe1f1b to
6f3bf01
Compare
| all. Trying to adjust such a setting will result in an error and a failure to | ||
| create the container. |
There was a problem hiding this comment.
An error from the container runtime? I don't know if we have validation for namespaced sysctls within containerd...
There was a problem hiding this comment.
It's probably in runc/crun. With runc I got an error message specifically stating that I was trying to set a non-namespaced sysctl setting, and it did fail container creation.
There was a problem hiding this comment.
runc guys.. it's probably in lib container ... lib container guys.. it's the kernel
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
klihub
force-pushed
the
devel/device-injector/sysctl-adjustment
branch
from
6f3bf01 to
624ea10
Compare
3 participants
Add support for adjusting sysctl settings.