Skip to content

Conversation

@skogta
Copy link
Contributor

@skogta skogta commented Dec 5, 2025

What this PR does / why we need it:

It is possible that after VM is deleted and batchattach spec has a deletionTimestamp, someone removes one or more of the volumes from batchattach spec.

This means CSI will never be able to remove finalizer from such volumes as CSI no longer has track record of which volumes were removed from the spec and which were attached to the VM.

There are 2 cases which need to be handled:

  1. VM itself is gone from the VC - in this case CSI should consider volumes present in batchattach status also while removing PVC finalizers.
  2. VM is not deleted yet, only batchattach spec has a deletion timestamp. In this case, CSI should detach all volumes attached to the VM on the vCenter and not only the ones mentioned in batchattach spec.

Testing done:

Detached 1 volume from batchattach CR - detach was succesful.

Deleted the batchattach CR, volume was detached and CR was deleted:

{"level":"info","time":"2025-12-05T07:12:00.713965369Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_controller.go:354","msg":"Deletion timestamp observed on instance test/test-new-2. Detaching all volumes.","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:00.713990037Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_controller.go:450","msg":"Detach call started for PVC pv-no-encrypt with volumeID d3171b93-a492-49f8-b39b-ae489a99aac9 in namespace test for instance test-new-2","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.584059742Z","caller":"volume/manager.go:1313","msg":"DetachVolume: Volume detached successfully. volumeID: \"d3171b93-a492-49f8-b39b-ae489a99aac9\", vm: \"VirtualMachine:vm-1002 [VirtualCenterHost: lvn-dvm-10-161-106-130.dvm.lvn.broadcom.net, UUID: 8b4e6ad5-b2b1-47bc-ad7f-ba9b0403735f, Datacenter: Datacenter [Datacenter: Datacenter:datacenter-3, VirtualCenterHost: lvn-dvm-10-161-106-130.dvm.lvn.broadcom.net]]\", opId: \"fb37ea79\"","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.584100759Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:909","msg":"Acquired lock for PVC test/pv-no-encrypt","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.584129943Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:776","msg":"Removing annotation cns.vmware.com/usedby-vm-8b4e6ad5-b2b1-47bc-ad7f-ba9b0403735f from PVC pv-no-encrypt","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.584153639Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:798","msg":"Patching PVC pv-no-encrypt with updated annotation","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.643335362Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:940","msg":"Successfully updated annotations on PVC pv-no-encrypt for VM 8b4e6ad5-b2b1-47bc-ad7f-ba9b0403735f","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.654608067Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:832","msg":"PVC pv-no-encrypt does not contain any annotations with prefix cns.vmware.com/usedby-vm-","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.654683699Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:956","msg":"VM 8b4e6ad5-b2b1-47bc-ad7f-ba9b0403735f was the last attached VM for the PVC pv-no-encrypt. Finalizer cns.vmware.com/pvc-protection can be safely removed from the PVC","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.654697861Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:969","msg":"Removing cns.vmware.com/pvc-protection finalizer from PVC: pv-no-encrypt on namespace: test","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.713667823Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:982","msg":"Successfully removed finalizer cns.vmware.com/pvc-protection from PVC pv-no-encrypt","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.714254864Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:912","msg":"Released lock for PVC test/pv-no-encrypt","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.714713921Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_controller.go:498","msg":"Successfully detached volume pv-no-encrypt from VM 8b4e6ad5-b2b1-47bc-ad7f-ba9b0403735f","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.71486518Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_controller.go:474","msg":"Detach call ended for PVC pv-no-encrypt in namespace test for instance test-new-2","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}
{"level":"info","time":"2025-12-05T07:12:01.715030952Z","caller":"cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go:77","msg":"Removing \"cns.vmware.com\" finalizer from CnsNodeVMBatchAttachment instance with name: \"test-new-2\" on namespace: \"test\"","TraceId":"fe1e9e45-70d7-40d3-9e18-142d97dfce35"}

WCP precheckin: https://jenkins-vcf-csifvt.devops.broadcom.net/view/instapp/job/wcp-instapp-e2e-pre-checkin/706/
VKS precheckin: https://jenkins-vcf-csifvt.devops.broadcom.net/job/vks-instapp-e2e-pre-checkin/649/

@skogta skogta marked this pull request as draft December 5, 2025 06:42
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot requested a review from gnufied December 5, 2025 06:42
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2025
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 5, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @skogta. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 5, 2025
@skogta skogta force-pushed the topic/skogta/alldisksfix branch 3 times, most recently from 7c0cd82 to 639ea5d Compare December 5, 2025 07:58
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2025
@skogta skogta marked this pull request as ready for review December 5, 2025 08:03
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2025
@k8s-ci-robot k8s-ci-robot requested a review from kolluria December 5, 2025 08:03
@deepakkinni
Copy link
Collaborator

Triggering CSI-WCP Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete
Jenkins Build #706

@deepakkinni
Copy link
Collaborator

Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete
Jenkins Build #649

@vdkotkar
Copy link
Contributor

vdkotkar commented Dec 5, 2025

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 5, 2025
@skogta skogta force-pushed the topic/skogta/alldisksfix branch from 639ea5d to b7bcf00 Compare December 8, 2025 07:17
@deepakkinni
Copy link
Collaborator

FAILED --- Jenkins Build #715

@deepakkinni
Copy link
Collaborator

Triggering CSI-TKG Pre-checkin Pipeline for this PR... Job takes approximately an hour to complete
Jenkins Build #663

@divyenpatel
Copy link
Member

@skogta Can we reject spec change on cnsnodevmbatchattachment instance, if it has deletion timestamp.
removal of volumes from batchattach spec should not allowed once instance has deletion timestamp.

Can we have webhook check for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants