Merge pull request #8465 from MaximilianoUribe/muribefalcon/feature-adding-force-delete

feature: adding support for force delete into azure

Author: k8s-ci-robot

cluster-autoscaler/cloudprovider/azure/azure_agent_pool.go

@@ -429,14 +429,14 @@ func (as *AgentPool) DeleteInstances(instances []*azureRef) error {
 		}
 	}
 
-	klog.V(6).Infof("DeleteInstances: invalidating cache")
+	klog.V(3).Infof("DeleteInstances: invalidating cache")
 	as.manager.invalidateCache()
 	return nil
 }
 
 // DeleteNodes deletes the nodes from the group.
 func (as *AgentPool) DeleteNodes(nodes []*apiv1.Node) error {
-	klog.V(6).Infof("Delete nodes requested: %v\n", nodes)
+	klog.V(3).Infof("Delete nodes requested: %v\n", nodes)
 	indexes, _, err := as.GetVMIndexes()
 	if err != nil {
 		return err
@@ -446,6 +446,11 @@ func (as *AgentPool) DeleteNodes(nodes []*apiv1.Node) error {
 		return fmt.Errorf("min size reached, nodes will not be deleted")
 	}
 
+	return as.ForceDeleteNodes(nodes)
+}
+
+// ForceDeleteNodes deletes nodes from the group regardless of constraints.
+func (as *AgentPool) ForceDeleteNodes(nodes []*apiv1.Node) error {
 	refs := make([]*azureRef, 0, len(nodes))
 	for _, node := range nodes {
 		belongs, err := as.Belongs(node)
@@ -463,19 +468,14 @@ func (as *AgentPool) DeleteNodes(nodes []*apiv1.Node) error {
 		refs = append(refs, ref)
 	}
 
-	err = as.deleteOutdatedDeployments()
+	err := as.deleteOutdatedDeployments()
 	if err != nil {
-		klog.Warningf("DeleteNodes: failed to cleanup outdated deployments with err: %v.", err)
+		klog.Warningf("ForceDeleteNodes: failed to cleanup outdated deployments with err: %v.", err)
 	}
 
 	return as.DeleteInstances(refs)
 }
 
-// ForceDeleteNodes deletes nodes from the group regardless of constraints.
-func (as *AgentPool) ForceDeleteNodes(nodes []*apiv1.Node) error {
-	return cloudprovider.ErrNotImplemented
-}
-
 // Debug returns a debug string for the agent pool.
 func (as *AgentPool) Debug() string {
 	return fmt.Sprintf("%s (%d:%d)", as.Name, as.MinSize(), as.MaxSize())

cluster-autoscaler/cloudprovider/azure/azure_agent_pool_test.go

@@ -410,6 +410,46 @@ func TestDeleteInstances(t *testing.T) {
 	assert.Equal(t, expectedErr, err)
 }
 
+func TestForceDeleteNodes(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	as := newTestAgentPool(newTestAzureManager(t), "as")
+	as1 := newTestAgentPool(newTestAzureManager(t), "as1")
+	as.manager.azureCache.instanceToNodeGroup[azureRef{Name: testValidProviderID0}] = as
+	as.manager.azureCache.instanceToNodeGroup[azureRef{Name: testValidProviderID1}] = as1
+	as.manager.azureCache.instanceToNodeGroup[azureRef{Name: testInvalidProviderID}] = as
+
+	mockVMClient := mockvmclient.NewMockInterface(ctrl)
+	as.manager.azClient.virtualMachinesClient = mockVMClient
+
+	mockSAClient := mockstorageaccountclient.NewMockInterface(ctrl)
+	as.manager.azClient.storageAccountsClient = mockSAClient
+
+	err := as.ForceDeleteNodes([]*apiv1.Node{})
+	assert.NoError(t, err)
+
+	nodes := []*apiv1.Node{
+		{
+			Spec:       apiv1.NodeSpec{ProviderID: testInvalidProviderID},
+			ObjectMeta: v1.ObjectMeta{Name: "node"},
+		},
+	}
+	err = as.ForceDeleteNodes(nodes)
+	expectedErr := fmt.Errorf("resource name was missing from identifier")
+	assert.Equal(t, expectedErr, err)
+
+	nodes = []*apiv1.Node{
+		{
+			Spec:       apiv1.NodeSpec{ProviderID: testValidProviderID1},
+			ObjectMeta: v1.ObjectMeta{Name: "node1"},
+		},
+	}
+	err = as.ForceDeleteNodes(nodes)
+	expectedErr = fmt.Errorf("node1 belongs to a different asg than as")
+	assert.Equal(t, expectedErr, err)
+}
+
 func TestAgentPoolDeleteNodes(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()

cluster-autoscaler/cloudprovider/azure/azure_scale_set.go

@@ -589,7 +589,7 @@ func (scaleSet *ScaleSet) waitForDeleteInstances(future *azure.Future, requiredI
 
 // DeleteNodes deletes the nodes from the group.
 func (scaleSet *ScaleSet) DeleteNodes(nodes []*apiv1.Node) error {
-	klog.V(8).Infof("Delete nodes requested: %q\n", nodes)
+	klog.V(3).Infof("Delete nodes requested: %q\n", nodes)
 	size, err := scaleSet.getScaleSetSize()
 	if err != nil {
 		return err
@@ -598,12 +598,14 @@ func (scaleSet *ScaleSet) DeleteNodes(nodes []*apiv1.Node) error {
 	if int(size) <= scaleSet.MinSize() {
 		return fmt.Errorf("min size reached, nodes will not be deleted")
 	}
+	return scaleSet.ForceDeleteNodes(nodes)
+}
 
-	// Distinguish between unregistered node deletion and normal node deletion
+// ForceDeleteNodes deletes nodes from the group regardless of constraints.
+func (scaleSet *ScaleSet) ForceDeleteNodes(nodes []*apiv1.Node) error {
+	klog.V(3).Infof("Delete nodes requested: %q\n", nodes)
 	refs := make([]*azureRef, 0, len(nodes))
 	hasUnregisteredNodes := false
-	unregisteredRefs := make([]*azureRef, 0, len(nodes))
-
 	for _, node := range nodes {
 		belongs, err := scaleSet.Belongs(node)
 		if err != nil {
@@ -620,28 +622,12 @@ func (scaleSet *ScaleSet) DeleteNodes(nodes []*apiv1.Node) error {
 		ref := &azureRef{
 			Name: node.Spec.ProviderID,
 		}
-
-		if node.Annotations[cloudprovider.FakeNodeReasonAnnotation] == cloudprovider.FakeNodeUnregistered {
-			klog.V(5).Infof("Node: %s type is unregistered..Appending to the unregistered list", node.Name)
-			unregisteredRefs = append(unregisteredRefs, ref)
-		} else {
-			refs = append(refs, ref)
-		}
-	}
-
-	if len(unregisteredRefs) > 0 {
-		klog.V(3).Infof("Removing unregisteredNodes: %v", unregisteredRefs)
-		return scaleSet.DeleteInstances(unregisteredRefs, true)
+		refs = append(refs, ref)
 	}
 
 	return scaleSet.DeleteInstances(refs, hasUnregisteredNodes)
 }
 
-// ForceDeleteNodes deletes nodes from the group regardless of constraints.
-func (scaleSet *ScaleSet) ForceDeleteNodes(nodes []*apiv1.Node) error {
-	return cloudprovider.ErrNotImplemented
-}
-
 // Id returns ScaleSet id.
 func (scaleSet *ScaleSet) Id() string {
 	return scaleSet.Name