From c6b8207bfe508995a5b641c21acae26a878512be Mon Sep 17 00:00:00 2001 From: "Krishnakumar R(KK)" <29471693+kkmsft@users.noreply.github.com> Date: Mon, 11 Nov 2019 10:53:43 -0800 Subject: [PATCH 1/2] Azure: Filter disks with ToBeDetached flag from attach/detach After the detach calls are made, any further updates with the disks in the process of getting detached will complain with errors such as: " Cannot attach data disk '' to VM '' because the disk's ToBeDetached flag is set to true. Please either remove the disk from APIModel or set the ToBeDetached flag to false and then try again " This PR will ensure that the disks with the ToBeDetached flag set are filtered out before the update to the cloud is made. --- .../azure/azure_controller_common.go | 14 ++++++++++++++ .../azure/azure_controller_standard.go | 6 ++---- .../azure/azure_controller_vmss.go | 6 ++---- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go index f0401d42270..9cf33b1cd0a 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common.go @@ -302,3 +302,17 @@ func (c *controllerCommon) DisksAreAttached(diskNames []string, nodeName types.N return attached, nil } + +func filterDetachingDisks(unfilteredDisks []compute.DataDisk) []compute.DataDisk { + filteredDisks := []compute.DataDisk{} + for _, disk := range unfilteredDisks { + if disk.ToBeDetached != nil && *disk.ToBeDetached { + if disk.Name != nil { + klog.V(2).Infof("Filtering disk: %s with ToBeDetached flag set.", *disk.Name) + } + } else { + filteredDisks = append(filteredDisks, disk) + } + } + return filteredDisks +} diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go index 995609d6899..93440be6aa6 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_standard.go @@ -42,8 +42,7 @@ func (as *availabilitySet) AttachDisk(isManagedDisk bool, diskName, diskURI stri return err } - disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) - copy(disks, *vm.StorageProfile.DataDisks) + disks := filterDetachingDisks(*vm.StorageProfile.DataDisks) if isManagedDisk { managedDisk := &compute.ManagedDiskParameters{ID: &diskURI} @@ -117,8 +116,7 @@ func (as *availabilitySet) DetachDisk(diskName, diskURI string, nodeName types.N return nil, err } - disks := make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) - copy(disks, *vm.StorageProfile.DataDisks) + disks := filterDetachingDisks(*vm.StorageProfile.DataDisks) bFoundDisk := false for i, disk := range disks { diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go index 2ca83e28aa1..c1692282b0d 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_vmss.go @@ -44,8 +44,7 @@ func (ss *scaleSet) AttachDisk(isManagedDisk bool, diskName, diskURI string, nod disks := []compute.DataDisk{} if vm.StorageProfile != nil && vm.StorageProfile.DataDisks != nil { - disks = make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) - copy(disks, *vm.StorageProfile.DataDisks) + disks = filterDetachingDisks(*vm.StorageProfile.DataDisks) } if isManagedDisk { managedDisk := &compute.ManagedDiskParameters{ID: &diskURI} @@ -123,8 +122,7 @@ func (ss *scaleSet) DetachDisk(diskName, diskURI string, nodeName types.NodeName disks := []compute.DataDisk{} if vm.StorageProfile != nil && vm.StorageProfile.DataDisks != nil { - disks = make([]compute.DataDisk, len(*vm.StorageProfile.DataDisks)) - copy(disks, *vm.StorageProfile.DataDisks) + disks = filterDetachingDisks(*vm.StorageProfile.DataDisks) } bFoundDisk := false for i, disk := range disks { From a10f80ffb996fa7281871cbe0e1461c3d3dc444a Mon Sep 17 00:00:00 2001 From: "Krishnakumar R(KK)" <29471693+kkmsft@users.noreply.github.com> Date: Mon, 11 Nov 2019 11:04:07 -0800 Subject: [PATCH 2/2] Azure: Filter disks with ToBeDetached flag from attach/detach- UT --- .../k8s.io/legacy-cloud-providers/azure/BUILD | 1 + .../azure/azure_controller_common_test.go | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/BUILD b/staging/src/k8s.io/legacy-cloud-providers/azure/BUILD index d410cf087c2..2b3f5dca616 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/BUILD +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/BUILD @@ -121,6 +121,7 @@ go_test( "//vendor/github.com/Azure/go-autorest/autorest:go_default_library", "//vendor/github.com/Azure/go-autorest/autorest/to:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", "//vendor/sigs.k8s.io/yaml:go_default_library", ], ) diff --git a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go index 905b95753c5..1e5168c4644 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/azure/azure_controller_common_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/pointer" ) func TestCommonAttachDisk(t *testing.T) { @@ -243,3 +244,38 @@ func TestDisksAreAttached(t *testing.T) { assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s", i, test.desc) } } + +func TestFilteredDetatchingDisks(t *testing.T) { + + disks := []compute.DataDisk{ + { + Name: pointer.StringPtr("DiskName1"), + ToBeDetached: pointer.BoolPtr(false), + ManagedDisk: &compute.ManagedDiskParameters{ + ID: pointer.StringPtr("ManagedID"), + }, + }, + { + Name: pointer.StringPtr("DiskName2"), + ToBeDetached: pointer.BoolPtr(true), + }, + { + Name: pointer.StringPtr("DiskName3"), + ToBeDetached: nil, + }, + { + Name: pointer.StringPtr("DiskName4"), + ToBeDetached: nil, + }, + } + + filteredDisks := filterDetachingDisks(disks) + assert.Equal(t, 3, len(filteredDisks)) + assert.Equal(t, "DiskName1", *filteredDisks[0].Name) + assert.Equal(t, "ManagedID", *filteredDisks[0].ManagedDisk.ID) + assert.Equal(t, "DiskName3", *filteredDisks[1].Name) + + disks = []compute.DataDisk{} + filteredDisks = filterDetachingDisks(disks) + assert.Equal(t, 0, len(filteredDisks)) +}