OCPNODE-4179: Migrating test case OCP-80983 from openshift-tests-private to origin#30899
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughMoved MicroShift detection to a Describe-level BeforeEach that skips the entire Describe on MicroShift; removed per-test MicroShift checks. Added an It test that verifies Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
| }) | ||
|
|
||
| //author: cmaurya@redhat.com | ||
| g.It("should verify cgroupv2 is the default and cgroupv1 cannot be set [OCP-80983] [apigroup:config.openshift.io] [apigroup:machineconfiguration.openshift.io]", func() { |
There was a problem hiding this comment.
Add the “[OTP]” annotation in the test name to tests that are ported from openshift-test-private. And also I think no need to add Polarian test case name.
Can remove [OCP-80983] [apigroup:config.openshift.io] [apigroup:machineconfiguration.openshift.io] .
There was a problem hiding this comment.
Changed in the latest comit
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
d167fb2 to
c6a09a4
Compare
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
| nodeConfig.Spec.CgroupMode = configv1.CgroupMode("v1") | ||
| _, updateErr := oc.AdminConfigClient().ConfigV1().Nodes().Update(ctx, nodeConfig, metav1.UpdateOptions{}) | ||
| o.Expect(updateErr).To(o.HaveOccurred(), "expected API server to reject cgroupMode v1, but update succeeded") | ||
| e2e.Logf("cgroupMode v1 correctly rejected with error: %v", updateErr) |
There was a problem hiding this comment.
When changing cgroup from v2 to v1, in message it should show like this: error: nodes.config.openshift.io "cluster" is invalid .
There was a problem hiding this comment.
please update the error log message
There was a problem hiding this comment.
Changed in the latest comit. The error message will look like this.
The Node "cluster" is invalid: spec.cgroupMode: Unsupported value: "v1": supported values: "v2", ""
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
c6a09a4 to
0fb028c
Compare
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
0fb028c to
332f7a4
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
|
||
| g "github.com/onsi/ginkgo/v2" | ||
| o "github.com/onsi/gomega" | ||
|
|
There was a problem hiding this comment.
removed in the latest commit
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 332f7a4
|
| g.By("2) Changing cgroup from v2 to v1 should result in error") | ||
| output, err := oc.AsAdmin().WithoutNamespace().Run("patch").Args("nodes.config.openshift.io", "cluster", "-p", `{"spec": {"cgroupMode": "v1"}}`, "--type=merge").Output() | ||
| o.Expect(err).Should(o.HaveOccurred()) | ||
| o.Expect(strings.Contains(output, "Unsupported value: \"v1\"")).Should(o.BeTrue()) |
There was a problem hiding this comment.
done in the latest code commit
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
c7c5091 to
c2c8ffd
Compare
![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 (1)
test/extended/node/node_e2e/node.go (1)
19-26: Cache the MicroShift probe result instead of re-running it before every test
exutil.IsMicroShiftClustercan poll for minutes; putting it inBeforeEachrepeats that cost for everyItand increases timeout/flakiness risk as this block grows. Compute once perDescribeand reuse the result inBeforeEach.Suggested refactor
+import "sync" + var _ = g.Describe("[sig-node] [Jira:Node/Kubelet] Kubelet, CRI-O, CPU manager", func() { var ( oc = exutil.NewCLIWithoutNamespace("node").AsAdmin() + microShiftOnce sync.Once + isMicroShift bool + microShiftErr error ) // Skip all tests on MicroShift clusters as MachineConfig resources are not available g.BeforeEach(func() { - isMicroShift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient()) - o.Expect(err).NotTo(o.HaveOccurred()) + microShiftOnce.Do(func() { + isMicroShift, microShiftErr = exutil.IsMicroShiftCluster(oc.AdminKubeClient()) + }) + o.Expect(microShiftErr).NotTo(o.HaveOccurred()) if isMicroShift { g.Skip("Skipping test on MicroShift cluster") } })As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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` around lines 19 - 26, Compute the MicroShift probe once and reuse it: declare a Describe-scoped (or package-scoped) variable like cachedIsMicroShift bool, run the expensive probe a single time (e.g., in g.BeforeSuite or at the outer Describe setup) by calling exutil.IsMicroShiftCluster(oc.AdminKubeClient()) and storing the result (and assert err with o.Expect(err).NotTo(o.HaveOccurred())), then change the existing g.BeforeEach block to check cachedIsMicroShift and call g.Skip("Skipping test on MicroShift cluster") when true instead of re-invoking exutil.IsMicroShiftCluster; keep existing references to o.Expect and g.Skip but remove the repeated probe call.
🤖 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 90-92: The assertion is too brittle by matching the full error
sentence; relax it to assert the command failed
(o.Expect(err).Should(o.HaveOccurred())) and that the output contains stable
fragments such as "spec.cgroupMode", "Unsupported value" and the rejected value
"\"v1\"" (or "v1") instead of the entire message. Locate the patch invocation
(oc.AsAdmin().WithoutNamespace().Run("patch")... ) and replace the single
o.ContainSubstring check with multiple contains assertions for those fragments
(or a single contains that concatenates stable fragments) to make the test
resilient to wording/casing changes.
---
Nitpick comments:
In `@test/extended/node/node_e2e/node.go`:
- Around line 19-26: Compute the MicroShift probe once and reuse it: declare a
Describe-scoped (or package-scoped) variable like cachedIsMicroShift bool, run
the expensive probe a single time (e.g., in g.BeforeSuite or at the outer
Describe setup) by calling exutil.IsMicroShiftCluster(oc.AdminKubeClient()) and
storing the result (and assert err with o.Expect(err).NotTo(o.HaveOccurred())),
then change the existing g.BeforeEach block to check cachedIsMicroShift and call
g.Skip("Skipping test on MicroShift cluster") when true instead of re-invoking
exutil.IsMicroShiftCluster; keep existing references to o.Expect and g.Skip but
remove the repeated probe call.
🪄 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: 8a542e0c-a347-48eb-b629-3c7e9932b606
📒 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 tests seen in this PR at sha: c2c8ffd
|
|
/retest |
| //author: cmaurya@redhat.com | ||
| g.It("[OTP] validate cgroupv2 is default", func() { | ||
| g.By("Get a Ready worker node and check cgroup version") | ||
| workerNode, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("nodes", "-l", "node-role.kubernetes.io/worker", "-o=jsonpath={.items[0].metadata.name}").Output() |
There was a problem hiding this comment.
Here you just check the first worker, if it is not Ready we can't go on checking other workers. It's better to check all the workers and skip when any worker is not Ready.
There was a problem hiding this comment.
Changes done in the latest commit. Now, each line shows the cgroup version for a different worker node which are ready and skip if not Ready.
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
beda6cf to
9cb316b
Compare
|
@Chandan9112: This pull request references OCPNODE-4179 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. |
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
2077761 to
a738151
Compare
openshift-ci
bot
removed
the
approved
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
a738151 to
42e4f62
Compare
openshift-ci
bot
added
the
approved
Made-with: Cursor
Chandan9112
force-pushed
the
migrate-ocp-80983
branch
from
42e4f62 to
3eb467c
Compare
|
Scheduling required tests: |
|
/lgtm |
sairameshv
left a comment
sairameshv
left a comment
There was a problem hiding this comment.
/hold cancel
/verified by ci
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 3eb467c
|
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 3eb467c
New tests seen in this PR at sha: 3eb467c
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Chandan9112, lyman9966, sairameshv 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 |
|
/verified by @Chandan9112 |
openshift-ci-robot
added
the
verified
|
@Chandan9112: 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. |
|
@Chandan9112: 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. |
6 participants
Adds automated test case OCP-80983 migrated from openshift-tests-private. The test validates:
Here is the test case link: Polarian-80983
Note for Reviewer:
Moved MicroShift skip check from individual test cases to g.BeforeEach block,
following the same pattern used in node_swap.go and image_volume.go.
This avoids duplicating the check inside every test case.
It passed successfully while executing on a live OCP 4.22 cluster: