Repository: Azure/ARO-RP
Pull request #2034 (2022-04-01 18:31:13)

Add logic to reconcile failed Nic on Cluster Delete
Co-authored-by: Ben Vesel <bennerv@users.noreply.github.com>

### Which issue this PR addresses:

<!--
Please include a link to the ADO work item as well as any GitHub issues.

Usage: `Fixes #<GitHub issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes 13962494

### What this PR does / why we need it:

<!--
Include a brief summary of what the PR is intended to accomplish and how the PR
does it. (2-3 sentences)
-->
If a Nic is in a failed provisioning state it will fail to be deleted.  This change will check the provisioning state of the nic and reconcile it if needed prior to attempting to delete the Nic.
### Test plan for issue:

<!--
How did you test that this PR works?

- Are there unit tests?
- Are there integration/e2e tests?
- If it is not possible to write automated tests, explain why and document how
  to manually test and verify the feature.
-->
Added delete_test.go for unit testing the deleteNic function.
Integration/e2e testing is not present for this change.  These tests would be difficult to automate as it requires a Nic in a failed provisioning state.

To manually test this change, one would need to find and delete a cluster with a failed Nic.
### Is there any documentation that needs to be updated for this PR?
No
<!--
- If yes and the docs are in GitHub, include doc updates in the PR.
- If yes and the docs are not in GitHub (i.e. ADO wiki), include a link to the
  docs.
- If no, explain why (e.g. "tech debt cleanup, N/A").
-->


bennerv: @tony-schndr It looks like there was a nil pointer dereference introduced here.  See https://msazure.visualstudio.com/AzureRedHatOpenShift/_build/results?buildId=53762829&view=logs&j=10f66467-59e5-53bc-0c8a-42cc89770de4&t=3bbe7e73-d181-5aca-4b77-91acc0ef0f4a for info.  It might be useful to create a development cluster with this code and try deleting it and see what happens if you can't figure it out based on logs.  
tony-schndr: @s-amann Yes I have time, I'll take a look at getting unit tests updated for the two additional test cases you mentioned.
gvanderpotte: LGTM
bennerv: /azp run ci 
bennerv: /azp run e2e
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/pkg/cluster/delete.go b/pkg/cluster/delete.go
index 85f1e290b..56b37bb64 100644
--- a/pkg/cluster/delete.go
+++ b/pkg/cluster/delete.go
@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
 	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/Azure/go-autorest/autorest/azure"
@@ -29,6 +30,19 @@ import (
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
 
+func (m *manager) deleteNic(ctx context.Context, resource mgmtfeatures.GenericResourceExpanded) error {
+	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
+
+	if resource.ProvisioningState != nil && !strings.EqualFold(*resource.ProvisioningState, "succeeded") {
+		m.log.Printf("NIC '%s' is not in a succeeded provisioning state, attempting to reconcile prior to deletion.", *resource.ID)
+		err := m.interfaces.CreateOrUpdateAndWait(ctx, resourceGroup, *resource.Name, mgmtnetwork.Interface{})
+		if err != nil {
+			return err
+		}
+	}
+	return m.interfaces.DeleteAndWait(ctx, resourceGroup, *resource.Name)
+}
+
 func (m *manager) deletePrivateDNSVirtualNetworkLinks(ctx context.Context, resourceID string) error {
 	r, err := azure.ParseResourceID(resourceID)
 	if err != nil {
@@ -124,16 +138,17 @@ func (m *manager) disconnectSecurityGroup(ctx context.Context, resourceID string
 // parallel and waiting for completion before we proceed.  Any type not in the
 // map is considered to be at level 0.  Keys must be lower case.
 var deleteOrder = map[string]int{
-	"microsoft.compute/virtualmachines":     -1, // first, and before microsoft.compute/disks, microsoft.network/networkinterfaces
-	"microsoft.network/privatelinkservices": -1, // before microsoft.network/loadbalancers
-	"microsoft.network/privateendpoints":    -1, // before microsoft.network/networkinterfaces
+	"microsoft.compute/virtualmachines":     -2, // first, and before microsoft.compute/disks, microsoft.network/networkinterfaces
+	"microsoft.network/privatelinkservices": -2, // before microsoft.network/loadbalancers
+	"microsoft.network/privateendpoints":    -2, // before microsoft.network/networkinterfaces
+	"microsoft.network/networkinterfaces":   -1, // before microsoft.network/loadbalancers
 	"microsoft.network/privatednszones":     1,  // after everything else: get other deletions underway first
 }
 
 func (m *manager) deleteResources(ctx context.Context) error {
 	resourceGroup := stringutils.LastTokenByte(m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID, '/')
 
-	resources, err := m.resources.ListByResourceGroup(ctx, resourceGroup, "", "", nil)
+	resources, err := m.resources.ListByResourceGroup(ctx, resourceGroup, "", "provisioningState", nil)
 	if detailedErr, ok := err.(autorest.DetailedError); ok &&
 		(detailedErr.StatusCode == http.StatusNotFound ||
 			detailedErr.StatusCode == http.StatusForbidden) {
@@ -187,6 +202,12 @@ func (m *manager) deleteResources(ctx context.Context) error {
 				if err != nil {
 					return err
 				}
+
+			case "microsoft.network/networkinterfaces":
+				err = m.deleteNic(ctx, *resource)
+				if err != nil {
+					return err
+				}
 			}
 
 			m.log.Printf("deleting %s", *resource.ID)


Test patch:
diff --git a/pkg/cluster/delete_test.go b/pkg/cluster/delete_test.go
new file mode 100644
index 000000000..9abc5544e
--- /dev/null
+++ b/pkg/cluster/delete_test.go
@@ -0,0 +1,112 @@
+package cluster
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	mgmtfeatures "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-07-01/features"
+	"github.com/Azure/go-autorest/autorest/to"
+	"github.com/golang/mock/gomock"
+	"github.com/sirupsen/logrus"
+
+	"github.com/Azure/ARO-RP/pkg/api"
+	mock_network "github.com/Azure/ARO-RP/pkg/util/mocks/azureclient/mgmt/network"
+	mock_env "github.com/Azure/ARO-RP/pkg/util/mocks/env"
+)
+
+func TestDeleteNic(t *testing.T) {
+	ctx := context.Background()
+	subscription := "00000000-0000-0000-0000-000000000000"
+	clusterRG := "cluster-rg"
+	nicName := "nic-name"
+	location := "eastus"
+
+	tests := []struct {
+		name              string
+		mocks             func(*mock_network.MockInterfacesClient)
+		provisioningState *string
+		wantErr           string
+	}{
+		{
+			name: "nic is in succeeded provisioning state",
+			mocks: func(networkInterfaces *mock_network.MockInterfacesClient) {
+				networkInterfaces.EXPECT().DeleteAndWait(gomock.Any(), clusterRG, nicName).Return(nil)
+			},
+			provisioningState: to.StringPtr("SUCCEEDED"),
+		},
+		{
+			name: "nic is in failed provisioning state",
+			mocks: func(networkInterfaces *mock_network.MockInterfacesClient) {
+				networkInterfaces.EXPECT().CreateOrUpdateAndWait(gomock.Any(), clusterRG, nicName, gomock.Any()).Return(nil)
+				networkInterfaces.EXPECT().DeleteAndWait(gomock.Any(), clusterRG, nicName).Return(nil)
+			},
+			provisioningState: to.StringPtr("FAILED"),
+		},
+		{
+			name: "provisioning state is failed and CreateOrUpdateAndWait returns error",
+			mocks: func(networkInterfaces *mock_network.MockInterfacesClient) {
+				networkInterfaces.EXPECT().CreateOrUpdateAndWait(gomock.Any(), clusterRG, nicName, gomock.Any()).Return(fmt.Errorf("Failed to update"))
+			},
+			provisioningState: to.StringPtr("FAILED"),
+			wantErr:           "Failed to update",
+		},
+		{
+			name: "DeleteAndWait returns error",
+			mocks: func(networkInterfaces *mock_network.MockInterfacesClient) {
+				networkInterfaces.EXPECT().DeleteAndWait(gomock.Any(), clusterRG, nicName).Return(fmt.Errorf("Failed to delete"))
+			},
+			provisioningState: to.StringPtr("SUCCEEDED"),
+			wantErr:           "Failed to delete",
+		},
+		{
+			name: "provisioningState is nil",
+			mocks: func(networkInterfaces *mock_network.MockInterfacesClient) {
+				networkInterfaces.EXPECT().DeleteAndWait(gomock.Any(), clusterRG, nicName).Return(nil)
+			},
+			provisioningState: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			env := mock_env.NewMockInterface(controller)
+			env.EXPECT().Location().AnyTimes().Return(location)
+
+			networkInterfaces := mock_network.NewMockInterfacesClient(controller)
+
+			tt.mocks(networkInterfaces)
+
+			m := manager{
+				log: logrus.NewEntry(logrus.StandardLogger()),
+				doc: &api.OpenShiftClusterDocument{
+					OpenShiftCluster: &api.OpenShiftCluster{
+						Properties: api.OpenShiftClusterProperties{
+							ClusterProfile: api.ClusterProfile{
+								ResourceGroupID: fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", subscription, clusterRG),
+							},
+						},
+					},
+				},
+				interfaces: networkInterfaces,
+			}
+
+			resource := mgmtfeatures.GenericResourceExpanded{
+				Name:              to.StringPtr(nicName),
+				ID:                to.StringPtr(fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/networkInterfaces/%s", subscription, clusterRG, nicName)),
+				ProvisioningState: tt.provisioningState,
+			}
+
+			err := m.deleteNic(ctx, resource)
+			if err != nil && err.Error() != tt.wantErr {
+				t.Errorf("got error: '%s'", err.Error())
+			}
+		})
+	}
+}