OCPNODE-4381: Migrate OCP-38271 from openshift-tests-private#30960
OCPNODE-4381: Migrate OCP-38271 from openshift-tests-private#30960openshift-merge-bot[bot] merged 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 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 either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
|
Scheduling required tests: |
|
/cc @cpmeadors |
|
@bitoku Can you review this? |
|
I'm not sure why it was created in the private in the first place. |
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
c8a84db to
91231fd
Compare
|
/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/90afedd0-4848-11f1-896a-d10f500b536f-0 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BhargaviGudi, cpmeadors, ngopalak-redhat 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 |
|
Scheduling required tests: |
|
/retest-required |
|
/verified by CI |
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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 91231fd
New tests seen in this PR at sha: 91231fd
|
|
/retest-required |
|
/unhold |
|
/retest |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@BhargaviGudi: all tests passed! 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. |
|
/cherry-pick release-4.22 |
|
@BhargaviGudi: new pull request created: #31143 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 kubernetes-sigs/prow repository. |
6 participants
Summary Adds test to verify init containers do not restart when the exited init container is removed from the node.
Changes
test/extended/node/node_e2e/initcontainer.go- New test file containing:[OTP] Init containers should not restart when the exited init container is removed from node [OCP-38271]crictl rmvianodeutils.ExecOnNodeWithChroot()to delete the init container from the nodetest/extended/node/node_e2e/node.go- Add MicroShift cluster check to existing testsOriginal author: minmli@redhat.com
Migrated by: bgudi@redhat.com
tested manually on 4.22.0-0.nightly-2026-04-06-051707