OCPEDGE-2612: Automate OCP-88799-Verify reclaimPolicy and volumeBindingMode combinations

[mmakwana30]

Jira: https://redhat.atlassian.net/browse/OCPEDGE-2612

Logs: https://privatebin.corp.redhat.com/?f3ebe41a5a3d0fdc#BfSzHS9X6eCKwBC8EsZ17D8Bv29RestR6KEK9UwtjSAF

Summary by CodeRabbit

• Tests

  • Added integration tests validating LVMS behavior for combinations of reclaimPolicy and volumeBindingMode. Covers immediate-binding with automatic reclamation and delayed binding (WaitForFirstConsumer) with retained/manual reclamation; exercises PVC/pod lifecycles, verifies PV bind/unbind states, and confirms cleanup of retained volumes.

• Chores

  • Added a parameterized StorageClass template and test utilities to create StorageClasses used by the new test scenarios.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Added an OpenShift StorageClass template and a helper to apply it with defaults, plus a Ginkgo integration test that validates PVC/PV behavior for Delete+Immediate and Retain+WaitForFirstConsumer scenarios and cleans up logicalvolume CRs for retained volumes.

Changes

LVMS StorageClass Policy Test

Layer / File(s)	Summary
StorageClass template test/integration/qe_tests/testdata/storageclass-template.yaml	OpenShift Template storageclass-template that instantiates a StorageClass with parameterized name, provisioner, csi fstype, device class, reclaimPolicy, allowVolumeExpansion, and volumeBindingMode.
StorageClass creation helper test/integration/qe_tests/lvms_utils.go	Adds storageClassConfig and createStorageClassWithOC(cfg) which fills defaults (provisioner=topolvm.io, fsType=xfs, reclaimPolicy=Delete, volumeBindingMode=WaitForFirstConsumer), forces ALLOWEXPANSION=true, and applies storageclass-template via applyResourceFromTemplate.
StorageClass lifecycle integration test test/integration/qe_tests/lvms.go	Ginkgo test with two scenarios: (A) reclaimPolicy=Delete + volumeBindingMode=Immediate — PVC binds without a pod and PV is deleted after PVC removal; (B) reclaimPolicy=Retain + volumeBindingMode=WaitForFirstConsumer — PVC pending until pod, binds to PV, PVC deletion yields PV in Released state, orphaned PV removed, and logicalvolume CR is cleaned up via cleanupLogicalVolumeByName.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 12 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	⚠️ Warning	Test violates single responsibility by testing two unrelated behaviors in one It block; lacks meaningful failure messages on assertions.	Split test into separate It blocks for each scenario (Delete+Immediate vs Retain+WaitForFirstConsumer). Add failure messages to assertions.
Ipv6 And Disconnected Network Test Compatibility	⚠️ Warning	New test uses createPodWithOC() which defaults to pulling image from registry.redhat.io (external registry), incompatible with disconnected IPv6-only environments.	Either provide custom image from internal registry via podConfig.image, or pass custom image in createPodWithOC call. Alternatively, add [Skipped:Disconnected] to test name.

✅ Passed checks (12 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	Test title uses only stable, static text and Jira ID without dynamic values, timestamps, or generated identifiers.
Microshift Test Compatibility	✅ Passed	Test uses only Kubernetes standard APIs (StorageClass, PVC, PV, Pod) and TopoLVM CRD (topolvm.io), both supported on MicroShift. Test has skipguard via checkLvmsOperatorInstalled().
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The new test is labeled g.Label("SNO", "MNO") indicating SNO compatibility. It creates a single pod without multi-node assumptions and tests StorageClass behavior that works on SNO.
Topology-Aware Scheduling Compatibility	✅ Passed	PR contains only test code and test utilities with no scheduling constraints; StorageClass and Pod templates contain no topology-aware scheduling assumptions.
Ote Binary Stdout Contract	✅ Passed	PR properly routes klog to GinkgoWriter via init(), uses logf() utility for test logging, and introduces no process-level stdout writes. All logging safe.
No-Weak-Crypto	✅ Passed	PR contains only test code for Kubernetes storage validation; no weak crypto, custom crypto implementations, or insecure secret comparisons detected.
Container-Privileges	✅ Passed	No privileged containers, elevated capabilities, or privilege escalation. Pod templates enforce restrictive security with runAsNonRoot, dropped capabilities, and allowPrivilegeEscalation disabled.
No-Sensitive-Data-In-Logs	✅ Passed	New test and utility code logs only non-sensitive Kubernetes resource names, statuses, and standard configuration values; no passwords, tokens, API keys, PII, or credentials are exposed in logs.
Title check	✅ Passed	The title clearly references OCPEDGE-2612 and describes the main objective: automating verification of reclaimPolicy and volumeBindingMode combinations, which aligns with the test additions across three files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 `@test/integration/qe_tests/lvms_utils.go`:
- Around line 2010-2032: The helper currently forces ALLOWEXPANSION to false
when callers omit allowExpansion because Go's zero value is false; change
storageClassConfig.allowExpansion from bool to *bool so you can distinguish
"unset" (nil) from set values, then update createStorageClassWithOC to only set
allowExpansionStr/ALOWEXPANSION when cfg.allowExpansion != nil (use "true" or
"false" based on *cfg.allowExpansion); update callers of
storageClassConfig/createStorageClassWithOC to pass a pointer when they need to
override the default.

🪄 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 YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 7d9d79af-1719-446d-9e5f-58496a1c6dc0

📥 Commits

Reviewing files that changed from the base of the PR and between 5d77a65 and a30ef1a.

📒 Files selected for processing (3)

• test/integration/qe_tests/lvms.go
• test/integration/qe_tests/lvms_utils.go
• test/integration/qe_tests/testdata/storageclass-template.yaml

[coderabbitai]

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 `@test/integration/qe_tests/lvms.go`:
- Around line 5055-5066: The StorageClass names (e.g., the scDeleteImmediate
variable used in the createStorageClassWithOC call and its matching
deleteSpecifiedResource defer) are fixed and must be made unique by appending
the existing uniqueSuffix; update each cluster-scoped StorageClass name
assignment (e.g., scDeleteImmediate and the other SC variables around lines
5105-5116) to include "+" uniqueSuffix (or fmt.Sprintf("%s-%s", name,
uniqueSuffix)) and ensure the same unique name is passed into
createStorageClassWithOC and deleteSpecifiedResource so creation and cleanup use
the identical, unique identifier.

🪄 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 YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: e4d67c65-8ae8-4208-b4d6-ca22c79db9bd

📥 Commits

Reviewing files that changed from the base of the PR and between a30ef1a and 8f23d88.

📒 Files selected for processing (3)

• test/integration/qe_tests/lvms.go
• test/integration/qe_tests/lvms_utils.go
• test/integration/qe_tests/testdata/storageclass-template.yaml

✅ Files skipped from review due to trivial changes (1)

• test/integration/qe_tests/testdata/storageclass-template.yaml

[coderabbitai]

♻️ Duplicate comments (1)

test/integration/qe_tests/lvms.go (1)

5046-5049: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use a unique namespace name to avoid test collisions.

testNs := "lvms-test-88799" is static and can collide across reruns/parallel jobs, causing flaky AlreadyExists failures.

Suggested diff

-		testNs := "lvms-test-88799"
+		testNs := fmt.Sprintf("lvms-test-88799-%d", time.Now().UnixNano())

🤖 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 `@test/integration/qe_tests/lvms.go` around lines 5046 - 5049, The test uses a
static namespace name via the testNs variable which can cause AlreadyExists
collisions; change the setup to generate a unique namespace name for each run
(e.g., append a timestamp/UUID/random suffix) where testNs is assigned before
calling createNamespaceWithOC, so createNamespaceWithOC(testNs) and the deferred
deleteSpecifiedResource("namespace", testNs, "") use that generated unique name.

🤖 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.

Duplicate comments:
In `@test/integration/qe_tests/lvms.go`:
- Around line 5046-5049: The test uses a static namespace name via the testNs
variable which can cause AlreadyExists collisions; change the setup to generate
a unique namespace name for each run (e.g., append a timestamp/UUID/random
suffix) where testNs is assigned before calling createNamespaceWithOC, so
createNamespaceWithOC(testNs) and the deferred
deleteSpecifiedResource("namespace", testNs, "") use that generated unique name.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: cebbfdcf-452e-4cbb-ac6e-d55977a87ecf

📥 Commits

Reviewing files that changed from the base of the PR and between 8f23d88 and e7e39ac.

📒 Files selected for processing (3)

• test/integration/qe_tests/lvms.go
• test/integration/qe_tests/lvms_utils.go
• test/integration/qe_tests/testdata/storageclass-template.yaml

[pacevedom]

/retest

[pacevedom]

/override ci/prow/snyk-deps

[openshift-ci-robot]

@mmakwana30: This pull request explicitly references no jira issue.

Details

In response to this:

Jira: https://redhat.atlassian.net/browse/OCPEDGE-2612

Logs: https://privatebin.corp.redhat.com/?f3ebe41a5a3d0fdc#BfSzHS9X6eCKwBC8EsZ17D8Bv29RestR6KEK9UwtjSAF

Summary by CodeRabbit

• Tests

• Added integration tests validating LVMS behavior for combinations of reclaimPolicy and volumeBindingMode. Covers immediate-binding with automatic reclamation and delayed binding (WaitForFirstConsumer) with retained/manual reclamation; exercises PVC/pod lifecycles, verifies PV bind/unbind states, and confirms cleanup of retained volumes.

• Chores

• Added a parameterized StorageClass template and test utilities to create StorageClasses used by the new test scenarios.

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]

