-
Notifications
You must be signed in to change notification settings - Fork 485
first iteration of NRI tests #2069
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # NRI Spec Discrepancies | ||
|
|
||
| This document tracks cases where observed runtime behavior differs from the | ||
| NRI Pod Sandbox Lifecycle specification (containerd/nri#286). | ||
|
|
||
| ## StopPodSandbox idempotency — sandbox reuse after Stop | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this deserve a bug in containerd? @samuelkarp |
||
|
|
||
| **Spec requirement** (from containerd/nri#286): After StopPodSandbox, the | ||
| sandbox is never reused — CreateContainer on a stopped sandbox MUST fail. | ||
|
|
||
| **Observed behavior**: containerd allows CreateContainer to succeed on a | ||
| stopped sandbox, returning a valid container ID instead of an error. | ||
|
|
||
| **Contract points in this scenario:** | ||
|
|
||
| - [x] StopPodSandbox is idempotent (multiple calls succeed without error) | ||
| - [x] StopPodSandbox NRI hook fires at least once | ||
| - [ ] CreateContainer on a stopped sandbox fails ← discrepancy | ||
|
|
||
| **Impact**: A plugin that assumes stopped sandboxes are immutable may see | ||
| unexpected container lifecycle events after StopPodSandbox. | ||
|
|
||
| **Status**: Open | ||
|
|
||
| ## Multi-plugin teardown hook delivery after plugin failure | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @samuelkarp is this a bug or intended behavior? It may be intended if we want success of "previous" plugin before running next one. wdyt? |
||
|
|
||
| **Spec requirement** (from containerd/nri#286): One plugin's failure on | ||
| StopPodSandbox/RemovePodSandbox MUST NOT prevent delivery of those hooks to | ||
| subsequent plugins. | ||
|
|
||
| **Observed behavior**: NRI aborts hook delivery to remaining plugins when one | ||
| plugin returns an error. Plugin 1 (higher index) receives zero | ||
| StopPodSandbox/RemovePodSandbox hooks when plugin 0 (lower index) returns an | ||
| error. | ||
|
|
||
| **Contract points in this scenario:** | ||
|
|
||
| - [x] Plugin 0 receives StopPodSandbox hook | ||
| - [x] Plugin 0 receives RemovePodSandbox hook | ||
| - [ ] Plugin 1 receives StopPodSandbox hook despite plugin 0's error ← discrepancy | ||
| - [ ] Plugin 1 receives RemovePodSandbox hook despite plugin 0's error ← discrepancy | ||
| - [x] StopPodSandbox CRI call succeeds despite plugin error | ||
| - [x] RemovePodSandbox CRI call succeeds despite plugin error | ||
| - [x] Sandbox is fully removed | ||
|
|
||
| **Impact**: A lower-index plugin returning an error on teardown silently | ||
| prevents all higher-index plugins from receiving teardown notifications, | ||
| potentially causing resource leaks in those plugins. | ||
|
|
||
| **Status**: Open | ||
|
|
||
| ## containerd: NRI plugin errors swallowed on StopPodSandbox CRI call | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I strongly suggest we fix it. Errors may be not even plugin related (conneciton issue). And we have to ensure that everything is cleaned up by all NRI plugins. @aojea to weight in on importance of clean up for dranet |
||
|
|
||
| **Spec requirement**: StopPodSandbox and RemovePodSandbox CRI calls MUST | ||
| propagate NRI plugin errors to the CRI caller so the caller is aware of | ||
| the failure. | ||
|
|
||
| **Observed behavior** (containerd): When an NRI plugin returns an error on | ||
| StopPodSandbox or RemovePodSandbox, containerd swallows the error and | ||
| returns success to the CRI caller. CRI-O correctly propagates the error. | ||
|
|
||
| **Affected runtimes**: containerd (CRI-O handles this correctly) | ||
|
|
||
| **Impact**: The CRI caller is unaware that an NRI plugin failed during | ||
| teardown, potentially masking resource leaks or plugin state corruption. | ||
|
|
||
| **Status**: Open | ||
|
|
||
| ## CRI-O: empty container name in NRI CreateContainer event metadata | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @haircommander does this deserve a bug in CRI-O? |
||
|
|
||
| **Spec requirement**: NRI CreateContainer events should include the container | ||
| name in the event metadata so plugins can identify containers. | ||
|
|
||
| **Observed behavior** (CRI-O): The `ContainerName` field in NRI | ||
| CreateContainer events is an empty string. | ||
|
|
||
| **Affected runtimes**: CRI-O (containerd populates this correctly) | ||
|
|
||
| **Impact**: Plugins that rely on container name for policy decisions cannot | ||
| identify containers during CreateContainer hooks on CRI-O. | ||
|
|
||
| **Status**: Open | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I'm not positive it makes sense to put this here. maybe we can open an issue here or in NRI about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I listed here for review simplification. I want some confirmation it makes sense and needs fixing and it may be easier to discuss in PR comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add those to a separate commit with a warning to not merge it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I can split this PR into smaller pieces to start making progress. If it looks generally OK as a approach/direction, we can start merging things