Skip to content

Commit 100fd93

Browse files
RadekManakRadekManak
committed
Implement DiskDeletionPolicyTypeDelete in controller for ASH
Because ASH does not support DeleteOption API field for dataDisks we need to implement the deletion behavior by deleting the disks that have DiskDeletionPolicyTypeDelete when machine is deleted.
1 parent 87cd7eb commit 100fd93

File tree

6 files changed

+280
-40
lines changed

6 files changed

+280
-40
lines changed

‎pkg/cloud/azure/actuators/machine/actuator_test.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,13 +89,13 @@ func newMachine(t *testing.T, machineConfig machinev1.AzureMachineProviderSpec,
8989
}
9090
}
9191

92-
func newFakeScope(t *testing.T, label string) *actuators.MachineScope {
92+
func newFakeMachineScope(t *testing.T, label string, cloudEnv string) *actuators.MachineScope {
9393
labels := make(map[string]string)
9494
labels[actuators.MachineRoleLabel] = label
9595
labels[machinev1.MachineClusterIDLabel] = "clusterID"
9696
machineConfig := machinev1.AzureMachineProviderSpec{}
9797
m := newMachine(t, machineConfig, labels)
98-
return &actuators.MachineScope{
98+
return actuators.NewFakeMachineScope(actuators.FakeMachineScopeParams{
9999
Context: context.Background(),
100100
Machine: m,
101101
MachineConfig: &machinev1.AzureMachineProviderSpec{
@@ -106,7 +106,8 @@ func newFakeScope(t *testing.T, label string) *actuators.MachineScope {
106106
ManagedIdentity: "dummyIdentity",
107107
},
108108
MachineStatus: &machinev1.AzureMachineProviderStatus{},
109-
}
109+
CloudEnv: cloudEnv,
110+
})
110111
}
111112

112113
func newFakeReconciler(t *testing.T) *Reconciler {
@@ -137,7 +138,7 @@ func newFakeReconciler(t *testing.T) *Reconciler {
137138
networkInterfacesSvc.EXPECT().Delete(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
138139

139140
return &Reconciler{
140-
scope: newFakeScope(t, actuators.ControlPlane),
141+
scope: newFakeMachineScope(t, actuators.ControlPlane, string(configv1.AzurePublicCloud)),
141142
networkInterfacesSvc: networkInterfacesSvc,
142143
virtualMachinesSvc: fakeVMSuccessSvc,
143144
virtualMachinesExtSvc: fakeSuccessSvc,
@@ -447,7 +448,7 @@ func (s *FakeAvailabilityZonesService) Delete(ctx context.Context, spec azure.Sp
447448
}
448449

449450
func TestAvailabilityZones(t *testing.T) {
450-
fakeScope := newFakeScope(t, actuators.ControlPlane)
451+
fakeScope := newFakeMachineScope(t, actuators.ControlPlane, string(configv1.AzurePublicCloud))
451452
fakeReconciler := newFakeReconcilerWithScope(t, fakeScope)
452453

453454
fakeReconciler.scope.MachineConfig.Zone = "2"
@@ -491,7 +492,7 @@ func TestGetZone(t *testing.T) {
491492
}
492493

493494
for _, tc := range testCases {
494-
fakeScope := newFakeScope(t, actuators.ControlPlane)
495+
fakeScope := newFakeMachineScope(t, actuators.ControlPlane, string(configv1.AzurePublicCloud))
495496
fakeReconciler := newFakeReconcilerWithScope(t, fakeScope)
496497
fakeReconciler.scope.MachineConfig.Zone = tc.inputZone
497498

@@ -512,7 +513,7 @@ func TestGetZone(t *testing.T) {
512513
}
513514

514515
func TestCustomUserData(t *testing.T) {
515-
fakeScope := newFakeScope(t, actuators.Node)
516+
fakeScope := newFakeMachineScope(t, actuators.Node, string(configv1.AzurePublicCloud))
516517
userDataSecret := &corev1.Secret{
517518
ObjectMeta: metav1.ObjectMeta{
518519
Name: "testCustomUserData",
@@ -541,7 +542,7 @@ func TestCustomUserData(t *testing.T) {
541542
}
542543

543544
func TestCustomDataFailures(t *testing.T) {
544-
fakeScope := newFakeScope(t, actuators.Node)
545+
fakeScope := newFakeMachineScope(t, actuators.Node, string(configv1.AzurePublicCloud))
545546
userDataSecret := &corev1.Secret{
546547
ObjectMeta: metav1.ObjectMeta{
547548
Name: "testCustomUserData",

‎pkg/cloud/azure/actuators/machine/reconciler.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,6 +529,29 @@ func (s *Reconciler) Delete(ctx context.Context) error {
529529
return fmt.Errorf("failed to delete OS disk: %w", err)
530530
}
531531

532+
// On Azure Stack Hub, DeleteOption is not supported on the VM API.
533+
// We need to manually delete data disks that have deletionPolicy set to Delete.
534+
if s.scope.IsStackHub() && len(s.scope.MachineConfig.DataDisks) > 0 {
535+
for _, disk := range s.scope.MachineConfig.DataDisks {
536+
if disk.DeletionPolicy == machinev1.DiskDeletionPolicyTypeDelete {
537+
dataDiskName := azure.GenerateDataDiskName(s.scope.Machine.Name, disk.NameSuffix)
538+
539+
dataDiskSpec := &disks.Spec{
540+
Name: dataDiskName,
541+
}
542+
if err := s.disksSvc.Delete(ctx, dataDiskSpec); err != nil {
543+
metrics.RegisterFailedInstanceDelete(&metrics.MachineLabels{
544+
Name: s.scope.Machine.Name,
545+
Namespace: s.scope.Machine.Namespace,
546+
Reason: "failed to delete data disk",
547+
})
548+
549+
return fmt.Errorf("failed to delete data disk %s: %w", dataDiskName, err)
550+
}
551+
}
552+
}
553+
}
554+
532555
if s.scope.MachineConfig.Vnet == "" {
533556
return fmt.Errorf("MachineConfig vnet is missing on machine %s", s.scope.Machine.Name)
534557
}

‎pkg/cloud/azure/actuators/machine/reconciler_test.go‎

Lines changed: 183 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,13 @@ import (
1111
"github.com/Azure/go-autorest/autorest"
1212
"github.com/golang/mock/gomock"
1313
. "github.com/onsi/gomega"
14+
configv1 "github.com/openshift/api/config/v1"
1415
machinev1 "github.com/openshift/api/machine/v1beta1"
1516
machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine"
1617
"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure"
1718
"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure/actuators"
1819
mock_azure "github.com/openshift/machine-api-provider-azure/pkg/cloud/azure/mock"
20+
"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure/services/disks"
1921
"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure/services/networkinterfaces"
2022
"k8s.io/apimachinery/pkg/api/resource"
2123
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -205,9 +207,11 @@ func TestSetMachineCloudProviderSpecificsTable(t *testing.T) {
205207
expectedSpecLabels map[string]string
206208
}{
207209
{
208-
name: "with a blank vm",
209-
scope: func(t *testing.T) *actuators.MachineScope { return newFakeScope(t, "worker") },
210-
vm: armcompute.VirtualMachine{},
210+
name: "with a blank vm",
211+
scope: func(t *testing.T) *actuators.MachineScope {
212+
return newFakeMachineScope(t, "worker", string(configv1.AzurePublicCloud))
213+
},
214+
vm: armcompute.VirtualMachine{},
211215
expectedLabels: map[string]string{
212216
actuators.MachineRoleLabel: "worker",
213217
machinev1.MachineClusterIDLabel: "clusterID",
@@ -218,8 +222,10 @@ func TestSetMachineCloudProviderSpecificsTable(t *testing.T) {
218222
expectedSpecLabels: nil,
219223
},
220224
{
221-
name: "with a running vm",
222-
scope: func(t *testing.T) *actuators.MachineScope { return newFakeScope(t, "good-worker") },
225+
name: "with a running vm",
226+
scope: func(t *testing.T) *actuators.MachineScope {
227+
return newFakeMachineScope(t, "good-worker", string(configv1.AzurePublicCloud))
228+
},
223229
vm: armcompute.VirtualMachine{
224230
Properties: &armcompute.VirtualMachineProperties{
225231
ProvisioningState: ptr.To[string]("Running"),
@@ -235,8 +241,10 @@ func TestSetMachineCloudProviderSpecificsTable(t *testing.T) {
235241
expectedSpecLabels: nil,
236242
},
237243
{
238-
name: "with a VMSize set vm",
239-
scope: func(t *testing.T) *actuators.MachineScope { return newFakeScope(t, "sized-worker") },
244+
name: "with a VMSize set vm",
245+
scope: func(t *testing.T) *actuators.MachineScope {
246+
return newFakeMachineScope(t, "sized-worker", string(configv1.AzurePublicCloud))
247+
},
240248
vm: armcompute.VirtualMachine{
241249
Properties: &armcompute.VirtualMachineProperties{
242250
HardwareProfile: &armcompute.HardwareProfile{
@@ -255,8 +263,10 @@ func TestSetMachineCloudProviderSpecificsTable(t *testing.T) {
255263
expectedSpecLabels: nil,
256264
},
257265
{
258-
name: "with a vm location",
259-
scope: func(t *testing.T) *actuators.MachineScope { return newFakeScope(t, "located-worker") },
266+
name: "with a vm location",
267+
scope: func(t *testing.T) *actuators.MachineScope {
268+
return newFakeMachineScope(t, "located-worker", string(configv1.AzurePublicCloud))
269+
},
260270
vm: armcompute.VirtualMachine{
261271
Location: ptr.To[string]("nowhere"),
262272
},
@@ -271,8 +281,10 @@ func TestSetMachineCloudProviderSpecificsTable(t *testing.T) {
271281
expectedSpecLabels: nil,
272282
},
273283
{
274-
name: "with a vm with zones",
275-
scope: func(t *testing.T) *actuators.MachineScope { return newFakeScope(t, "zoned-worker") },
284+
name: "with a vm with zones",
285+
scope: func(t *testing.T) *actuators.MachineScope {
286+
return newFakeMachineScope(t, "zoned-worker", string(configv1.AzurePublicCloud))
287+
},
276288
vm: armcompute.VirtualMachine{
277289
Zones: abcZones,
278290
},
@@ -287,8 +299,10 @@ func TestSetMachineCloudProviderSpecificsTable(t *testing.T) {
287299
expectedSpecLabels: nil,
288300
},
289301
{
290-
name: "with a vm with a faultdomain set",
291-
scope: func(t *testing.T) *actuators.MachineScope { return newFakeScope(t, "availabilityset-worker") },
302+
name: "with a vm with a faultdomain set",
303+
scope: func(t *testing.T) *actuators.MachineScope {
304+
return newFakeMachineScope(t, "availabilityset-worker", string(configv1.AzurePublicCloud))
305+
},
292306
vm: armcompute.VirtualMachine{
293307
Properties: &armcompute.VirtualMachineProperties{
294308
InstanceView: &armcompute.VirtualMachineInstanceView{
@@ -309,7 +323,7 @@ func TestSetMachineCloudProviderSpecificsTable(t *testing.T) {
309323
{
310324
name: "with a vm on spot",
311325
scope: func(t *testing.T) *actuators.MachineScope {
312-
scope := newFakeScope(t, "spot-worker")
326+
scope := newFakeMachineScope(t, "spot-worker", string(configv1.AzurePublicCloud))
313327
scope.MachineConfig.SpotVMOptions = &machinev1.SpotVMOptions{}
314328
return scope
315329
},
@@ -708,7 +722,9 @@ func TestMachineUpdateWithProvisionsngFailedNic(t *testing.T) {
708722

709723
testCtx := context.TODO()
710724

711-
scope := func(t *testing.T) *actuators.MachineScope { return newFakeScope(t, "worker") }
725+
scope := func(t *testing.T) *actuators.MachineScope {
726+
return newFakeMachineScope(t, "worker", string(configv1.AzurePublicCloud))
727+
}
712728
r := newFakeReconcilerWithScope(t, scope(t))
713729
r.networkInterfacesSvc = networkSvc
714730

@@ -744,3 +760,155 @@ func TestMachineUpdateWithProvisionsngFailedNic(t *testing.T) {
744760
})
745761
}
746762
}
763+
764+
// TestStackHubDataDiskDeletion tests that data disks with DeletionPolicy=Delete are manually deleted on Azure Stack Hub
765+
func TestStackHubDataDiskDeletion(t *testing.T) {
766+
testCases := []struct {
767+
name string
768+
isStackHub bool
769+
dataDisks []machinev1.DataDisk
770+
}{
771+
{
772+
name: "Stack Hub with single data disk with Delete policy",
773+
isStackHub: true,
774+
dataDisks: []machinev1.DataDisk{
775+
{
776+
NameSuffix: "disk1",
777+
DiskSizeGB: 128,
778+
Lun: 0,
779+
DeletionPolicy: machinev1.DiskDeletionPolicyTypeDelete,
780+
ManagedDisk: machinev1.DataDiskManagedDiskParameters{StorageAccountType: machinev1.StorageAccountPremiumLRS},
781+
},
782+
},
783+
},
784+
{
785+
name: "Stack Hub with multiple data disks with Delete policy",
786+
isStackHub: true,
787+
dataDisks: []machinev1.DataDisk{
788+
{
789+
NameSuffix: "disk1",
790+
DiskSizeGB: 128,
791+
Lun: 0,
792+
DeletionPolicy: machinev1.DiskDeletionPolicyTypeDelete,
793+
ManagedDisk: machinev1.DataDiskManagedDiskParameters{StorageAccountType: machinev1.StorageAccountPremiumLRS},
794+
},
795+
{
796+
NameSuffix: "disk2",
797+
DiskSizeGB: 256,
798+
Lun: 1,
799+
DeletionPolicy: machinev1.DiskDeletionPolicyTypeDelete,
800+
ManagedDisk: machinev1.DataDiskManagedDiskParameters{StorageAccountType: machinev1.StorageAccountPremiumLRS},
801+
},
802+
},
803+
},
804+
{
805+
name: "Stack Hub with data disk with Detach policy",
806+
isStackHub: true,
807+
dataDisks: []machinev1.DataDisk{
808+
{
809+
NameSuffix: "disk1",
810+
DiskSizeGB: 128,
811+
Lun: 0,
812+
DeletionPolicy: machinev1.DiskDeletionPolicyTypeDetach,
813+
ManagedDisk: machinev1.DataDiskManagedDiskParameters{StorageAccountType: machinev1.StorageAccountPremiumLRS},
814+
},
815+
},
816+
},
817+
{
818+
name: "Stack Hub with mixed deletion policies",
819+
isStackHub: true,
820+
dataDisks: []machinev1.DataDisk{
821+
{
822+
NameSuffix: "disk1",
823+
DiskSizeGB: 128,
824+
Lun: 0,
825+
DeletionPolicy: machinev1.DiskDeletionPolicyTypeDelete,
826+
ManagedDisk: machinev1.DataDiskManagedDiskParameters{StorageAccountType: machinev1.StorageAccountPremiumLRS},
827+
},
828+
{
829+
NameSuffix: "disk2",
830+
DiskSizeGB: 256,
831+
Lun: 1,
832+
DeletionPolicy: machinev1.DiskDeletionPolicyTypeDetach,
833+
ManagedDisk: machinev1.DataDiskManagedDiskParameters{StorageAccountType: machinev1.StorageAccountPremiumLRS},
834+
},
835+
},
836+
},
837+
{
838+
name: "Non-Stack Hub with data disks",
839+
isStackHub: false,
840+
dataDisks: []machinev1.DataDisk{
841+
{
842+
NameSuffix: "disk1",
843+
DiskSizeGB: 128,
844+
Lun: 0,
845+
DeletionPolicy: machinev1.DiskDeletionPolicyTypeDelete,
846+
ManagedDisk: machinev1.DataDiskManagedDiskParameters{StorageAccountType: machinev1.StorageAccountPremiumLRS},
847+
},
848+
},
849+
},
850+
}
851+
852+
for _, tc := range testCases {
853+
t.Run(tc.name, func(t *testing.T) {
854+
g := NewWithT(t)
855+
mockCtrl := gomock.NewController(t)
856+
857+
vmSvc := mock_azure.NewMockService(mockCtrl)
858+
disksSvc := mock_azure.NewMockService(mockCtrl)
859+
networkSvc := mock_azure.NewMockService(mockCtrl)
860+
availabilitySetsSvc := mock_azure.NewMockService(mockCtrl)
861+
862+
testCtx := context.TODO()
863+
864+
platformType := string(configv1.AzureStackCloud)
865+
if !tc.isStackHub {
866+
platformType = string(configv1.AzurePublicCloud)
867+
}
868+
scope := newFakeMachineScope(t, "worker", platformType)
869+
scope.MachineConfig.DataDisks = tc.dataDisks
870+
871+
r := &Reconciler{
872+
scope: scope,
873+
virtualMachinesSvc: vmSvc,
874+
disksSvc: disksSvc,
875+
networkInterfacesSvc: networkSvc,
876+
availabilitySetsSvc: availabilitySetsSvc,
877+
}
878+
879+
// Expect VM deletion
880+
vmSvc.EXPECT().Delete(testCtx, gomock.Any()).Return(nil).Times(1)
881+
882+
// Expect OS disk deletion - always happens
883+
expectedOSDiskName := azure.GenerateOSDiskName(scope.Machine.Name)
884+
disksSvc.EXPECT().Delete(testCtx, gomock.Any()).DoAndReturn(
885+
func(ctx context.Context, spec azure.Spec) error {
886+
diskSpec, ok := spec.(*disks.Spec)
887+
g.Expect(ok).To(BeTrue(), "spec should be a disk spec")
888+
g.Expect(diskSpec.Name).To(Equal(expectedOSDiskName), "OS disk name should match")
889+
return nil
890+
},
891+
).Times(1)
892+
893+
// Expect data disk deletions only for disks with Delete policy on Stack Hub
894+
if tc.isStackHub {
895+
for _, disk := range tc.dataDisks {
896+
if disk.DeletionPolicy == machinev1.DiskDeletionPolicyTypeDelete {
897+
expectedDataDiskName := azure.GenerateDataDiskName(scope.Machine.Name, disk.NameSuffix)
898+
disksSvc.EXPECT().Delete(testCtx, gomock.Any()).DoAndReturn(
899+
func(ctx context.Context, spec azure.Spec) error {
900+
diskSpec, ok := spec.(*disks.Spec)
901+
g.Expect(ok).To(BeTrue(), "spec should be a disk spec")
902+
g.Expect(diskSpec.Name).To(Equal(expectedDataDiskName), "data disk name should match")
903+
return nil
904+
},
905+
).Times(1)
906+
}
907+
}
908+
}
909+
910+
err := r.Delete(testCtx)
911+
g.Expect(err).To(BeNil())
912+
})
913+
}
914+
}

0 commit comments

Comments
 (0)