Skip to content

Conversation

@skogta
Copy link
Contributor

@skogta skogta commented Nov 21, 2025

What this PR does / why we need it:
After 9.1, all VM service VM volume attachments should happen via the new Batch Attach CRD.

This PR blocks any new attachments via the old CnsNodeVMAttachment CR if the shared disk FSS is enabled.

Testing done:

On a testbed with capability disabled, I attempted to attach a volume to a VM service VM. CnsNodeVMAttachment CR got created. I purposefully failed this attachment by manipulating the code:

Name:         testvm-upgrade-my-pvc
Namespace:    test
Labels:       <none>
Annotations:  <none>
API Version:  cns.vmware.com/v1alpha1
Kind:         CnsNodeVmAttachment
Metadata:
  Creation Timestamp:  2025-11-25T09:39:01Z
  Finalizers:
    cns.vmware.com
  Generation:  1
  Owner References:
    API Version:           vmoperator.vmware.com/v1alpha5
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  VirtualMachine
    Name:                  testvm-upgrade
    UID:                   6e24a83f-8df3-437a-ade0-06768b734fdd
  Resource Version:        7272705
  UID:                     a88582f4-c5cb-4d4a-88bb-58364b4b3b8d
Spec:
  Nodeuuid:    b87295ee-d8a2-4b5e-8759-13cfbb7d1112
  Volumename:  my-pvc
Status:
  Attached:  false
  Error:     failed temp
Events:
  Type     Reason              Age                From            Message
  ----     ------              ----               ----            -------
  Warning  NodeVMAttachFailed  27s (x6 over 58s)  cns.vmware.com  

I then enabled the capability and observed that the same CR got updated with the following message:

Name:         testvm-upgrade-my-pvc
Namespace:    test
Labels:       <none>
Annotations:  <none>
API Version:  cns.vmware.com/v1alpha1
Kind:         CnsNodeVmAttachment
Metadata:
  Creation Timestamp:  2025-11-25T09:39:01Z
  Finalizers:
    cns.vmware.com
  Generation:  1
  Owner References:
    API Version:           vmoperator.vmware.com/v1alpha5
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  VirtualMachine
    Name:                  testvm-upgrade
    UID:                   6e24a83f-8df3-437a-ade0-06768b734fdd
  Resource Version:        7274925
  UID:                     a88582f4-c5cb-4d4a-88bb-58364b4b3b8d
Spec:
  Nodeuuid:    b87295ee-d8a2-4b5e-8759-13cfbb7d1112
  Volumename:  my-pvc
Status:
  Attached:  false
  Error:     CnsNodeVMAttachment CR is deprecated. Please detach this PVC from the VM and then reattach it.
Events:
  Type     Reason              Age                    From            Message
  ----     ------              ----                   ----            -------
  Warning  NodeVMAttachFailed  2m48s (x8 over 4m55s)  cns.vmware.com  
  Warning  NodeVMAttachFailed  2m3s                   cns.vmware.com  CnsNodeVMAttachment CR is deprecated. Please detach this PVC from the VM and then reattach it.

Same error got propagated to VM also:

  Unique ID:  vm-2021
  Volumes:
    Error:      CnsNodeVMAttachment CR is deprecated. Please detach this PVC from the VM and then reattach it.
    Limit:      100Mi
    Name:       my-pvc
    Requested:  100Mi
    Type:       Managed
    Attached:   true

Next, I removed this volume from VM spec and observed that the old CnsNodeVmAttachment CR got deleted.

I then re-added the volume to the VM spec and observed that batchattach CR was created and it attach was successful also:

Name:         testvm-upgrade
Namespace:    test
Labels:       <none>
Annotations:  <none>
API Version:  cns.vmware.com/v1alpha1
Kind:         CnsNodeVMBatchAttachment
Metadata:
  Creation Timestamp:  2025-11-25T09:41:56Z
  Finalizers:
    cns.vmware.com
  Generation:  2
  Owner References:
    API Version:           vmoperator.vmware.com/v1alpha5
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  VirtualMachine
    Name:                  testvm-upgrade
    UID:                   6e24a83f-8df3-437a-ade0-06768b734fdd
  Resource Version:        7288948
  UID:                     512ae04f-53b0-4b87-a8bf-2232cd0a083d
Spec:
  Instance UUID:  a136f0bd-d942-4961-9174-5b001d22e440
  Volumes:
    Name:  my-pvc
    Persistent Volume Claim:
      Claim Name:      my-pvc
      Controller Key:  1001
      Disk Mode:       persistent
      Sharing Mode:    sharingNone
      Unit Number:     0
Status:
  Volumes:
    Name:  my-pvc
    Persistent Volume Claim:
      Attached:       true
      Claim Name:     my-pvc
      Cns Volume Id:  df9480f6-e501-4d5f-92ee-ddd622d32780
      Disk UUID:      6000C29e-697d-1716-fe86-c13aa22207a7
Events:
  Type     Reason                      Age                From            Message
  ----     ------                      ----               ----            -------
  Normal   NodeVmBatchAttachSucceeded  19m                cns.vmware.com  ReconcileCnsNodeVMBatchAttachment: Successfully processed instance test/testvm-upgrade in namespace "test/testvm-upgrade".
  Normal   NodeVmBatchAttachSucceeded  7s (x2 over 2m7s)  cns.vmware.com  ReconcileCnsNodeVMBatchAttachment: Successfully processed instance test/testvm-upgrade in namespace "test/testvm-upgrade".

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 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 Nov 21, 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 the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 21, 2025
@deepakkinni
Copy link
Collaborator

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

@deepakkinni
Copy link
Collaborator

SUCCESS --- Jenkins Build #607

@skogta skogta force-pushed the topic/skogta/upgradeAttachDisable branch from 5995cd7 to 1ad6def Compare November 24, 2025 06:18
@deepakkinni
Copy link
Collaborator

FAILED --- Jenkins Build #610

@deepakkinni
Copy link
Collaborator

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

@deepakkinni
Copy link
Collaborator

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

@skogta skogta force-pushed the topic/skogta/upgradeAttachDisable branch from 1ad6def to 71a4440 Compare November 25, 2025 10:03
@divyenpatel
Copy link
Member

/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 Nov 26, 2025
Copy link
Member

@divyenpatel divyenpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyenpatel, skogta

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

The pull request process is described 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2025
@k8s-ci-robot
Copy link
Contributor

@skogta: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-vsphere-csi-driver-verify-golangci-lint 71a4440 link true /test pull-vsphere-csi-driver-verify-golangci-lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@skogta skogta force-pushed the topic/skogta/upgradeAttachDisable branch from 71a4440 to 97a3313 Compare November 26, 2025 06:40
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2025
@divyenpatel
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2025
@divyenpatel
Copy link
Member

/test pull-vsphere-csi-driver-verify-golangci-lint

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants