NO-JIRA: Move "managementState: Removed" test to OTE#337
NO-JIRA: Move "managementState: Removed" test to OTE#337jsafrane wants to merge 4 commits intoopenshift:mainfrom
Conversation
Move the test from "unit test" framework in test/e2e to OpenShift test extension in openshift-tests. Move the common code from the existing test to common.go
openshift-ci-robot
added
the
jira/valid-reference
|
@jsafrane: This pull request explicitly references no 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. |
openshift-ci
Bot
added
the
approved
WalkthroughAdds an exported E2E helper to build clientsets and a cancellable timeout context from the default kubeconfig, refactors hybrid environment setup to use this helper, and introduces a new vSphere CSI operator-removal conformance E2E test that drives ClusterCSIDriver management-state changes and asserts operator/resource removal and recovery. ChangesvSphere CSI operator removal & client initialization
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant KubeAPI as Kubernetes API
participant ClusterOp as ClusterOperator/storage
participant CSIDriver as ClusterCSIDriver
participant Deployment as Deployment/DaemonSet
Test->>KubeAPI: NewE2EClientsFromDefaultKubeconfig(userAgent, timeout)
KubeAPI-->>Test: Clientsets + ctx
Test->>ClusterOp: Poll for Available=True,Progressing=False,Degraded=False
ClusterOp-->>Test: Ready
Test->>CSIDriver: Read ClusterCSIDriver (enabled)
CSIDriver-->>Test: Enabled
Test->>CSIDriver: Update Spec.ManagementState=Removed
CSIDriver-->>Test: Spec updated
Test->>ClusterOp: Poll for VMwareVSphereControllerDisabled condition
ClusterOp-->>Test: Condition true
Test->>Deployment: Verify controller Deployment deleted
Deployment-->>Test: Deleted
Test->>Deployment: Verify node DaemonSet deleted
Deployment-->>Test: Deleted
Test->>CSIDriver: DeferCleanup -> set Spec.ManagementState=Managed
CSIDriver-->>Test: Restored
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![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)
openshift-tests/e2e/operator_removal.go (2)
105-131: Consider simplifying the return statements.At lines 116 and 130, the nil checks (
deployment != nil,daemonset != nil) are redundant. WhenGetsucceeds (no error), the returned object is always non-nil. The code is correct but could be clearer.♻️ Suggested simplification
func checkForDeploymentCreation(ctx context.Context, kubeClient *kubernetes.Clientset) (bool, error) { - deployment, err := kubeClient.AppsV1().Deployments(csiDriverNameSpace).Get(ctx, csiControllerDeploymentName, metav1.GetOptions{}) + _, err := kubeClient.AppsV1().Deployments(csiDriverNameSpace).Get(ctx, csiControllerDeploymentName, metav1.GetOptions{}) if apierrors.IsNotFound(err) { GinkgoWriter.Printf("Deployment does not exist\n") return false, nil } if err != nil { GinkgoWriter.Printf("Unable to retrieve deployment: %v\n", err) return false, err } - return deployment != nil, nil + return true, nil }Apply similar change to
checkForDaemonset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests/e2e/operator_removal.go` around lines 105 - 131, The helper functions checkForDeploymentCreation and checkForDaemonset perform redundant nil checks after successful Gets; replace the final "return deployment != nil, nil" and "return daemonset != nil, nil" with an explicit "return true, nil" since a successful Get (err == nil) guarantees a non-nil object, keeping the existing error handling unchanged.
204-254: Minor logging clarity issue at line 250.The log output uses double negatives (
!notProgressing,!notDegraded) which can be confusing. Consider renaming variables or adjusting the log format for clarity.♻️ Suggested improvement for clearer logging
conditions := clusterOperator.Status.Conditions available := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorAvailable, configv1.ConditionTrue) - notProgressing := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorProgressing, configv1.ConditionFalse) - notDegraded := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorDegraded, configv1.ConditionFalse) - done := available && notProgressing && notDegraded + progressing := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorProgressing, configv1.ConditionTrue) + degraded := clusteroperatorhelpers.IsStatusConditionPresentAndEqual(conditions, configv1.OperatorDegraded, configv1.ConditionTrue) + done := available && !progressing && !degraded ... - GinkgoWriter.Printf("ClusterOperator/storage: Available: %v Progressing: %v Degraded: %v\n", available, !notProgressing, !notDegraded) + GinkgoWriter.Printf("ClusterOperator/storage: Available: %v Progressing: %v Degraded: %v\n", available, progressing, degraded)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openshift-tests/e2e/operator_removal.go` around lines 204 - 254, The log prints double-negatives which is confusing; in waitForStorageAvailable compute positive flags (e.g., progressing := !notProgressing and degraded := !notDegraded) or rename the existing booleans, then change the GinkgoWriter.Printf call to log available, progressing, degraded (using the new variables) instead of using !notProgressing and !notDegraded so the output is clear; update references to available, notProgressing, notDegraded accordingly in that scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openshift-tests/e2e/operator_removal.go`:
- Around line 79-84: The hardcoded time.Sleep(10 * time.Second) after
waitForStorageResourceRemoval makes the test flaky/slow; either remove it if
waitForStorageResourceRemoval already guarantees the desired state, or replace
it with an explicit wait using Gomega's Eventually (or equivalent) that checks
the concrete condition you need (e.g., resources absent, operator status stable)
and times out appropriately; if you truly must keep a pause, add a clear comment
explaining why and what specific race/propagation delay it mitigates. Reference:
remove/replace the time.Sleep call in the same block where
waitForStorageResourceRemoval(ctx, operatorClient, kubeClient) is invoked and
note that restoreStorage() runs via DeferCleanup.
---
Nitpick comments:
In `@openshift-tests/e2e/operator_removal.go`:
- Around line 105-131: The helper functions checkForDeploymentCreation and
checkForDaemonset perform redundant nil checks after successful Gets; replace
the final "return deployment != nil, nil" and "return daemonset != nil, nil"
with an explicit "return true, nil" since a successful Get (err == nil)
guarantees a non-nil object, keeping the existing error handling unchanged.
- Around line 204-254: The log prints double-negatives which is confusing; in
waitForStorageAvailable compute positive flags (e.g., progressing :=
!notProgressing and degraded := !notDegraded) or rename the existing booleans,
then change the GinkgoWriter.Printf call to log available, progressing, degraded
(using the new variables) instead of using !notProgressing and !notDegraded so
the output is clear; update references to available, notProgressing, notDegraded
accordingly in that scope.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9ed4ab04-2195-4815-92e1-0511638eb0b7
📒 Files selected for processing (3)
openshift-tests/e2e/common.goopenshift-tests/e2e/hybrid_environment.goopenshift-tests/e2e/operator_removal.go
jsafrane
changed the title
|
|
||
| By("Marking ClusterCSIDriver as removed") | ||
| makeClusterCSIDriverRemoved(ctx, operatorClient) | ||
| GinkgoWriter.Printf("✓ ClusterCSIDriver marked as removed\n") |
There was a problem hiding this comment.
I had some trouble logs to appear correctly with GinkgoWriter and CSO uses:
g.GinkgoLogr.Info("Created namespace with label", "namespace", nsObj.Name, "label", tc.namespaceLabel, "value", tc.namespaceLabelValue)
There was a problem hiding this comment.
Yes, IMO we should change all files to use GinkgoLogr, which is somewhat cleaner IMO.
I forgot why it exists. but with |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, jsafrane 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 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-serial |
|
@jsafrane: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9ab50c80-43c9-11f1-9e55-0645922153fc-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-serial |
|
@jsafrane: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/6023da70-47d3-11f1-93a9-c7ed02f5ef46-0 |
CI showed just an endless wall of "ClusterOperator/storage: Available: true Progressing: false Degraded: false", but the test timed out waiting for "Storage should be available".
|
New changes are detected. LGTM label has been removed. |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-serial |
|
@jsafrane: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/cfcb3290-4875-11f1-879c-4b412895851e-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-serial openshift/origin#31124 |
|
@jsafrane: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/feb5b540-4879-11f1-9b84-0bd24eaf093f-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-serial openshift/origin#31124 |
|
@jsafrane: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/37759370-4936-11f1-9e56-9cae5cc88990-0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift-tests/e2e/operator_removal.go`:
- Line 105: The debug log in GinkgoWriter.Printf currently includes a developer
prefix "JSAF" that should be removed; update the call to GinkgoWriter.Printf in
operator_removal.go (the invocation of GinkgoWriter.Printf("JSAF
ClusterCSIDriver: %s\n", string(obj))) to a cleaned, professional message such
as "ClusterCSIDriver: %s\n" (or another appropriate production log prefix) so
the debug artifact is not present in merged logs.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f7841e9a-24f0-413e-ae34-aa9973d8e7ad
📒 Files selected for processing (1)
openshift-tests/e2e/operator_removal.go
| GinkgoWriter.Printf("Unable to marshal ClusterCSIDriver: %v\n", err) | ||
| return clusterCSIDriverNotFound, err | ||
| } | ||
| GinkgoWriter.Printf("JSAF ClusterCSIDriver: %s\n", string(obj)) |
There was a problem hiding this comment.
Remove debug artifact before merge.
The log message contains what appears to be a developer prefix "JSAF" that should be removed before merging to keep logs clean and professional.
🧹 Suggested fix
- GinkgoWriter.Printf("JSAF ClusterCSIDriver: %s\n", string(obj))
+ GinkgoWriter.Printf("ClusterCSIDriver: %s\n", string(obj))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GinkgoWriter.Printf("JSAF ClusterCSIDriver: %s\n", string(obj)) | |
| GinkgoWriter.Printf("ClusterCSIDriver: %s\n", string(obj)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-tests/e2e/operator_removal.go` at line 105, The debug log in
GinkgoWriter.Printf currently includes a developer prefix "JSAF" that should be
removed; update the call to GinkgoWriter.Printf in operator_removal.go (the
invocation of GinkgoWriter.Printf("JSAF ClusterCSIDriver: %s\n", string(obj)))
to a cleaned, professional message such as "ClusterCSIDriver: %s\n" (or another
appropriate production log prefix) so the debug artifact is not present in
merged logs.
|
@jsafrane: 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. |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-serial openshift/origin#31124 |
|
@jsafrane: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/96558910-4952-11f1-97b0-fb3ae878c0e7-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-serial openshift/origin#31124 |
|
@jsafrane: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d510caf0-4968-11f1-8c7c-279887907322-0 |
4 participants
Move the test from "unit test" framework in test/e2e to OpenShift test extension in openshift-tests. I kept the test structure as close to the original code as possible, including magic
Sleep()at the end.Move the common code from the existing test to common.go
The old test in test/e2e needs to be removed in a subsequent PR.
Summary by CodeRabbit
New Features
Refactor