pkg/cri/sbserver: experimental NRI integration for CRI. #7954
Conversation
|
Hi @klihub. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
klihub
force-pushed
the
devel/sbserver-nri-integration
branch
3 times, most recently
from
f7377f9 to
d0ca371
Compare
klihub
force-pushed
the
devel/sbserver-nri-integration
branch
6 times, most recently
from
06e4121 to
6de3983
Compare
klihub
force-pushed
the
devel/sbserver-nri-integration
branch
from
6de3983 to
bc00906
Compare
klihub
force-pushed
the
devel/sbserver-nri-integration
branch
from
bc00906 to
2bede55
Compare
pkg/cri/sbserver/container_create.go
Outdated
| opts = append(opts, containerd.WithSandbox(sandboxID)) | ||
| } | ||
|
|
||
| if c.nri.IsEnabled() { |
There was a problem hiding this comment.
Is the caller responsible for checking IsEnabled?
nri.API sometimes checks IsEnabled inside its associated functions, sometimes doesn't and we have nri.API's no-op implementation for non-Linux platforms.
Is it possible to use the no-op implementation for this disabled case?
There was a problem hiding this comment.
Is the caller responsible for checking IsEnabled?
It should not be strictly necessary, as there is a test later in the call chain that checks and short-circuits the call at that level. Here I wanted to avoid scheduling the deferred function for possible failure cleanup and avoid adding the extra option which is unnecessary if NRI is disabled (and now that I look at it that option is ignorant about being disabled and relies on the other infra to properly not do anything then).
nri.API sometimes checks IsEnabled inside its associated functions, sometimes doesn't and we have nri.API's no-op implementation for non-Linux platforms.
Is it possible to use the no-op implementation for this disabled case?
Do you mean something like this ?
- change
APIto be a interface not a struct - have a common no-op implementation of
API(without any platform-specific build flags) - have platform-specific real implementations of
API - if NRI is disabled instantiate the no-op implementation,
- otherwise instantiate the real one
- eliminate all/most of the server- and sbserver-level
if c.nri.IsEnabled()checks
Or maybe instead of an interface, do the same thing with a 'frontend' struct that would then call to a real or a no-op implementation... but anyway, still mostly the same idea ?
If you think that would be a better approach (could be less intrusive/improve readability), I can give it a try and see what I manage to cook up...
There was a problem hiding this comment.
I wanted to remove IsEnabled checks, but this wouldn't cover defer case...
If NRI check is required for defer, how about using nri != nil instead of IsEnabled? It is not coming from your change though.
There was a problem hiding this comment.
If we make UndoCreateContainer() to properly become a no-op when NRI is disabled, then it should be fine to unconditionally defer calling it on the error return path. There are a few places where currently we need to do an extra sandbox store lookup for the container's sandbox on the error branch if NRI is enabled. But maybe if we did those lookups from within the called NRI API function then we could make those calls unconditionally, too.
Anyway, I'll check if I can check if I could remove the top-level IsEnabled() checks in some safe way.
There was a problem hiding this comment.
@kzys I updated this PR, moving the enabled/disabled checks from server and sbserver to the common, split out bits. PTAL.
klihub
force-pushed
the
devel/sbserver-nri-integration
branch
from
2bede55 to
389bec4
Compare
|
/retest-required |
Split out the criService-agnostic bits of nri-api* from pkg/cri/server to pkg/cri/nri to allow sharing a single implementation betwen the server and sbserver versions. Rework the interfaces to not require access to package internals. Signed-off-by: Krisztian Litkey <[email protected]>
Hook the NRI service plugin into CRI sbserver request processing. Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/sbserver-nri-integration
branch
from
389bec4 to
ebbcb57
Compare
|
/ok-to-test |
Update fork-external/main with upstream main at commit [081d818](containerd@081d818) Marged upstream container/main into fork-external/main Related work items: containerd#7864, containerd#7954, containerd#8041, containerd#8044, containerd#8051, containerd#8062, containerd#8096, containerd#8103, containerd#8109, containerd#8110, containerd#8113, containerd#8114, containerd#8119, containerd#8120, containerd#8128, containerd#8130, containerd#8134, containerd#8140, containerd#8142, containerd#8143, containerd#8152, containerd#8154, containerd#8162, containerd#8164, containerd#8165, containerd#8172, containerd#8173, containerd#8177, containerd#8178, containerd#8181, containerd#8183, containerd#8187, containerd#8188, containerd#8189, containerd#8190, containerd#8191, containerd#8192, containerd#8193
5 participants
Split out the NRI integration bits ('nri-api') from
pkg/cri/servertopkg/cri/nri, allowing a single implementation to be shared between theserverandsbservervariants of the CRI plugin. Rework it to not assume access to unexported internals ofserverorsbserver. Add code to hook the NRI service plugin and NRI processing into the 'sbserver' CRI plugin variant.