Use optimistic merge for paravirt volumes patch

akutz commented 3 weeks ago

What this PR does / why we need it:

This patch fixes a possible race condition that occurs when using patch to update the volumes list in a VM Op VirtualMachine. It is possible with a vanilla patch that in between calculating and applying the patch the original object changed.

CRDs cannot use the strategic merge strategy, so instead this fix switches to an optimistic merge, which acts like update and refuses to make changes if the object has changed, but unlike update, does not drop fields.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged):

This fixes an internal bug blocking a vSphere Kubernetes Release. Please find us in Slack.

Testing done:

$ go test ./pkg/csi/service/wcpguest 
ok      sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/wcpguest  6.604s

Special notes for your reviewer:

Ideally we would use StrategicMergeFrom, but it does not support CRDs.

Release note:

Use optimistic merge when updating VirtualMachine volumes list to prevent overwriting list with stale data.

divyenpatel commented 3 weeks ago

/ok-to-test

divyenpatel commented 3 weeks ago

/lgtm

k8s-ci-robot commented 3 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akutz, bryanv, divyenpatel

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/vsphere-csi-driver/blob/master/OWNERS)~~ [divyenpatel] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

kubernetes-sigs / vsphere-csi-driver

Use optimistic merge for paravirt volumes patch #2925