OCPNODE-4381: Migrate OCP-38271 from openshift-tests-private#30960
OCPNODE-4381: Migrate OCP-38271 from openshift-tests-private#30960BhargaviGudi wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced immediate MicroShift detection with a retrying check that fails after retries; added a new Ginkgo suite and an OCP-38271 test that creates a Pod with an init container, removes the init container via CRI on the node, and verifies the init container does not restart. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
BhargaviGudi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@BhargaviGudi: This pull request references OCPNODE-4381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
BhargaviGudi
force-pushed
the
migrate-38271
branch
from
cba718d to
b05cf8a
Compare
|
/test verify |
BhargaviGudi
force-pushed
the
migrate-38271
branch
2 times, most recently
from
f9f4ebe to
44944db
Compare
|
@BhargaviGudi: This pull request references OCPNODE-4381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by @BhargaviGudi |
openshift-ci-robot
added
the
verified
|
@BhargaviGudi: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
BhargaviGudi
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@BhargaviGudi: This pull request references OCPNODE-4381 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
BhargaviGudi
force-pushed
the
migrate-38271
branch
from
44944db to
3c8a2c2
Compare
openshift-ci-robot
removed
the
verified
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/extended/node/node_e2e/node.go (2)
151-151: Use absolute path/bin/shinstead of relativebin/sh.The command uses a relative path
bin/shwhich only works because the container's default working directory is/. Using the absolute path/bin/shis the standard convention and avoids potential issues ifWorkingDiris ever specified.♻️ Suggested fix
- Command: []string{"bin/sh", "-ec", "echo running >> /mnt/data/test"}, + Command: []string{"/bin/sh", "-ec", "echo running >> /mnt/data/test"},- Command: []string{"bin/sh", "-c", "sleep 3600"}, + Command: []string{"/bin/sh", "-c", "sleep 3600"},Also applies to: 164-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/node_e2e/node.go` at line 151, Replace the relative shell path in the container Command arrays with the absolute path; specifically update the Command entry in the container spec(s) in node.go (the Command: []string{"bin/sh", "-ec", ...} occurrences) to use "/bin/sh" instead of "bin/sh" (apply the same change to the other occurrence mentioned).
116-116: Remove unnecessarydefer g.GinkgoRecover().
GinkgoRecover()is designed for recovering panics in goroutines spawned within tests. At the Describe level, this defer executes when the Describe function returns (before any tests run), serving no purpose. Ginkgo already has built-in panic recovery for test blocks.♻️ Suggested fix
var _ = g.Describe("[sig-node] [Jira:Node/Kubelet] NODE initContainer policy,volume,readines,quota", func() { - defer g.GinkgoRecover() - var (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/node_e2e/node.go` at line 116, Remove the unnecessary defer g.GinkgoRecover() call from the Describe-level scope: the defer is only useful for recovering panics in goroutines spawned inside test blocks, and at the Describe level it runs too early and is redundant since Ginkgo already handles test panic recovery. Locate the defer g.GinkgoRecover() invocation (inside the Describe/registration setup in node.go) and delete that single line so no external panic recovery is deferred at describe initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/node_e2e/node.go`:
- Around line 241-258: The test currently returns success as soon as
RestartCount==0, which allows a race; change the wait.Poll predicate so it does
NOT return success when RestartCount==0 but instead keeps polling for the full
duration: inside the func passed to wait.Poll iterate
pod.Status.InitContainerStatuses for the "inittest" entry and if RestartCount>0
immediately return true, fmt.Errorf("init container restarted"); otherwise
return false, nil so polling continues; after wait.Poll, assert that the error
is a timeout (wait.ErrWaitTimeout) and fail if err is nil or a different error
(i.e. a restart was detected) — update the expectation that currently uses
o.Expect(err).NotTo(o.HaveOccurred()) accordingly.
---
Nitpick comments:
In `@test/extended/node/node_e2e/node.go`:
- Line 151: Replace the relative shell path in the container Command arrays with
the absolute path; specifically update the Command entry in the container
spec(s) in node.go (the Command: []string{"bin/sh", "-ec", ...} occurrences) to
use "/bin/sh" instead of "bin/sh" (apply the same change to the other occurrence
mentioned).
- Line 116: Remove the unnecessary defer g.GinkgoRecover() call from the
Describe-level scope: the defer is only useful for recovering panics in
goroutines spawned inside test blocks, and at the Describe level it runs too
early and is redundant since Ginkgo already handles test panic recovery. Locate
the defer g.GinkgoRecover() invocation (inside the Describe/registration setup
in node.go) and delete that single line so no external panic recovery is
deferred at describe initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d5d9835-9727-4042-b00a-649da2b71dd8
📒 Files selected for processing (1)
test/extended/node/node_e2e/node.go
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 3c8a2c2
New tests seen in this PR at sha: 3c8a2c2
|
|
/jira refresh |
|
@cpmeadors: This pull request references OCPNODE-4381 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
BhargaviGudi
force-pushed
the
migrate-38271
branch
from
3c8a2c2 to
8817055
Compare
BhargaviGudi
force-pushed
the
migrate-38271
branch
from
f68b696 to
5f0b875
Compare
|
Scheduling required tests: |
|
/test e2e-vsphere-ovn |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 5f0b875
New tests seen in this PR at sha: 5f0b875
|
|
/test e2e-metal-ipi-ovn-ipv6 |
BhargaviGudi
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
BhargaviGudi
force-pushed
the
migrate-38271
branch
from
5f0b875 to
d709535
Compare
|
Scheduling required tests: |
|
/test e2e-gcp-ovn |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d709535
New tests seen in this PR at sha: d709535
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BhargaviGudi, cpmeadors 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 |
|
/retest-required |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d709535
New tests seen in this PR at sha: d709535
|
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d709535
New tests seen in this PR at sha: d709535
|
|
/retest-required |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d709535
New tests seen in this PR at sha: d709535
|
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d709535
New tests seen in this PR at sha: d709535
|
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d709535
New tests seen in this PR at sha: d709535
|
|
/payload 4.22 nightly blocking |
|
@BhargaviGudi: trigger 13 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a3d021c0-3996-11f1-9435-b3930cff8739-0 |
Add test to verify init containers do not restart when removed from node. - Add pod-initContainer.yaml template - Add helper functions in node_utils.go - Add OCP-38271 test in node_e2e/node.go Author: minmli@redhat.com (original) Migrated-by: bgudi@redhat.com Move OCP-38271 test to separate Describe block Refactor OCP-38271 to use standard origin patterns instead of compat_otp Fix race condition and Add retry logic and explicit failure for MicroShift cluster check Resolved typo issue Resolved gofmt issue Missing leading slash in shell command path Use direct crictl args instead of bash -c Fix shell command Container image - Changed from busybox to hello-openshift@sha256:4200f438
BhargaviGudi
force-pushed
the
migrate-38271
branch
from
d709535 to
3cd0bdf
Compare
|
New changes are detected. LGTM label has been removed. |
|
Scheduling required tests: |
|
@BhargaviGudi: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: 3cd0bdf
|
3 participants
Summary Adds test to verify init containers do not restart when the exited init container is removed from the node.
Changes
test/extended/testdata/node/node_e2e/pod-initContainer.yaml- Pod template with init containertest/extended/node/node_utils.go:PodInitConDescriptionstruct with methods:Create(),Delete(),ContainerExit(),DeleteInitContainer(),InitContainerNotRestart()PodStatus()function to check pod readinesstest/extended/node/node_e2e/node.go:[OTP] Init containers should not restart when the exited init container is removed from node [OCP-38271]Original author: minmli@redhat.com
Migrated by: bgudi@redhat.com
tested manually on 4.22.0-0.nightly-2026-04-06-051707