first success path test for NRI integration tests#2071
Conversation
k8s-ci-robot
added
kind/feature
k8s-ci-robot
added
approved
|
@haircommander this is the framework with a single smoke test |
|
to simplify the review here are snippets from CI execution: CRI-O: https://github.com/kubernetes-sigs/cri-tools/actions/runs/25518494319/job/74895989859?pr=2071 Containerd: |
8c1493d to
7107aca
Compare
SergeyKanzhelev
changed the title
|
@haircommander @saschagrunert @samuelkarp can you please review this? Let's merge a simple test and handle more complex use cases one by one |
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Overall design is solid: an in-process NRI stub that registers as a plugin, records events, and validates ordering. The test is well-structured and the CI integration covers both containerd and CRI-O. A few comments inline.
| } | ||
|
|
||
| p.mu.Unlock() | ||
| time.Sleep(50 * time.Millisecond) |
There was a problem hiding this comment.
nit: Both WaitForEvent and WaitForEventCount use time.Sleep(50ms) polling loops. A sync.Cond broadcast from recordPodEvent/recordContainerEvent would be more idiomatic, avoid the latency/CPU tradeoff, and scale better as more tests use this framework.
Not a blocker, but worth considering since this is reusable test infrastructure.
There was a problem hiding this comment.
With sync.Cond, we will need a goroutine to cancel by timeout, right? I can do channel instead, but not sure it worth it
| - name: Configure NRI for CRI-O | ||
| run: | | ||
| sudo mkdir -p /var/run/nri /opt/nri/plugins | ||
| sudo tee /etc/crio/crio.conf.d/05-nri.conf <<EOF |
There was a problem hiding this comment.
nit: The heredoc body is indented to match the YAML nesting level, so the leading whitespace ends up in the actual TOML file written to /etc/crio/crio.conf.d/05-nri.conf. CRI-O's parser tolerates this, but it's a bit fragile. Unindenting the heredoc body or using <<-EOF with tabs would be cleaner.
There was a problem hiding this comment.
I thought yaml will remove all whitespaces. Also those are whitespaces, I looked at <<-EOF and it removes tabs.
The CI shows no 9 whitespaces, I cannot tell if those two are definitely a log formatting, but it sure looks like it:
7107aca to
d402009
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert, SergeyKanzhelev The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
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.
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This is the initial framework from #2069
Related to: #2046