OCPBUGS-98984: Implement opt in for reclaim disk#641
Conversation
WalkthroughThe change makes symlink mismatch detection account for missing on-disk links, ensures recreation directories exist, updates PV recovery checks, wires the path through reconcilers, and adds unit and end-to-end restoration coverage. ChangesSymlink restoration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant WipeJob
participant NodeFilesystem
participant LVDL
E2ETest->>WipeJob: wipe /mnt/local-storage symlinks
WipeJob->>NodeFilesystem: remove host symlinks
LVDL->>NodeFilesystem: restore preferred target
E2ETest->>NodeFilesystem: verify restored symlink
E2ETest->>LVDL: verify target convergence
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@pkg/common/current_block_device_info.go`:
- Around line 95-98: Update the symlink handling around internal.Readlink so
recovery proceeds only when the path is missing (os.IsNotExist(err)) or Readlink
succeeds with an alternate target; return a wrapped error for permission, I/O,
non-symlink, and other failures. Preserve the existing error when the target
equals currentLinkTarget, and do not ignore any Readlink error.
In `@pkg/common/device_link_handler.go`:
- Around line 467-472: The symlink validation around os.Lstat in the device-link
handler ignores non-ENOENT failures. Update this block to return the existing
error type or propagate the Lstat error when symlinkPath is set and the call
fails for any reason other than os.IsNotExist(err), while preserving the current
missing-symlink log and true result for ENOENT.
In `@pkg/common/provisioner_utils.go`:
- Line 129: Move the existing-PV lookup and VolumeReleased check before the
HasMismatchingSymlink/RecreateSymlinkIfNeeded recovery flow in the relevant
provisioner utility function. Ensure released volumes return ErrTryAgain without
recreating the directory or symlink or updating LVDL status, while non-released
volumes retain the current recovery behavior.
In `@test/e2e/symlink_check.go`:
- Around line 439-444: Add a descriptive failure message to the Job completion
assertion in the Eventually chain, including the check job, node, and path
details available in the surrounding test context. Keep the existing completion
condition and polling behavior unchanged.
- Around line 370-399: Update newWipeLocalStorageJob to remove the “|| true”
suppression from the symlink deletion command so find failures cause the job to
fail. Before running the wipe, assert that the target PV symlink exists, using
the test’s existing PV-link identification or validation mechanism, while
preserving the current job flow and failure propagation.
🪄 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: 8d21efd3-85f6-4885-a7fd-c0a0b305ffc9
📒 Files selected for processing (10)
pkg/common/current_block_device_info.gopkg/common/device_link_handler.gopkg/common/device_link_handler_test.gopkg/common/provisioner_utils.gopkg/common/pv_link_cache_test.gopkg/diskmaker/controllers/lv/reconcile.gopkg/diskmaker/controllers/lvset/reconcile.gotest/e2e/localvolume_test.gotest/e2e/localvolumeset_test.gotest/e2e/symlink_check.go
gnufied
force-pushed
the
implement-opt-in-for-reclaim-disk
branch
from
dec0980 to
c3fff76
Compare
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/diskmaker/controllers/lv/create_pv_test.go (1)
663-676: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unused filesystem stubs.
SyncPVAndLVDLreturns on the Released-PV path beforeHasMismatchingSymlink/RecreateSymlinkIfNeeded, soFilePathGlobandFilePathEvalSymlinksnever affect this test case.🤖 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 `@pkg/diskmaker/controllers/lv/create_pv_test.go` around lines 663 - 676, Remove the unused FilePathGlob and FilePathEvalSymLinks stubs from the diskmakertest.WithInternalMocks block in the SyncPVAndLVDL test, along with any now-unneeded imports or setup they require; preserve the test’s Released-PV behavior.
🤖 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.
Nitpick comments:
In `@pkg/diskmaker/controllers/lv/create_pv_test.go`:
- Around line 663-676: Remove the unused FilePathGlob and FilePathEvalSymLinks
stubs from the diskmakertest.WithInternalMocks block in the SyncPVAndLVDL test,
along with any now-unneeded imports or setup they require; preserve the test’s
Released-PV behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3627e0d5-404b-4bbb-9156-ce8f994fa5ec
📒 Files selected for processing (9)
pkg/common/current_block_device_info.gopkg/common/device_link_handler.gopkg/common/device_link_handler_test.gopkg/common/provisioner_utils.gopkg/common/pv_link_cache_test.gopkg/diskmaker/controllers/lv/create_pv_test.gopkg/diskmaker/controllers/lv/reconcile.gopkg/diskmaker/controllers/lvset/reconcile.gotest/e2e/symlink_check.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/diskmaker/controllers/lv/reconcile.go
- pkg/diskmaker/controllers/lvset/reconcile.go
- pkg/common/device_link_handler_test.go
- pkg/common/provisioner_utils.go
- pkg/common/current_block_device_info.go
- pkg/common/device_link_handler.go
- test/e2e/symlink_check.go
|
@gnufied: 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. |
gnufied
changed the title
openshift-ci-robot
added
jira/severity-important
|
@gnufied: This pull request references Jira Issue OCPBUGS-98984, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@gnufied: This pull request references Jira Issue OCPBUGS-98984, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@gnufied: This pull request references Jira Issue OCPBUGS-98984, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
2 participants
Fixes https://redhat.atlassian.net/browse/OCPBUGS-98984
Summary by CodeRabbit
Bug Fixes
Tests