From 34476f0ccdf0d90f6c0ce0401a1680665f0f0320 Mon Sep 17 00:00:00 2001 From: Bryce Soghigian Date: Tue, 14 Jan 2025 16:16:06 -0800 Subject: [PATCH] test: adding tests for unremovable nics --- .../nic_garbagecollection.go | 8 +- .../nodeclaim/garbagecollection/suite_test.go | 90 ++++++++++++++----- pkg/fake/networkinterfaceapi.go | 2 +- pkg/test/networkinterfaces.go | 2 +- pkg/test/virtualmachines.go | 60 +++++++++++++ 5 files changed, 135 insertions(+), 27 deletions(-) create mode 100644 pkg/test/virtualmachines.go diff --git a/pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go b/pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go index 92aa61e7d..2931765c6 100644 --- a/pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go +++ b/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 } diff --git a/pkg/controllers/nodeclaim/garbagecollection/suite_test.go b/pkg/controllers/nodeclaim/garbagecollection/suite_test.go index 4864589f5..2b14761d4 100644 --- a/pkg/controllers/nodeclaim/garbagecollection/suite_test.go +++ b/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)) + }) +}) diff --git a/pkg/fake/networkinterfaceapi.go b/pkg/fake/networkinterfaceapi.go index 1b4da8157..eb4ff67a8 100644 --- a/pkg/fake/networkinterfaceapi.go +++ b/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 }) diff --git a/pkg/test/networkinterfaces.go b/pkg/test/networkinterfaces.go index 40c10170c..d10f11701 100644 --- a/pkg/test/networkinterfaces.go +++ b/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{ diff --git a/pkg/test/virtualmachines.go b/pkg/test/virtualmachines.go new file mode 100644 index 000000000..f5de475a9 --- /dev/null +++ b/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 +}