OCPBUGS-101813: only block vSphere machine deletion for VMDK-backed v… - #1528
OCPBUGS-101813: only block vSphere machine deletion for VMDK-backed v…#1528jcpowermac wants to merge 2 commits into
Conversation
…olumes The nodeHasVolumesAttached() check was volume-type-agnostic, blocking machine deletion for any attached volume. This caused indefinite blocking when non-VMDK volumes (NFS, iSCSI, etc.) were attached, since DaemonSet pods with these volumes remain Running and never get evicted. The VMDK data loss risk only applies to vSphere-backed volumes. This change filters by VolumeAttachment.Spec.Attacher to only block for: - csi.vsphere.vmware.com (vSphere CSI) - kubernetes.io/vsphere-volume (vSphere in-tree) Non-vSphere volumes no longer block deletion. Conservative behavior: if VolumeAttachment lookup fails, treat as vSphere-backed and block. Includes unit tests for volume type filtering and e2e test for NFS volume deletion scenario.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/valid-reference
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-101813, 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughChangesThe vSphere reconciler classifies attached volumes by vSphere volume-aware machine deletion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant KubernetesAPI
participant vCenter
Reconciler->>KubernetesAPI: Look up VolumeAttachment objects
KubernetesAPI-->>Reconciler: Return attachers or lookup results
Reconciler->>Reconciler: Classify attached volumes
Reconciler->>vCenter: Delete VM when no vSphere volume blocks deletion
Reconciler-->>Reconciler: Requeue when a vSphere volume blocks deletion
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test ? |
|
/test e2e-vsphere-ovn-techpreview-serial |
|
/test unit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/controller/vsphere/reconciler_test.go`:
- Around line 2598-2771: Extend the volumeTypeFilteringTestCases table to cover
nodeHasVolumesAttached conservative fallbacks: add a case with a node-attached
volume but no matching VolumeAttachment object, and a case with a matching
attachment whose Attacher is empty or unrecognized. Configure both cases to
expect deletion blocking with the existing attached-volumes requeue error, and
retain the established machine/node setup and reconcile assertions.
In `@pkg/controller/vsphere/reconciler.go`:
- Around line 581-587: Update the VolumeAttachment attacher classification
around the switch in the reconciler so only explicitly recognized safe
non-vSphere attachers remain in nonVSphereVolumes; route unknown or empty
Attacher values into unknownVolumes, alongside failed lookups. Ensure the
existing unknownVolumes handling and logging are exercised, preserving vSphere
attachers in vsphereVolumes.
- Around line 567-587: The VolumeAttachment lookup in the node volume
classification loop must use the production metadata name for CSI attachments
rather than treating AttachedVolume.Name as the object name. Update the logic
around apiReader.Get and the surrounding reconciliation method to derive the
csi-<sha256(volumeHandle+driver+nodeName)> name, or list and correlate
VolumeAttachment objects by node and attached volume, so non-vSphere CSI volumes
are classified by their real attacher. Update the related tests to use
production-style hashed VolumeAttachment names.
In `@test/e2e/vsphere/machines.go`:
- Around line 282-354: The test must attach an NFS volume to the created
machine’s node before deletion. Extend the flow around testNSName,
nfsStorageClassName, and the machine readiness check to create a PVC and
mounting Pod or DaemonSet using that storage class, ensure it schedules onto
machine.Name’s node, and wait for the corresponding VolumeAttachment or
node.Status.VolumesAttached entry before deleting the machine. Keep the existing
deletion and cluster-size assertions unchanged.
🪄 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: c530423b-96ea-445c-9067-22760792a36c
📒 Files selected for processing (4)
ocpbugs-101813.mdpkg/controller/vsphere/reconciler.gopkg/controller/vsphere/reconciler_test.gotest/e2e/vsphere/machines.go
|
@jcpowermac: The following test failed, say
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. |
openshift-ci-robot
added
jira/valid-bug
|
@jcpowermac: This pull request references Jira Issue OCPBUGS-101813, 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/controller/vsphere/reconciler_test.go (2)
2887-2889: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse one deadline-bound context for simulator operations.
context.TODO()andcontext.Background()have no deadline. IfAddDeviceorreconciler.delete()stalls, this test can wait indefinitely. Create acontext.WithTimeoutcontext per subtest and pass it to both operations.As per path instructions, use
context.Contextfor cancellation and timeouts.Also applies to: 2913-2918
🤖 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/controller/vsphere/reconciler_test.go` around lines 2887 - 2889, Create one deadline-bound context with context.WithTimeout per subtest in the relevant test flow, and defer its cancellation. Reuse that context for addDiskToVm and the reconciler.delete operation, replacing context.TODO()/context.Background() so both simulator operations terminate when the deadline expires.Source: Path instructions
2880-2881: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease simulator resources after each subtest.
Each table entry creates a simulator model and server. The subtest does not call
srv.Close()ormodel.Remove(). Repeated cases can retain listeners and simulator resources until process exit.Proposed fix
model, sess, srv := initSimulator(t) +t.Cleanup(func() { + srv.Close() + model.Remove() +}) simParams, err := getVcenterSimParams(srv, namespace)🤖 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/controller/vsphere/reconciler_test.go` around lines 2880 - 2881, Update the table-driven subtest around initSimulator to release each simulator’s resources before the subtest exits: ensure srv.Close() and model.Remove() are called for every entry, including when the subtest fails, using cleanup or deferred teardown tied to the subtest lifecycle.test/e2e/vsphere/machines.go (1)
293-299: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSelect only attachers that the reconciler treats as non-vSphere.
This test treats
openshift-storage.noobaa.ioas an NFS provisioner.pkg/controller/vsphere/reconciler.gopermits onlynfs.csi.k8s.ioandcsi.nfs.ioin its non-vSphere branch. The NooBaa attacher reaches the unknown branch and blocks deletion.Remove
openshift-storage.noobaa.iofrom this selection, or add the same attacher to the reconciler policy and its unit coverage.🤖 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/e2e/vsphere/machines.go` around lines 293 - 299, Update the storage class selection in the test’s storageClasses loop to recognize only provisioners permitted by the reconciler’s non-vSphere branch: nfs.csi.k8s.io and csi.nfs.io. Remove openshift-storage.noobaa.io from the condition, unless instead you also update the reconciler policy and its unit coverage consistently.
♻️ Duplicate comments (1)
pkg/controller/vsphere/reconciler_test.go (1)
2598-2605: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the remaining vSphere attachment paths.
The table does not test
VSphereInTreePluginName, althoughnodeHasVolumesAttachedblocks this attacher.The name-matched fixtures make the first
Getsucceed. The missing-attachment case only tests an empty list result. Add cases that forceGetto returnNotFoundandListto return a matching attachment, and that force a lookup error. Assert the required deletion and blocking outcomes.🤖 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/controller/vsphere/reconciler_test.go` around lines 2598 - 2605, Extend the volumeTypeFilteringTestCases table and its test logic to cover VSphereInTreePluginName, including name-matched fixtures where Get returns NotFound, List returns a matching attachment, and lookup returns an error. Assert that the NotFound/list paths produce the required deletion behavior and that lookup errors preserve the expected blocking outcome, while keeping existing attacher cases unchanged.
🤖 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/e2e/vsphere/machines.go`:
- Around line 363-417: Reorder the flow so the Pod creation follows PVC creation
rather than waiting for ClaimBound first, allowing WaitForFirstConsumer
provisioning. Replace the generic node.Status.VolumesAttached length check with
waits that verify pvcName reaches ClaimBound and podName reaches Running on
nodeName; if validating reconciler correlation, additionally locate the PVC’s
VolumeAttachment and confirm its nodeName and expected attacher.
- Around line 372-407: Update the container image in the Pod created by the NFS
test setup around the pod creation call, replacing
registry.redhat.io/ubi8/ubi-minimal:latest with the repository’s configured or
cluster-provided test image, or a fixed digest. Keep the existing container
behavior unchanged while ensuring image resolution is deterministic and does not
depend on an unpinned external tag.
---
Outside diff comments:
In `@pkg/controller/vsphere/reconciler_test.go`:
- Around line 2887-2889: Create one deadline-bound context with
context.WithTimeout per subtest in the relevant test flow, and defer its
cancellation. Reuse that context for addDiskToVm and the reconciler.delete
operation, replacing context.TODO()/context.Background() so both simulator
operations terminate when the deadline expires.
- Around line 2880-2881: Update the table-driven subtest around initSimulator to
release each simulator’s resources before the subtest exits: ensure srv.Close()
and model.Remove() are called for every entry, including when the subtest fails,
using cleanup or deferred teardown tied to the subtest lifecycle.
In `@test/e2e/vsphere/machines.go`:
- Around line 293-299: Update the storage class selection in the test’s
storageClasses loop to recognize only provisioners permitted by the reconciler’s
non-vSphere branch: nfs.csi.k8s.io and csi.nfs.io. Remove
openshift-storage.noobaa.io from the condition, unless instead you also update
the reconciler policy and its unit coverage consistently.
---
Duplicate comments:
In `@pkg/controller/vsphere/reconciler_test.go`:
- Around line 2598-2605: Extend the volumeTypeFilteringTestCases table and its
test logic to cover VSphereInTreePluginName, including name-matched fixtures
where Get returns NotFound, List returns a matching attachment, and lookup
returns an error. Assert that the NotFound/list paths produce the required
deletion behavior and that lookup errors preserve the expected blocking outcome,
while keeping existing attacher cases unchanged.
🪄 Autofix
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: fc19e4b2-2cdd-432d-b0a9-6f58ccf03c43
📒 Files selected for processing (3)
pkg/controller/vsphere/reconciler.gopkg/controller/vsphere/reconciler_test.gotest/e2e/vsphere/machines.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/vsphere/reconciler.go
| By("creating Pod that mounts the NFS volume on the target node") | ||
| podName := "nfs-test-pod-" + machineName | ||
| _, err = c.CoreV1().Pods(testNSName).Create(ctx, &corev1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: podName, | ||
| Namespace: testNSName, | ||
| }, | ||
| Spec: corev1.PodSpec{ | ||
| NodeName: nodeName, | ||
| Containers: []corev1.Container{ | ||
| { | ||
| Name: "nfs-test", | ||
| Image: "registry.redhat.io/ubi8/ubi-minimal:latest", | ||
| Command: []string{ | ||
| "sh", "-c", "while true; do sleep 3600; done", | ||
| }, | ||
| VolumeMounts: []corev1.VolumeMount{ | ||
| { | ||
| Name: "nfs-volume", | ||
| MountPath: "/mnt/nfs", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Volumes: []corev1.Volume{ | ||
| { | ||
| Name: "nfs-volume", | ||
| VolumeSource: corev1.VolumeSource{ | ||
| PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ | ||
| ClaimName: pvcName, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, metav1.CreateOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, confirm the exact content at the specified lines
sed -n '372,407p' test/e2e/vsphere/machines.goRepository: openshift/machine-api-operator
Length of output: 1120
🏁 Script executed:
# Search for image-related constants or configuration in the test directory
rg "registry\.|ubi|image.*=" test/e2e/ -A 2 -B 2 | head -100Repository: openshift/machine-api-operator
Length of output: 483
🏁 Script executed:
# Look for test image setup or fixtures
fd -e go test/e2e/ -type f -exec rg -l "Image:|testImage|imageURL" {} \;Repository: openshift/machine-api-operator
Length of output: 401
🏁 Script executed:
# Search for any test configuration or setup that might define a default image
rg "testImage|TEST_IMAGE|ImageName" test/e2e/vsphere/ -A 2Repository: openshift/machine-api-operator
Length of output: 168
🏁 Script executed:
# Search for how other e2e tests handle pod images
rg "Image:" test/e2e/ -B 2 -A 0 | grep -v "ubi-minimal"Repository: openshift/machine-api-operator
Length of output: 258
🏁 Script executed:
# Search for test image constants or configuration files
rg -i "testimage|image.*constant|fixture" test/ --type go | head -20Repository: openshift/machine-api-operator
Length of output: 168
🏁 Script executed:
# Look for any centralized image configuration in the test directory
find test -name "*.go" -exec rg "const.*Image|var.*Image" {} + | head -20Repository: openshift/machine-api-operator
Length of output: 168
🏁 Script executed:
# Check for image pull or mirror configuration
rg "imagePull|registry|mirror" test/e2e/vsphere/machines.go -B 2 -A 2Repository: openshift/machine-api-operator
Length of output: 338
🏁 Script executed:
# Look at the broader context of the test function to understand test setup
sed -n '340,380p' test/e2e/vsphere/machines.goRepository: openshift/machine-api-operator
Length of output: 1696
🏁 Script executed:
# Search for any documentation about test image handling
find test -name "*.md" -o -name "README*" -o -name "*.txt" | xargs cat 2>/dev/null | grep -i image | head -20Repository: openshift/machine-api-operator
Length of output: 168
🏁 Script executed:
# Look for test setup files or helpers that might configure images
rg "BeforeSuite|SetupTest|Setup" test/e2e/vsphere/machines.go -B 2 -A 5Repository: openshift/machine-api-operator
Length of output: 168
🏁 Script executed:
# Check what the actual test function looks like to understand context
sed -n '300,340p' test/e2e/vsphere/machines.goRepository: openshift/machine-api-operator
Length of output: 1779
🏁 Script executed:
# Search for any container image registry configuration in the codebase
rg -i "containerimage|image.registry" . --type go -l | head -10Repository: openshift/machine-api-operator
Length of output: 723
🏁 Script executed:
# Look for how images are configured in CI/test environments
find . -name "Makefile" -o -name "*.sh" -o -name ".github" -type d | head -10Repository: openshift/machine-api-operator
Length of output: 921
🏁 Script executed:
# Look at the root Makefile for test execution patterns
head -100 MakefileRepository: openshift/machine-api-operator
Length of output: 3180
🏁 Script executed:
# Search for any test requirements or documentation
ls -la . | grep -i readme
cat README.md 2>/dev/null | head -50Repository: openshift/machine-api-operator
Length of output: 2300
🏁 Script executed:
# Check if there are any image pull policies or test fixtures defined
rg "ImagePullPolicy|image.pull" test/e2e/vsphere/ -B 2 -A 2Repository: openshift/machine-api-operator
Length of output: 168
🏁 Script executed:
# Look for any network or internet access restrictions in tests
rg -i "offline|network|internet|air.gap" test/e2e/ -lRepository: openshift/machine-api-operator
Length of output: 281
🏁 Script executed:
# Check how the test is actually invoked - look for test files
ls -la test/e2e/vsphere/Repository: openshift/machine-api-operator
Length of output: 600
🏁 Script executed:
# Check the e2e.sh script to understand test configuration
cat hack/e2e.shRepository: openshift/machine-api-operator
Length of output: 340
🏁 Script executed:
# Look for any test helper or util files that might configure images
cat test/e2e/vsphere/util.goRepository: openshift/machine-api-operator
Length of output: 10717
🏁 Script executed:
# Search for any image configuration in setup or initialization
rg "registry.redhat.io|ubi8|ubi-minimal" . --type go -B 3 -A 1 | head -50Repository: openshift/machine-api-operator
Length of output: 11956
🏁 Script executed:
# Verify if the coding guidelines referenced in the original comment actually exist
cat /dev/null # Just confirming - let me check what guidelines were providedRepository: openshift/machine-api-operator
Length of output: 168
🏁 Script executed:
# Check for any documentation about image pinning or test stability requirements
find . -name "*.md" -o -name "CONTRIBUTING*" | xargs grep -l "image\|pin\|digest" 2>/dev/null | head -5Repository: openshift/machine-api-operator
Length of output: 364
🏁 Script executed:
# Look for other test files to see if there's a pattern for how images should be handled
rg "Image:" test/ -B 3 -A 1 --type go | grep -E "(Image:|^test/)" | head -30Repository: openshift/machine-api-operator
Length of output: 465
Replace the mutable image tag with a pinned digest or configured test image.
The Pod uses registry.redhat.io/ubi8/ubi-minimal:latest, which has two issues: the :latest tag is mutable and may resolve to different image content on each test run, and the external registry dependency adds a network requirement to the test. Replace this with either a digest pin (e.g., registry.redhat.io/ubi8/ubi-minimal@sha256:...) or use a test image that the cluster provides or mirrors locally, ensuring deterministic and reliable test execution.
🤖 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/e2e/vsphere/machines.go` around lines 372 - 407, Update the container
image in the Pod created by the NFS test setup around the pod creation call,
replacing registry.redhat.io/ubi8/ubi-minimal:latest with the repository’s
configured or cluster-provided test image, or a fixed digest. Keep the existing
container behavior unchanged while ensuring image resolution is deterministic
and does not depend on an unpinned external tag.
Source: Coding guidelines
eedede3 to
bd366a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/vsphere/reconciler_test.go (1)
2455-2597: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated test helpers.
vCenterSimConfig,getVcenterSimParams,getMachineWithStatus,getNodeWithConditions, andaddDiskToVmare copies of the closures inTestDelete(lines 1935-2059 and 2223-2252). The two copies differ only ininstanceUUID. Two copies will drift when the simulator setup changes.Move these to package-level test helpers in this file, parameterize
instanceUUIDandnodeName, and call them from both tests.🤖 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/controller/vsphere/reconciler_test.go` around lines 2455 - 2597, Extract vCenterSimConfig and the helpers getVcenterSimParams, getMachineWithStatus, getNodeWithConditions, and addDiskToVm from TestDeleteWithVolumeTypeFiltering and TestDelete into package-level test helpers. Add instanceUUID and nodeName parameters where needed, preserving each test’s current values and behavior, then update both tests to call the shared helpers and remove their duplicated local closures.
🤖 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/controller/vsphere/reconciler_test.go`:
- Around line 2810-2837: Register the fake client's VolumeAttachment field index
for spec.nodeName in the test setup used by the reconciler cases, including the
index key expected by the fallback List in the missing-attachment scenario.
Ensure the existing test reaches the va == nil path instead of failing because
the field selector index is unavailable.
---
Nitpick comments:
In `@pkg/controller/vsphere/reconciler_test.go`:
- Around line 2455-2597: Extract vCenterSimConfig and the helpers
getVcenterSimParams, getMachineWithStatus, getNodeWithConditions, and
addDiskToVm from TestDeleteWithVolumeTypeFiltering and TestDelete into
package-level test helpers. Add instanceUUID and nodeName parameters where
needed, preserving each test’s current values and behavior, then update both
tests to call the shared helpers and remove their duplicated local closures.
🪄 Autofix
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: ad47fa12-ce18-438c-b167-f6e793884628
📒 Files selected for processing (3)
pkg/controller/vsphere/reconciler.gopkg/controller/vsphere/reconciler_test.gotest/e2e/vsphere/machines.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/vsphere/machines.go
- pkg/controller/vsphere/reconciler.go
- Whitelist known safe non-vSphere CSI attachers instead of blanket default - Route empty/unrecognized attachers to unknownVolumes, blocking deletion - Add VolumeAttachment fallback correlation via node-list for hashed CSI names - Add tests for missing VA, empty attacher, and unknown attacher cases - Update e2e test to actually attach NFS volume before machine deletion
bd366a4 to
8eb49f2
Compare
2 participants
…olumes
The nodeHasVolumesAttached() check was volume-type-agnostic, blocking machine deletion for any attached volume. This caused indefinite blocking when non-VMDK volumes (NFS, iSCSI, etc.) were attached, since DaemonSet pods with these volumes remain Running and never get evicted.
The VMDK data loss risk only applies to vSphere-backed volumes. This change filters by VolumeAttachment.Spec.Attacher to only block for:
Non-vSphere volumes no longer block deletion. Conservative behavior: if VolumeAttachment lookup fails, treat as vSphere-backed and block.
Includes unit tests for volume type filtering and e2e test for NFS volume deletion scenario.
Summary by CodeRabbit
Bug Fixes
Tests