test: adding tests for unremovable nics

pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go

@@ -109,23 +109,21 @@ func (c *NetworkInterfaceController) Reconcile(ctx context.Context) (reconcile.R
 	}
 
 	c.populateUnremovableNics(ctx)
-	errs := make([]error, len(nics))
 	workqueue.ParallelizeUntil(ctx, 100, len(nics), func(i int) {
 		nicName := lo.FromPtr(nics[i].Name)
-		_, removableNic := c.unremovableNics.Get(nicName)
+		_, unremovableNic := c.unremovableNics.Get(nicName)
 		// The networkInterface is unremovable if its
 		// A: Reserved by NRP
 		// B: Belongs to a Nodeclaim
 		// C: Belongs to VM
-		if removableNic {
+		if !unremovableNic {
 			err := c.instanceProvider.DeleteNic(ctx, nicName)
 			if sdkerrors.IsNicReservedForAnotherVM(err) {
 				// cache the network interface as unremovable for 180 seconds
 				c.unremovableNics.SetDefault(nicName, NicReason)
-				return
 			}
 			if err != nil {
-				errs[i] = err
+				logging.FromContext(ctx).Error(err)
 				return
 			}
 

pkg/controllers/nodeclaim/garbagecollection/suite_test.go

@@ -35,9 +35,9 @@ import (
 	"github.com/Azure/karpenter-provider-azure/pkg/fake"
 	"github.com/Azure/karpenter-provider-azure/pkg/operator/options"
 	"github.com/Azure/karpenter-provider-azure/pkg/providers/instance"
+	. "github.com/Azure/karpenter-provider-azure/pkg/test"
+	. "github.com/Azure/karpenter-provider-azure/pkg/test/expectations"
 	"github.com/Azure/karpenter-provider-azure/pkg/utils"
-	. "github.com/Azure/karpenter-provider-azure/pkg/test/expectations" 
-	. "github.com/Azure/karpenter-provider-azure/pkg/test" 
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -54,7 +54,7 @@ import (
 	coreoptions "sigs.k8s.io/karpenter/pkg/operator/options"
 	coretest "sigs.k8s.io/karpenter/pkg/test"
 	. "sigs.k8s.io/karpenter/pkg/test/expectations"
-	
+
 	karpv1 "sigs.k8s.io/karpenter/pkg/apis/v1"
 )
 
@@ -338,24 +338,74 @@ var _ = Describe("VirtualMachine Garbage Collection", func() {
 })
 
 var _ = Describe("NetworkInterface Garbage Collection", func() {
-	Context("Basic", func() {
+	It("should not delete a network interface if a nodeclaim exists for it", func() {
+		// Create and apply a NodeClaim that references this NIC
+		nodeClaim := coretest.NodeClaim()
+		ExpectApplied(ctx, env.Client, nodeClaim)
+
+		// Create a managed NIC
+		nic := Interface(
+			InterfaceOptions{
+				Name: instance.GenerateResourceName(nodeClaim.Name),
+				Tags: ManagedTags(nodePool.Name),
+			},
+		)
+		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(nic.ID), *nic)
 
-		It("should delete a NIC if there is no associated VM", func() {
-			nic := Interface(
-				InterfaceOptions{
-					Tags: ManagedTags("default-np"),
-				},
-			)
-
-			azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(nic.ID), *nic)
-			nicsBeforeGC, err := azureEnv.InstanceProvider.ListNics(ctx)
-			ExpectNoError(err)
-			Expect(len(nicsBeforeGC)).To(Equal(1))
-			// add a nic to azure env, and call reconcile. It should show up in the list before reconcile
-			// then it should not showup after
-			ExpectSingletonReconciled(ctx, networkInterfaceGCController)
-		})
+		nicsBeforeGC, err := azureEnv.InstanceProvider.ListNics(ctx)
+		ExpectNoError(err)
+		Expect(len(nicsBeforeGC)).To(Equal(1))
 
+		// Run garbage collection
+		ExpectSingletonReconciled(ctx, networkInterfaceGCController)
+
+		// Verify NIC still exists after GC
+		nicsAfterGC, err := azureEnv.InstanceProvider.ListNics(ctx)
+		ExpectNoError(err)
+		Expect(len(nicsAfterGC)).To(Equal(1))
 	})
-})
+	It("should delete a NIC if there is no associated VM", func() {
+		nic := Interface(
+			InterfaceOptions{
+				Tags: ManagedTags(nodePool.Name),
+			},
+		)
+		nic2 := Interface(
+			InterfaceOptions{
+				Tags: ManagedTags(nodePool.Name),
+			},
+		)
+		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(nic.ID), *nic)
+		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(nic2.ID), *nic2)
+		nicsBeforeGC, err := azureEnv.InstanceProvider.ListNics(ctx)
+		ExpectNoError(err)
+		Expect(len(nicsBeforeGC)).To(Equal(2))
+		// add a nic to azure env, and call reconcile. It should show up in the list before reconcile
+		// then it should not showup after
+		ExpectSingletonReconciled(ctx, networkInterfaceGCController)
+		nicsAfterGC, err := azureEnv.InstanceProvider.ListNics(ctx)
+		ExpectNoError(err)
+		Expect(len(nicsAfterGC)).To(Equal(0))
+	})
+	It("should not delete a NIC if there is an associated VM", func() {
+		managedNic := Interface(
+			InterfaceOptions{
+				Tags: ManagedTags(nodePool.Name),
+			},
+		)
+		azureEnv.NetworkInterfacesAPI.NetworkInterfaces.Store(lo.FromPtr(managedNic.ID), *managedNic)
+		managedVM := VirtualMachine(
+			VirtualMachineOptions{
+				Name: lo.FromPtr(managedNic.Name),
+				Tags: ManagedTags(nodePool.Name),
+			},
+		)
+		azureEnv.VirtualMachinesAPI.VirtualMachinesBehavior.Instances.Store(lo.FromPtr(managedVM.ID), *managedVM)
+		ExpectSingletonReconciled(ctx, networkInterfaceGCController)
+		// We should still have a network interface here
+		nicsAfterGC, err := azureEnv.InstanceProvider.ListNics(ctx)
+		ExpectNoError(err)
+		Expect(len(nicsAfterGC)).To(Equal(1))
 
+	})
+})

pkg/fake/networkinterfaceapi.go

@@ -100,7 +100,7 @@ func (c *NetworkInterfacesAPI) BeginDelete(_ context.Context, resourceGroupName
 		InterfaceName:     interfaceName,
 	}
 	return c.NetworkInterfacesDeleteBehavior.Invoke(input, func(input *NetworkInterfaceDeleteInput) (*armnetwork.InterfacesClientDeleteResponse, error) {
-		id := mkNetworkInterfaceID(resourceGroupName, interfaceName)
+		id := mkNetworkInterfaceID(input.ResourceGroupName, input.InterfaceName)
 		c.NetworkInterfaces.Delete(id)
 		return &armnetwork.InterfacesClientDeleteResponse{}, nil
 	})

pkg/test/networkinterfaces.go

@@ -39,7 +39,7 @@ func Interface(overrides ...InterfaceOptions) *armnetwork.Interface {
 	}
 
 	nic := &armnetwork.Interface{
-		ID:       lo.ToPtr(fmt.Sprintf("/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/test-rg/providers/Microsoft.Network/networkInterfaces/%s", options.Name)),
+		ID:       lo.ToPtr(fmt.Sprintf("/subscriptions/subscriptionID/resourceGroups/test-resourceGroup/providers/Microsoft.Network/networkInterfaces/%s", options.Name)),
 		Name:     &options.Name,
 		Location: &options.Location,
 		Properties: &armnetwork.InterfacePropertiesFormat{

pkg/test/virtualmachines.go

@@ -0,0 +1,60 @@
+package test
+
+import (
+	"fmt"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute"
+	"github.com/imdario/mergo"
+	"github.com/samber/lo"
+)
+
+// VirtualMachineOptions customizes an Azure Virtual Machine for testing.
+type VirtualMachineOptions struct {
+	Name       string
+	Location   string
+	Properties *armcompute.VirtualMachineProperties
+	Tags       map[string]*string
+}
+
+// VirtualMachine creates a test Azure Virtual Machine with defaults that can be overridden by VirtualMachineOptions.
+// Overrides are applied in order, with last-write-wins semantics.
+func VirtualMachine(overrides ...VirtualMachineOptions) *armcompute.VirtualMachine {
+	options := VirtualMachineOptions{}
+	for _, o := range overrides {
+		if err := mergo.Merge(&options, o, mergo.WithOverride); err != nil {
+			panic(fmt.Sprintf("Failed to merge VirtualMachine options: %s", err))
+		}
+	}
+
+	// Provide default values if none are set
+	if options.Name == "" {
+		options.Name = RandomName("aks")
+	}
+	if options.Location == "" {
+		options.Location = "eastus"
+	}
+	if options.Properties == nil {
+		options.Properties = &armcompute.VirtualMachineProperties{}
+	}
+
+	// Construct the basic VM
+	vm := &armcompute.VirtualMachine{
+		ID:         lo.ToPtr(fmt.Sprintf("/subscriptions/subscriptionID/resourceGroups/test-resourceGroup/providers/Microsoft.Compute/virtualMachines/%s", options.Name)),
+		Name:       lo.ToPtr(options.Name),
+		Location:   lo.ToPtr(options.Location),
+		Properties: &armcompute.VirtualMachineProperties{
+			// Minimal default properties can be set here if you like:
+			// HardwareProfile: &armcompute.HardwareProfile{
+			// 	VMSize: lo.ToPtr(armcompute.VirtualMachineSizeTypesStandardDS1V2),
+			// },
+		},
+		Tags: options.Tags,
+	}
+
+	// If the user wants to override the entire VirtualMachineProperties, apply it here
+	if options.Properties != nil {
+		vm.Properties = options.Properties
+	}
+
+	return vm
+}