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
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 |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe vSphere reconciler now classifies attached volumes by ChangesvSphere 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, 3 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
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
8eb49f2 to
54b5eea
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
…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 deletion for csi.vsphere.vmware.com (vSphere CSI) and kubernetes.io/vsphere-volume (vSphere in-tree), whitelisting known-safe non-vSphere CSI attachers (NFS, iSCSI, Cinder, EBS, GCE PD, Azure Disk) instead of a blanket default. Empty or unrecognized attachers, and volumes whose VolumeAttachment lookup fails, are conservatively treated as vSphere-backed and continue to block deletion. Adds unit test coverage (TestDeleteWithVolumeTypeFiltering) exercising NFS, vSphere CSI, vSphere in-tree, mixed, and conservative-blocking scenarios against a simulated vCenter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
54b5eea to
2a06443
Compare
|
/test ? |
|
/test e2e-vsphere-ovn-multi-vcenter |
| return va, nil | ||
| } | ||
|
|
||
| fieldSelector, err := fields.ParseSelector("spec.nodeName=" + nodeName) |
There was a problem hiding this comment.
The node selector specified this way will not work for VolumeAttachment objects. For this to work, the k8s API server has to explicitly register spec.nodeName as a selectable. You will have to list all VA objects and filter.
| // csi-<sha256(volumeHandle+driver+nodeName)>). | ||
| func (r *Reconciler) getVolumeAttachmentForAttachedVolume(ctx context.Context, volName, nodeName, machineName string) (*storagev1.VolumeAttachment, error) { | ||
| va := &storagev1.VolumeAttachment{} | ||
| if err := r.apiReader.Get(ctx, apimachinerytypes.NamespacedName{Name: volName}, va); err != nil { |
There was a problem hiding this comment.
This will never work. VA names are hashes, not the name in node.VolumesAttached
…t lookup The previous VolumeAttachment-based approach had two fundamental issues: - VolumeAttachment names are SHA256 hashes, not the AttachedVolume.Name - spec.nodeName field selector is not registered for VolumeAttachment Instead, parse the driver name directly from the UniqueVolumeName format in node.Status.VolumesAttached (kubernetes.io/csi/<driver>^<handle> for CSI, kubernetes.io/vsphere-volume/<path> for in-tree). This eliminates VolumeAttachment API calls entirely while correctly classifying volumes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/test e2e-vsphere-ovn-multi-vcenter |
|
/lgtm |
3 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