OCPBUGS-78953: Fix bug with PV not getting provisioned after deletion#595
OCPBUGS-78953: Fix bug with PV not getting provisioned after deletion#595gnufied wants to merge 3 commits intoopenshift:mainfrom
Conversation
if preferredSymlink changes
gnufied
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a shared symlink lookup helper to pkg/common, updates disk-maker controllers to use it, and extends e2e test tooling to manipulate udev symlinks on nodes via Kubernetes Jobs while simplifying job node-handling to accept hostnames. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
|
[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
gnufied
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@gnufied: This pull request references Jira Issue OCPBUGS-78953, 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@gnufied: This pull request references Jira Issue OCPBUGS-78953, 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/e2e/hostudev.go`:
- Around line 80-100: The ls command in newRemoveUdevSymlinkJobs' script will
cause the job to exit when PATTERN matches nothing because set -e is enabled;
make the script resilient by preventing ls from failing (e.g., change the ls
line to a tolerant form such as running ls -la "$PATTERN" || true or temporarily
disable errexit around that command) so the job continues to the rm step; update
the script string in newRemoveUdevSymlinkJobs (refer to the local variable
script and the NodeJobOptions that injects PATTERN) accordingly.
In `@test/e2e/localvolume_test.go`:
- Around line 167-174: The bug is that currentSymlink is left unset when
selectedDisk.id is empty and the code incorrectly assigns expectedPath to
filepath.Join("/dev", "name"); change that branch so currentSymlink is assigned
filepath.Join("/dev", selectedDisk.name) (use selectedDisk.name, not the literal
"name"), leaving expectedPath untouched, so downstream call addNewUdevSymlink
uses a correct currentSymlink value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b8a6088-bc61-475f-b702-21e00021b57a
📒 Files selected for processing (8)
pkg/common/symlink_utils.gopkg/diskmaker/controllers/lv/reconcile.gopkg/diskmaker/controllers/lvset/reconcile.gotest/e2e/cleanup_hostdirs.gotest/e2e/hostudev.gotest/e2e/jobs.gotest/e2e/localvolume_test.gotest/e2e/symlink_check.go
gnufied
force-pushed
the
fix-pv-deletion-recreation-bug
branch
from
9aac475 to
94edc48
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/localvolume_test.go (1)
167-174:⚠️ Potential issue | 🔴 CriticalBug:
currentSymlinkuses literal"name"instead ofselectedDisk.name.Line 172 uses the string literal
"name"instead ofselectedDisk.name, resulting in an incorrect path (/dev/name) whenselectedDisk.idis empty.Proposed fix
if selectedDisk.id != "" { currentSymlink = filepath.Join("/dev/disk/by-id", selectedDisk.id) } else { - currentSymlink = filepath.Join("/dev", "name") + currentSymlink = filepath.Join("/dev", selectedDisk.name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/localvolume_test.go` around lines 167 - 174, The test builds currentSymlink incorrectly using the literal string "name" when selectedDisk.id is empty; update the conditional that sets currentSymlink to use selectedDisk.name (i.e., filepath.Join("/dev", selectedDisk.name)) instead of the hardcoded "name" so the path reflects the disk's actual name; keep the surrounding logic that only sets currentSymlink when len(pvs) > 0 and still prefer selectedDisk.id when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/localvolume_test.go`:
- Around line 167-174: The test builds currentSymlink incorrectly using the
literal string "name" when selectedDisk.id is empty; update the conditional that
sets currentSymlink to use selectedDisk.name (i.e., filepath.Join("/dev",
selectedDisk.name)) instead of the hardcoded "name" so the path reflects the
disk's actual name; keep the surrounding logic that only sets currentSymlink
when len(pvs) > 0 and still prefer selectedDisk.id when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 574583b4-dcfb-4820-8542-81b959c940e1
📒 Files selected for processing (8)
pkg/common/symlink_utils.gopkg/diskmaker/controllers/lv/reconcile.gopkg/diskmaker/controllers/lvset/reconcile.gotest/e2e/cleanup_hostdirs.gotest/e2e/hostudev.gotest/e2e/jobs.gotest/e2e/localvolume_test.gotest/e2e/symlink_check.go
| } | ||
| } | ||
| newPreferredTarget := "/dev/disk/by-id/scsi-1-local-storage-e2e-test" | ||
| addNewUdevSymlink(t, ctx, nodeHostName, currentSymlink, newPreferredTarget) |
There was a problem hiding this comment.
Is there an easy way how to test that the newly created PV many lines below actually used newPreferredTarget?
There was a problem hiding this comment.
The idea isn't here that the newly created will use new preferredSymlink, it won't. The problem and bug was - no new PV was getting provisioned at all.
After my fix - the recreated PV will come up with same symlink it was using before. We aren't changing symlinks in this bug fix.
|
/retest |
|
@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. |
3 participants
if preferredSymlink changes then PV doesn't get re-provisioned on deletion.
https://redhat.atlassian.net/browse/OCPBUGS-78953