Skip to content

first iteration of NRI tests#2069

Open
SergeyKanzhelev wants to merge 2 commits into
kubernetes-sigs:masterfrom
SergeyKanzhelev:nri-integration-tests
Open

first iteration of NRI tests#2069
SergeyKanzhelev wants to merge 2 commits into
kubernetes-sigs:masterfrom
SergeyKanzhelev:nri-integration-tests

Conversation

@SergeyKanzhelev

@SergeyKanzhelev SergeyKanzhelev commented May 7, 2026

Copy link
Copy Markdown
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

See description: #2046

Special notes for your reviewer:

Look at docs/nri-spec-discrepancies.md in this PR. Also tests are leaking because of containerd/containerd#13355

AI was heavily used for this.

Does this PR introduce a user-facing change?

The critest now allow to validate the NRI callbacks sequence and node lifecycle in relation with the CRI API.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 7, 2026
@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SergeyKanzhelev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 7, 2026
@SergeyKanzhelev

Copy link
Copy Markdown
Member Author

@SergeyKanzhelev
SergeyKanzhelev force-pushed the nri-integration-tests branch from 73919a8 to 71384d3 Compare May 7, 2026 06:24
Comment thread .github/workflows/containerd.yml Outdated
runtime_type = \"${{matrix.runtime}}\"
EOF'

# Enable NRI for containerd 2.x (main branch only)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

in 2.x is enabled by default, is this necessary?

@@ -0,0 +1,82 @@
# NRI Spec Discrepancies

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

is this deserve a bug in containerd? @samuelkarp


**Status**: Open

## Multi-plugin teardown hook delivery after plugin failure

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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?


**Status**: Open

## containerd: NRI plugin errors swallowed on StopPodSandbox CRI call

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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


**Status**: Open

## CRI-O: empty container name in NRI CreateContainer event metadata

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@haircommander does this deserve a bug in CRI-O?

Comment thread .github/workflows/containerd.yml Outdated
runtime_type = \"${{matrix.runtime}}\"
EOF'

# Enable NRI for containerd 2.x (main branch only)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary either, but he might want to skip certain nri tests for older releases

@mikebrow

mikebrow commented May 7, 2026

Copy link
Copy Markdown
Contributor

cc @klihub

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2026
@SergeyKanzhelev
SergeyKanzhelev force-pushed the nri-integration-tests branch from 71384d3 to b3d2022 Compare May 16, 2026 17:26
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 16, 2026
@SergeyKanzhelev
SergeyKanzhelev force-pushed the nri-integration-tests branch from b3d2022 to 23a9493 Compare May 20, 2026 22:24
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2026
SergeyKanzhelev added a commit to AIAgentSandox/cri-tools that referenced this pull request May 23, 2026
Extract the "RunPodSandbox state contract and blocking behavior" Context
from PR kubernetes-sigs#2069 into the NRI validation suite. The block adds two specs:

1. The sandbox is not exposed via List/Status while the RunPodSandbox NRI
   hook is in progress, and becomes Ready once the hook returns.
2. RunPodSandbox is gated by the hook completion, so workload container
   creation cannot proceed until the hook releases.

Also adds the supporting NRITestPlugin.Reset and
NRITestPlugin.LastRunPodSandboxID helpers, and resets recorded events on
NRITestStub.Cleanup so the new AfterEach can recover the sandbox ID when
a test fails before capturing it from the CRI call.

Following the styling feedback from PR kubernetes-sigs#2071, cleanup errors are logged
through framework.Logf rather than silenced.
SergeyKanzhelev added a commit to AIAgentSandox/cri-tools that referenced this pull request May 23, 2026
Extract the "RunPodSandbox state contract and blocking behavior" Context
from PR kubernetes-sigs#2069 into the NRI validation suite. The block adds two specs:

1. The sandbox is not exposed via List/Status while the RunPodSandbox NRI
   hook is in progress, and becomes Ready once the hook returns.
2. RunPodSandbox is gated by the hook completion, so workload container
   creation cannot proceed until the hook releases.

Also adds the supporting NRITestPlugin.Reset and
NRITestPlugin.LastRunPodSandboxID helpers, and resets recorded events on
NRITestStub.Cleanup so the new AfterEach can recover the sandbox ID when
a test fails before capturing it from the CRI call.

Following the styling feedback from PR kubernetes-sigs#2071, cleanup errors are logged
through framework.Logf rather than silenced.
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2026
@SergeyKanzhelev
SergeyKanzhelev force-pushed the nri-integration-tests branch from 23a9493 to 250e898 Compare May 27, 2026 18:54
SergeyKanzhelev added a commit to AIAgentSandox/cri-tools that referenced this pull request May 27, 2026
Add NRI test that validates the StopPodSandbox spec contract:
workload containers must be stopped before the StopPodSandbox NRI
hook fires, and sandbox infrastructure (PodSandboxStatus) must
still be accessible while the hook is in progress.

Extracted from kubernetes-sigs#2069 and aligned with the
existing "RunPodSandbox contract" tests in pkg/validate/nri_linux.go
(naming, AfterEach pattern using framework.Logf, hook blocking via
channels, sync.WaitGroup.Go).
SergeyKanzhelev added a commit to AIAgentSandox/cri-tools that referenced this pull request May 28, 2026
Add NRI test that validates the StopPodSandbox spec contract:
workload containers must be stopped before the StopPodSandbox NRI
hook fires, and sandbox infrastructure (PodSandboxStatus) must
still be accessible while the hook is in progress.

Extracted from kubernetes-sigs#2069 and aligned with the
existing "RunPodSandbox contract" tests in pkg/validate/nri_linux.go
(naming, AfterEach pattern using framework.Logf, hook blocking via
channels, sync.WaitGroup.Go).
@SergeyKanzhelev
SergeyKanzhelev force-pushed the nri-integration-tests branch from 250e898 to 2991f67 Compare May 29, 2026 16:25
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2026
Drop the smoke test ("should receive RunPodSandbox event when a pod is
created via CRI") — the pod sandbox lifecycle test already asserts the
RunPodSandbox event with metadata, plus Stop/Remove. Drop the
single-plugin "should propagate NRI plugin errors on StopPodSandbox and
RemovePodSandbox" test — the multi-plugin "should deliver teardown hooks
to all plugins even if one fails" test asserts the same error
propagation, plus fault isolation across plugins.

Align the remaining Context names with the existing short-noun style
(pod sandbox lifecycle, RunPodSandbox contract).
@SergeyKanzhelev
SergeyKanzhelev force-pushed the nri-integration-tests branch from 2991f67 to 26f9b89 Compare June 4, 2026 18:22
SergeyKanzhelev added a commit to AIAgentSandox/cri-tools that referenced this pull request Jun 4, 2026
Add the container lifecycle NRI test from kubernetes-sigs#2069 to nri_linux.go,
asserting that CreateContainer, StartContainer, StopContainer, and
RemoveContainer events arrive in strict order with correct metadata.

Align with the existing tests in the file by using framework.Logf for
cleanup error reporting (instead of the discarded errors used in the
original PR) and by following the same Context/BeforeEach/AfterEach
structure already established for the pod sandbox lifecycle and
StopPodSandbox contract tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants