STOR-2841: add LSO localvolume fsType tests by radeore · Pull Request #632 · openshift/local-storage-operator

radeore · 2026-06-17T18:31:05Z

Summary

Adds fsType/volumeMode subtests (from PR #574) and restructures the LocalVolume E2E so fsType tests and the existing LVDL flow run on separate disks without interfering.

Add localvolume fsTypes tests.
Switch to use gp3 volume type to reduce storage costs(xref: https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-plan-storage-compare-volume-types.html)
Clean up unused vars cleanupRetryInterval and cleanupTimeout

$ go test -timeout 0 ./test/e2e/... -root=/go/src/github.com/openshift/local-storage-operator -kubeconfig=/tmp/kubeconfig -v -parallel=1
...
--- PASS: TestLocalStorage (1974.10s)
    --- PASS: TestLocalStorage/LocalVolumeDiscovery (30.52s)
    --- PASS: TestLocalStorage/LocalVolumeSet (907.87s)
    --- PASS: TestLocalStorage/LocalVolume (910.23s)
        --- PASS: TestLocalStorage/LocalVolume/default-filesystem-with-tolerations (93.38s)
        --- PASS: TestLocalStorage/LocalVolume/ext4-filesystem (64.80s)
        --- PASS: TestLocalStorage/LocalVolume/xfs-filesystem (55.87s)
        --- PASS: TestLocalStorage/LocalVolume/block-mode (52.70s)
PASS
ok  	github.com/openshift/local-storage-operator/test/e2e	1974.123s

Summary by CodeRabbit

Tests
- Updated AWS EBS volume type configuration in test scenarios for improved compatibility.
- Expanded test coverage for local storage supporting multiple filesystem types and volume modes.
- Refined deployment timeout settings for improved test stability.

openshift-ci-robot · 2026-06-17T18:31:09Z

@radeore: This pull request references STOR-2841 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 sub-task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

Adds fsType/volumeMode subtests (from PR #574) and restructures the LocalVolume E2E so fsType tests and the existing LVDL flow run on separate disks without interfering. LVDL coverage is preserved from main; execution order and LVDL disk (15 GiB, index 4) changed.

Add localvolume fsTypes tests.

Switch to use gp3 volume type to reduce storage costs(xref: https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-plan-storage-compare-volume-types.html)

Clean up unused vars cleanupRetryInterval and cleanupTimeout
./hack/test-e2e.sh --suite LocalVolume
--- PASS: TestLocalStorage (978.54s)
   --- PASS: TestLocalStorage/LocalVolume (906.36s)
       --- PASS: TestLocalStorage/LocalVolume/default-filesystem-with-tolerations (88.66s)
       --- PASS: TestLocalStorage/LocalVolume/ext4-filesystem (68.98s)
       --- PASS: TestLocalStorage/LocalVolume/xfs-filesystem (60.93s)
       --- PASS: TestLocalStorage/LocalVolume/block-mode (52.89s)
PASS
ok  	github.com/openshift/local-storage-operator/test/e2e	978.557s

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.

coderabbitai · 2026-06-17T18:31:22Z

Walkthrough

E2E tests for LocalVolume are expanded with a table-driven subtest suite covering default filesystem, ext4, xfs, and block PersistentVolumeMode configurations. Helper constructors getLocalVolume and getLocalVolumeWithFSType are refactored to support parameterized fsType and tolerations. AWS EBS provisioning switches from gp2 to gp3, and the e2e timeout variable declaration is cleaned up.

Changes

E2E LocalVolume fsType/volumeMode test expansion

Layer / File(s)	Summary
Test infrastructure: disk provisioning, disk index, and timeout `test/e2e/aws_disks.go`, `test/e2e/main_test.go`, `test/e2e/localvolume_test.go`	AWS EBS volume type updated from `gp2` to `gp3`; `lvdlDiskIndex` variable declared; per-node disk environment expanded to allocate one disk per fsType subtest plus a dedicated LVDL disk; stale first-disk assertion removed; `timeout` variable declaration cleaned up.
LocalVolume helper constructor refactoring `test/e2e/localvolume_test.go`	`getLocalVolume` delegates to `getLocalVolumeWithFSType` with explicit defaults. `getLocalVolumeWithFSType` now propagates passed-in tolerations and builds `StorageClassDevices` from the computed `scd`.
Table-driven fsType/volumeMode subtests and LVDL handoff `test/e2e/localvolume_test.go`	New table-driven loop over default filesystem, ext4, xfs, and block entries; each subtest creates a `LocalVolume` CR, verifies PV path/fsType/volumeMode, checks StorageClass and provisioner annotations, forces PV deletion/recreation, consumes and releases PVs with filesystem-UUID verification, and conditionally tests finalizer and symlink-cleanup behavior. LVDL handoff selects the dedicated disk and asserts a non-empty device path. Commented sleep removed; finalizer release-PV block tightened.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 3 warnings)

Check name	Status	Explanation	Resolution
Container-Privileges	❌ Error	PR adds container/K8s manifests with privileged: true and hostPID in diskmaker daemonsets, which the check flags as security concerns despite operational justification.	Verify these privilege escalations are necessary for storage operator functionality, or add securityContext justification comments in manifests.
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% 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 structure violates single responsibility principle. Each subtest (lines 254-441) tests 15+ behaviors including CR creation, PV lifecycle, PV consumption, finalizer behavior, and symlink verifi...	Split subtests by behavioral area: one for PV lifecycle/fsType validation, one for consumption/recreation, one for finalizers. Add meaningful messages to assertions: `Expect(...).To(..., "expected PV path to match disk")`.
Single Node Openshift (Sno) Test Compatibility	⚠️ Warning	The LocalVolumeTest function requires >= 3 worker nodes (line 58-60) and explicitly tests multi-node scenarios (e.g., line 121-188: "expanding shared localvolume reproducer to a second node"), whic...	Add SNO skip protection: either add `[Skipped:SingleReplicaTopology]` label to test name, or guard test with `skipOnSingleNodeTopology()` or `infrastructure.Status.ControlPlaneTopology == configv1.SingleReplicaTopologyMode` check.

✅ Passed checks (11 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main addition to the PR: implementing fsType tests for LocalVolume E2E tests, which is the primary change across the modified files.
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	All test names in the PR are stable and deterministic: "LocalVolumeDiscovery", "LocalVolumeSet", "LocalVolume", "default-filesystem-with-tolerations", "ext4-filesystem", "xfs-filesystem", and "bloc...
Microshift Test Compatibility	✅ Passed	New table-driven subtests added to LocalVolumeTest (lines 254-280+) use local.storage.openshift.io v1 CRD and operatorv1 conditions, neither of which are explicitly listed as unavailable on MicroSh...
Topology-Aware Scheduling Compatibility	✅ Passed	PR modifies only E2E test files (aws_disks.go, localvolume_test.go, main_test.go) with no deployment manifests, operator code, or scheduling constraints introduced.
Ote Binary Stdout Contract	✅ Passed	No process-level stdout violations found. The fmt.Println call is inside TestLocalStorage (a test function) within a signal-handling goroutine, which the spec explicitly excludes. All top-level var...
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo e2e tests are added in this PR. Tests use Go standard testing with t.Run() subtests, not Ginkgo (It/Describe/etc). Custom check applies only to Ginkgo e2e tests.
No-Weak-Crypto	✅ Passed	No weak crypto patterns (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto implementations, or non-constant-time secret comparisons found in modified test files.
No-Sensitive-Data-In-Logs	✅ Passed	No sensitive data logging detected. AWS credentials properly isolated in Kubernetes secrets, passed to SDK config only, never logged. Error messages use generic strings. Logging limited to test res...
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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

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

openshift-ci · 2026-06-17T18:31:36Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: radeore
Once this PR has been reviewed and has the lgtm label, please assign rhrmo for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

openshift-ci · 2026-06-17T20:30:35Z

@radeore: all tests passed!

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.

gnufied · 2026-06-23T14:22:26Z

+		// Main LocalVolume LVDL / symlink reconciliation flow,
+		// on a dedicated disk after fsType subtests complete.
+		selectedDisk := nodeEnv[0].disks[lvdlDiskIndex]
+		matcher.Expect(selectedDisk.path).ShouldNot(gomega.BeZero(), "device path should not be empty")


So these tests validate whether setting fsType works for LocalVolume and some checks for SC and PV paths by the looks of it.

I am not sure if putting a big table of tests in the middle of the other tests is a good idea. This makes the code harder to understand and parse. Also since side effect from one test appears to flow into others, this is especially tricky.

The fsType cases are isolated by dedicated disk index + storage class, scsi-8 cleanup runs before them, and LVDL uses disk-4. How about refactoring test into three named helpers with top-level t.Run boundaries (shared-byid-reproducer, fstype subtests, lvdl-symlink-reconciliation) and move the case table out of the main flow so the test reads as explicit phases rather than one long script?

STOR-2841: add LSO localvolume fsType tests

41d7097

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 17, 2026

openshift-ci Bot requested review from dfajmon and jsafrane June 17, 2026 18:31

gnufied reviewed Jun 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

STOR-2841: add LSO localvolume fsType tests#632

STOR-2841: add LSO localvolume fsType tests#632
radeore wants to merge 1 commit into
openshift:mainfrom
radeore:lso-fstests

radeore commented Jun 17, 2026 •

edited

Loading

Uh oh!

openshift-ci-robot commented Jun 17, 2026 •

edited by openshift-ci Bot

Loading

Summary

Uh oh!

coderabbitai Bot commented Jun 17, 2026 •

edited

Loading

Pre-merge checks failed

Uh oh!

openshift-ci Bot commented Jun 17, 2026

Uh oh!

openshift-ci Bot commented Jun 17, 2026

Uh oh!

gnufied Jun 23, 2026

Uh oh!

radeore Jun 23, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

radeore commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by CodeRabbit

Uh oh!

openshift-ci-robot commented Jun 17, 2026 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

coderabbitai Bot commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks failed

❌ Failed checks (1 error, 3 warnings)

Uh oh!

openshift-ci Bot commented Jun 17, 2026

Uh oh!

openshift-ci Bot commented Jun 17, 2026

Uh oh!

gnufied Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

radeore Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

radeore commented Jun 17, 2026 •

edited

Loading

openshift-ci-robot commented Jun 17, 2026 •

edited by openshift-ci Bot

Loading

coderabbitai Bot commented Jun 17, 2026 •

edited

Loading

radeore Jun 23, 2026 •

edited

Loading