fix: Dont expunge all attached data volumes on machine destroy

If csMachine.Spec.DiskOffering is nil, skip looking up data volumes attached to VM for expunging

If csMachine.Spec.DiskOffering is not nil, only add data volumes for expunging that are named 'DATA-<something>'

Author: hrak

pkg/cloud/instance.go

@@ -427,12 +427,15 @@ func findVirtualMachine(
 func (c *client) DestroyVMInstance(csMachine *infrav1.CloudStackMachine) error {
 	// Attempt deletion regardless of machine state.
 	p := c.csAsync.VirtualMachine.NewDestroyVirtualMachineParams(*csMachine.Spec.InstanceID)
-	volIDs, err := c.listVMInstanceDatadiskVolumeIDs(*csMachine.Spec.InstanceID)
-	if err != nil {
-		return err
+	// If an additional data disk was requested on creation of this machine, find it and expunge it as well.
+	if csMachine.Spec.DiskOffering != nil {
+		volIDs, err := c.listVMInstanceDatadiskVolumeIDs(*csMachine.Spec.InstanceID)
+		if err != nil {
+			return err
+		}
+		setArrayIfNotEmpty(volIDs, p.SetVolumeids)
 	}
 	p.SetExpunge(true)
-	setArrayIfNotEmpty(volIDs, p.SetVolumeids)
 	if _, err := c.csAsync.VirtualMachine.DestroyVirtualMachine(p); err != nil &&
 		strings.Contains(strings.ToLower(err.Error()), "unable to find uuid for id") {
 		// VM doesn't exist. Success...
@@ -462,6 +465,17 @@ func (c *client) listVMInstanceDatadiskVolumeIDs(instanceID string) ([]string, e
 	p.SetVirtualmachineid(instanceID)
 	// VM root volumes are destroyed automatically, no need to explicitly include
 	p.SetType("DATADISK")
+	// This makes extra sure that data disks created/attached by something other than CloudStack itself
+	// are not expunged. Right now this is the only way to sort of distinguish a data volume automatically
+	// created on deployVirtualMachine (by passing diskoffering ID or name) from a volume attached at a later
+	// stage (assuming those aren't called 'DATA-<something>').
+	//
+	// This f.e. prevents data volumes backing PVC's from the CloudStack CSI driver from being expunged.
+	//
+	// See:
+	// - https://github.com/apache/cloudstack/blob/b69cc0272d48f0aea7353627d760c27c284dad84/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java#L524
+	// - https://github.com/kubernetes-sigs/cluster-api-provider-cloudstack/issues/389
+	p.SetKeyword("DATA-")
 
 	listVOLResp, err := c.csAsync.Volume.ListVolumes(p)
 	if err != nil {

pkg/cloud/instance_test.go

@@ -19,11 +19,9 @@ package cloud_test
 import (
 	"encoding/base64"
 	"fmt"
-
 	"github.com/apache/cloudstack-go/v2/cloudstack"
 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo/v2"
-
 	infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3"
 	"sigs.k8s.io/cluster-api-provider-cloudstack/pkg/cloud"
 	dummies "sigs.k8s.io/cluster-api-provider-cloudstack/test/dummies/v1beta3"
@@ -750,6 +748,7 @@ var _ = Describe("Instance", func() {
 		It("calls destroy and finds VM doesn't exist, then returns nil", func() {
 			listVolumesParams.SetVirtualmachineid(*dummies.CSMachine1.Spec.InstanceID)
 			listVolumesParams.SetType("DATADISK")
+			listVolumesParams.SetKeyword("DATA-")
 			vms.EXPECT().NewDestroyVirtualMachineParams(*dummies.CSMachine1.Spec.InstanceID).
 				Return(expungeDestroyParams)
 			vms.EXPECT().DestroyVirtualMachine(expungeDestroyParams).Return(nil, fmt.Errorf("unable to find uuid for id"))
@@ -762,6 +761,7 @@ var _ = Describe("Instance", func() {
 		It("calls destroy and returns unexpected error", func() {
 			listVolumesParams.SetVirtualmachineid(*dummies.CSMachine1.Spec.InstanceID)
 			listVolumesParams.SetType("DATADISK")
+			listVolumesParams.SetKeyword("DATA-")
 			vms.EXPECT().NewDestroyVirtualMachineParams(*dummies.CSMachine1.Spec.InstanceID).
 				Return(expungeDestroyParams)
 			vms.EXPECT().DestroyVirtualMachine(expungeDestroyParams).Return(nil, fmt.Errorf("new error"))
@@ -773,6 +773,7 @@ var _ = Describe("Instance", func() {
 		It("calls destroy without error but cannot resolve VM after", func() {
 			listVolumesParams.SetVirtualmachineid(*dummies.CSMachine1.Spec.InstanceID)
 			listVolumesParams.SetType("DATADISK")
+			listVolumesParams.SetKeyword("DATA-")
 			vms.EXPECT().NewDestroyVirtualMachineParams(*dummies.CSMachine1.Spec.InstanceID).
 				Return(expungeDestroyParams)
 			vms.EXPECT().DestroyVirtualMachine(expungeDestroyParams).Return(nil, nil)
@@ -787,6 +788,7 @@ var _ = Describe("Instance", func() {
 		It("calls destroy without error and identifies it as expunging", func() {
 			listVolumesParams.SetVirtualmachineid(*dummies.CSMachine1.Spec.InstanceID)
 			listVolumesParams.SetType("DATADISK")
+			listVolumesParams.SetKeyword("DATA-")
 			vms.EXPECT().NewDestroyVirtualMachineParams(*dummies.CSMachine1.Spec.InstanceID).
 				Return(expungeDestroyParams)
 			vms.EXPECT().DestroyVirtualMachine(expungeDestroyParams).Return(nil, nil)
@@ -803,6 +805,7 @@ var _ = Describe("Instance", func() {
 		It("calls destroy without error and identifies it as expunged", func() {
 			listVolumesParams.SetVirtualmachineid(*dummies.CSMachine1.Spec.InstanceID)
 			listVolumesParams.SetType("DATADISK")
+			listVolumesParams.SetKeyword("DATA-")
 			vms.EXPECT().NewDestroyVirtualMachineParams(*dummies.CSMachine1.Spec.InstanceID).
 				Return(expungeDestroyParams)
 			vms.EXPECT().DestroyVirtualMachine(expungeDestroyParams).Return(nil, nil)
@@ -819,6 +822,7 @@ var _ = Describe("Instance", func() {
 		It("calls destroy without error and identifies it as stopping", func() {
 			listVolumesParams.SetVirtualmachineid(*dummies.CSMachine1.Spec.InstanceID)
 			listVolumesParams.SetType("DATADISK")
+			listVolumesParams.SetKeyword("DATA-")
 			vms.EXPECT().NewDestroyVirtualMachineParams(*dummies.CSMachine1.Spec.InstanceID).
 				Return(expungeDestroyParams)
 			vms.EXPECT().DestroyVirtualMachine(expungeDestroyParams).Return(nil, nil)
@@ -830,5 +834,35 @@ var _ = Describe("Instance", func() {
 				}, 1, nil)
 			Ω(client.DestroyVMInstance(dummies.CSMachine1)).Should(MatchError("VM deletion in progress"))
 		})
+
+		It("calls destroy without error on a VM without additional diskoffering and does not search for data disks", func() {
+			dummies.CSMachine1.Spec.DiskOffering = nil
+			expungeDestroyParams := &cloudstack.DestroyVirtualMachineParams{}
+			expungeDestroyParams.SetExpunge(true)
+
+			vms.EXPECT().NewDestroyVirtualMachineParams(*dummies.CSMachine1.Spec.InstanceID).
+				Return(expungeDestroyParams)
+			vms.EXPECT().DestroyVirtualMachine(gomock.Any()).DoAndReturn(
+				func(p *cloudstack.DestroyVirtualMachineParams) (*cloudstack.DestroyVirtualMachineResponse, error) {
+					value, ok := p.GetExpunge()
+					Ω(value).To(BeTrue())
+					Ω(ok).To(BeTrue())
+
+					ids, ok := p.GetVolumeids()
+					Ω(len(ids)).To(Equal(0))
+					Ω(ok).To(BeFalse())
+
+					return nil, nil
+				})
+			vs.EXPECT().NewListVolumesParams().Times(0)
+			vs.EXPECT().ListVolumes(gomock.Any()).Times(0)
+			vms.EXPECT().GetVirtualMachinesMetricByID(*dummies.CSMachine1.Spec.InstanceID).
+				Return(&cloudstack.VirtualMachinesMetric{
+					State: "Expunged",
+				}, 1, nil)
+			Ω(dummies.CSMachine1.Spec.DiskOffering).Should(BeNil())
+			Ω(client.DestroyVMInstance(dummies.CSMachine1)).
+				Should(Succeed())
+		})
 	})
 })