@pacevedom: Overrode contexts on behalf of pacevedom: ci/prow/snyk-deps

Details

In response to this:

/override ci/prow/snyk-deps

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.

[Neilhamza]

/lgtm

[pacevedom]

/retitle OCPEDGE-2612: Automate OCP-88799-Verify reclaimPolicy and volumeBindingMode combinations

[openshift-ci-robot]

@mmakwana30: This pull request references OCPEDGE-2612 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 "5.0.0" version, but no target version was set.

Details

In response to this:

Jira: https://redhat.atlassian.net/browse/OCPEDGE-2612

Logs: https://privatebin.corp.redhat.com/?f3ebe41a5a3d0fdc#BfSzHS9X6eCKwBC8EsZ17D8Bv29RestR6KEK9UwtjSAF

Summary by CodeRabbit

• Tests

• Added integration tests validating LVMS behavior for combinations of reclaimPolicy and volumeBindingMode. Covers immediate-binding with automatic reclamation and delayed binding (WaitForFirstConsumer) with retained/manual reclamation; exercises PVC/pod lifecycles, verifies PV bind/unbind states, and confirms cleanup of retained volumes.

• Chores

• Added a parameterized StorageClass template and test utilities to create StorageClasses used by the new test scenarios.

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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mmakwana30, pacevedom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [pacevedom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pacevedom]

/retest

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD 4899c37 and 2 for PR HEAD 1b98235 in total

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD a7db9bd and 1 for PR HEAD 1b98235 in total

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD 46a0d55 and 0 for PR HEAD 1b98235 in total

[openshift-merge-bot]

/hold

Revision 1b98235 was retested 3 times: holding

[pacevedom]

/test e2e-aws-hypershift

[kasturinarra]

/retest

[pacevedom]

/retest

[pacevedom]

/hold cancel

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD 0ce5bec and 2 for PR HEAD 1b98235 in total

[openshift-ci]

@mmakwana30: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/images	1b98235	link	unknown	/test images
ci/prow/unit-test	1b98235	link	unknown	/test unit-test
ci/prow/ci-index-lvm-operator-bundle	1b98235	link	unknown	/test ci-index-lvm-operator-bundle
ci/prow/nightly-images	1b98235	link	unknown	/test nightly-images

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD 32df2a1 and 1 for PR HEAD 1b98235 in total

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD e7a7ab3 and 0 for PR HEAD 1b98235 in total

[openshift-ci]

PR needs rebase.

Details

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.