fix: properly garbage collecting orphaned network interfaces #642

Bryce-Soghigian · 2025-01-08T18:51:31Z

Fixes #92

Description
This introduces a garbage collection controller that will remove network interfaces that are orphaned.

How was this change tested?

make test
make az-all + scale up

Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

Release Note

Properly garbage collect network interfaces.

pkg/cloudprovider/cloudprovider.go

Bryce-Soghigian · 2025-01-08T19:16:38Z

Testing this locally.

Initially after our first scaleup, we were at 810 network interfaces.

After garbage collection ran we can see that we deleted 435 network interfaces, and were left with 375 for our 375 nodes.

~/dev/focus/karpenter-provider-azure (bsoghigian/network-interface-gc*) » cat logs.txt| grep "garbage collected NIC" | wc -l sillygoose@Bryces-MacBook-Pro
435

tallaxes

Realize this is still draft to be cleaned up, so apologies if feedback is too early. Leaving some comments, overall I think I would be tempted to have a separate "networkinterface.garbagecollection" controller - unless there is a good reason not to.

pkg/controllers/nodeclaim/garbagecollection/controller.go

coveralls · 2025-01-10T22:03:32Z

Pull Request Test Coverage Report for Build 12738183194

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

100 of 190 (52.63%) changed or added relevant lines in 8 files are covered.
21 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.1%) to 95.275%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controllers/nodeclaim/garbagecollection/instance_garbagecollection.go	4	5	80.0%
pkg/providers/instance/instance.go	23	28	82.14%
pkg/providers/instance/arglist.go	42	58	72.41%
pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go	0	68	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/providers/instance/armutils.go	1	65.85%
pkg/providers/instance/instance.go	20	83.87%

Totals
Change from base Build 12661198057:	-0.1%
Covered Lines:	48191
Relevant Lines:	50581

💛 - Coveralls

pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go

pkg/test/networkinterfaces.go

pkg/providers/instance/instance.go

tallaxes

Like the separate controller! Please take a look at questions around the controller and use of cache first, everything else is minor and/or easy to fix.

pkg/cloudprovider/cloudprovider.go

pkg/test/networkinterfaces.go

pkg/test/virtualmachines.go

pkg/test/networkinterfaces.go

pkg/fake/azureresourcegraphapi.go

pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go

tallaxes · 2025-01-20T07:24:53Z

pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go

+			if err != nil {
+				logging.FromContext(ctx).Error(err)
+				return
+			}


Instance GC controller combines the errors and returns them, here they are only logged an never returned. Why the difference in behaviour?

I tried looking into the original pr to see if we got any rationality behind that: aws/karpenter-provider-aws#3405

Didn't see much.

We shouldn't be requeuing more frequently in the presence of errors. We should probably change that, if VM Deletion fails due to a TooManyRequests error, we can't retry until the quota window resets(After 5 minutes?)

2m might make some sense for vms as the volume of vms that need garbage collection is in most cases significantly lower than the volume of interfaces that need garbage collection.

We could try requeuing depending on the kind of error at different intervals.

If we see TooManyRequests Error wait 5 minutes to retry, if we see IsNicReservedForAnotherVM retry 3 minutes later?

Overall it seems simpler to just not do all of that requeuing and come and optimize it later if we are optimizing for performance. For now just requeuing every 5 minutes seems to be fine.

Appreciate the explanation. Proper treatment of retries and rate limiting is not trivial, let's not tackle it here. I am Ok with keeping constant retry period here for now.

pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go

johnthompson-ybor · 2025-01-21T15:02:50Z

I'd really love to see this merged, this is causing regular outages for us.

pkg/controllers/nodeclaim/garbagecollection/instance_garbagecollection.go

Bryce-Soghigian · 2025-01-28T02:14:32Z

Ci here is failing due to vulncheck: #665 should fix it

…tor of arg related functions to their own file

…ctoring gc functions slightly

…fix tests

…ate between them to prevent conflicts in nic deletion calls

…impl

…ontroller to avoid attempts to delete nics that are managed by a vm

…cleaner state list

…llection.go

…lection tests

…eating nodeclaims with empty required properties

…late vm gc

…py case

…chine and test.NetworkInterface

…name

Bryce-Soghigian · 2025-01-30T00:44:19Z

Marked resolved with no comment or action. You feel strongly about keeping this? Or plan to address later?
I removed the uses but seem to have forgotten to remove the error itself. Will get that in.

tallaxes

Approving with suggestions

pkg/test/networkinterfaces.go

tallaxes · 2025-01-30T00:55:33Z

pkg/controllers/nodeclaim/garbagecollection/suite_test.go

+		nic := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})
+		nic2 := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})


If there is a reason to test two - please capture it in a comment

tallaxes · 2025-01-30T01:01:59Z

pkg/controllers/nodeclaim/garbagecollection/suite_test.go

+		ExpectApplied(ctx, env.Client, nodeClaim)
+
+		// Create a managed NIC
+		nic := test.Interface(test.InterfaceOptions{Name: instance.GenerateResourceName(nodeClaim.Name), NodepoolName: nodePool.Name})


Yes I suggested elsewhere to collapse these for readability, but I meant (and included in the example) only test.Interface(test.InterfaceOptions{ part, like this:

Suggested change

nic := test.Interface(test.InterfaceOptions{Name: instance.GenerateResourceName(nodeClaim.Name), NodepoolName: nodePool.Name})

nic := test.Interface(test.InterfaceOptions{

Name: instance.GenerateResourceName(nodeClaim.Name),

NodepoolName: nodePool.Name,

})

This is the pattern that we use in many other places for readability. The rationale is that the type names are usually boilerplate, while the field values are usually of interest.

tallaxes · 2025-01-30T01:11:51Z

pkg/controllers/nodeclaim/garbagecollection/suite_test.go

+		Expect(len(nicsAfterGC)).To(Equal(1))
+
+	})
+	It("the vm gc controller should remove the nic if there is an associated vm", func() {


Had to read the code to understand what it does; maybe belongs under VM GC test section? Or just a better description:

Suggested change

It("the vm gc controller should remove the nic if there is an associated vm", func() {

It("the VM GC controller should also delete the associated NIC", func() {

tallaxes · 2025-01-30T01:18:40Z

pkg/providers/instance/instance.go

+var (
+	vmListQuery  string
+	nicListQuery string
+)


This is probably a leftover from VM GC implementation, but not clear to me why these need to be global rather than DefaultProvider fields

tallaxes · 2025-01-30T01:22:16Z

pkg/providers/instance/suite_test.go

+	})
+	It("should only list nics that belong to karpenter", func() {
+		managedNic := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})
+		unmanagedNic := test.Interface(test.InterfaceOptions{Tags: map[string]*string{"kubernetes.io/cluster/test-cluster": lo.ToPtr("random-aks-vm")}})


Same comment about the degree of collapsing as elsewhere ...

tallaxes · 2025-01-30T01:28:52Z

pkg/test/virtualmachines.go

+// VirtualMachineOptions customizes an Azure Virtual Machine for testing.
+type VirtualMachineOptions struct {
+	Name         string
+	NodepoolName string


Having VM - even test VM - or interface be internally aware of the Nodepool is a bad smell. I think I see why you are doing this - to handle tags automatically - but that pushes even more higher level logic into the test helpers. Overall, I don't think this is an improvement.

Ok, I see now this was maybe in response to my question

What happens if options.Tags are not passed?

I was just asking about whether it would be tolerated / won't crash. Did not mean to suggest they should be calculated automatically (and definetely not at the expense of having to pass nodepool name). Will be more clear next time.

* feat: adding ListNics to instanceprovider interface alongside a refactor of arg related functions to their own file * feat: adding garbage collection logic for network interfaces and refactoring gc functions slightly * feat: working poc of ARG Queries and nic garbage collection, need to fix tests * fix: tests failing due to ListVM * feat: split nic and vm into their own gc controllers, added shared state between them to prevent conflicts in nic deletion calls * feat: Add DeleteNic option to instance provider * docs(code-comment): adding clarification to unremovableNics * test: instanceprovider.ListNics barebones test and arg fake list nic impl * fix: bootstrap.sh * feat: adding in VM List into the networkinterface.garbagecollection controller to avoid attempts to delete nics that are managed by a vm * refactor: unremovableNics out of the vm gc controller in favor for a cleaner state list * fix:updating references to cache * Update pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go * test: adding composable network interface options to our test utils based on existing karp-core pattern * test: adding test we don't include unmanaged nics * test: adding network garbage collection suite and happy path * test: adding tests for unremovable nics * test: adding coverage that vm controller cleans up nics * refactor: renaming controller * fix: refactor name * refactor: using import directly * ci: checking error for controller * fix: ci * fix: addressing comments * Update pkg/controllers/nodeclaim/garbagecollection/instance_garbagecollection.go * refactor: removing name constant * refactor: moving to test utils * fix: removing GetZoneID * style: improving the readability of the network interface garbage collection tests * revert: removing lo.FromPtr checks for nodeclaim creation to avoid creating nodeclaims with empty required properties * refactor: VirtualmachineProperties needs a default for time created * fix: modifying the tests to be aware of time created to properly simulate vm gc * refactor: added nodepoolName to test.VirtualMachine and test.Interface * refactor: moving vm gc tests to use test.VirtualMachine * refactor: renaming arg files to azureresourcegraph * refactor: using deleteIfNicExists directly in cleanupAzureResources * test: createNicFromQueryResponseData missing id, missing name and happy case * refactor: using fake.Region for the default region for test.VirtualMachine and test.NetworkInterface * fix: using input.InterfaceName rather than the outer scope interface name * test: modifying fake to only initialize the query once

Bryce-Soghigian commented Jan 8, 2025

View reviewed changes

pkg/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved

tallaxes reviewed Jan 9, 2025

View reviewed changes

Bryce-Soghigian commented Jan 12, 2025

View reviewed changes

pkg/controllers/nodeclaim/garbagecollection/nic_garbagecollection.go Outdated Show resolved Hide resolved

tallaxes assigned Bryce-Soghigian Jan 14, 2025

tallaxes added area/networking Issues or PRs related to networking area/provisioning Issues or PRs related to provisioning (instance provider) area/error-handling Issues or PRs related to handling of errors labels Jan 14, 2025

Bryce-Soghigian commented Jan 15, 2025

View reviewed changes

pkg/test/networkinterfaces.go Outdated Show resolved Hide resolved

Bryce-Soghigian marked this pull request as ready for review January 15, 2025 23:52

Bryce-Soghigian force-pushed the bsoghigian/network-interface-gc branch from 9eb0362 to 20c6625 Compare January 16, 2025 10:40

Bryce-Soghigian requested a review from tallaxes January 17, 2025 03:59

Bryce-Soghigian commented Jan 19, 2025

View reviewed changes

pkg/providers/instance/instance.go Show resolved Hide resolved

tallaxes reviewed Jan 20, 2025

View reviewed changes

Bryce-Soghigian commented Jan 25, 2025

View reviewed changes

pkg/controllers/nodeclaim/garbagecollection/instance_garbagecollection.go Show resolved Hide resolved

Bryce-Soghigian and others added 12 commits January 28, 2025 02:37

feat: adding ListNics to instanceprovider interface alongside a refac…

5d7853b

…tor of arg related functions to their own file

feat: adding garbage collection logic for network interfaces and refa…

4c3e8df

…ctoring gc functions slightly

feat: working poc of ARG Queries and nic garbage collection, need to …

3a77d94

…fix tests

fix: tests failing due to ListVM

d595df5

feat: split nic and vm into their own gc controllers, added shared st…

4ad1738

…ate between them to prevent conflicts in nic deletion calls

feat: Add DeleteNic option to instance provider

d066fc5

docs(code-comment): adding clarification to unremovableNics

c71f4dc

test: instanceprovider.ListNics barebones test and arg fake list nic …

900c48d

…impl

fix: bootstrap.sh

cce9f3c

feat: adding in VM List into the networkinterface.garbagecollection c…

d7eb78c

…ontroller to avoid attempts to delete nics that are managed by a vm

refactor: unremovableNics out of the vm gc controller in favor for a …

97db74f

…cleaner state list

fix:updating references to cache

8867815

Bryce-Soghigian and others added 16 commits January 28, 2025 02:37

test: adding tests for unremovable nics

85d84f5

test: adding coverage that vm controller cleans up nics

c495b97

refactor: renaming controller

176c7f8

fix: refactor name

d8fa55b

refactor: using import directly

a02263c

ci: checking error for controller

41319bf

fix: ci

e300439

fix: addressing comments

8418208

Update pkg/controllers/nodeclaim/garbagecollection/instance_garbageco…

ad84b51

…llection.go

refactor: removing name constant

aaccc95

refactor: moving to test utils

fa14e9a

fix: removing GetZoneID

fdf3147

style: improving the readability of the network interface garbage col…

730b754

…lection tests

revert: removing lo.FromPtr checks for nodeclaim creation to avoid cr…

fe0c4a0

…eating nodeclaims with empty required properties

refactor: VirtualmachineProperties needs a default for time created

3d0b0b9

fix: modifying the tests to be aware of time created to properly simu…

c0e4043

…late vm gc

Bryce-Soghigian force-pushed the bsoghigian/network-interface-gc branch from e1c79e5 to c0e4043 Compare January 28, 2025 02:37

Bryce-Soghigian added 8 commits January 28, 2025 02:58

refactor: added nodepoolName to test.VirtualMachine and test.Interface

50d9455

refactor: moving vm gc tests to use test.VirtualMachine

f4f2e59

refactor: renaming arg files to azureresourcegraph

cfa1776

refactor: using deleteIfNicExists directly in cleanupAzureResources

0ea44f4

test: createNicFromQueryResponseData missing id, missing name and hap…

d9d858a

…py case

refactor: using fake.Region for the default region for test.VirtualMa…

d631e19

…chine and test.NetworkInterface

fix: using input.InterfaceName rather than the outer scope interface …

9d49d25

…name

test: modifying fake to only initialize the query once

38666ed

tallaxes approved these changes Jan 30, 2025

View reviewed changes

Bryce-Soghigian merged commit 7563bca into main Jan 30, 2025
15 of 16 checks passed

Bryce-Soghigian deleted the bsoghigian/network-interface-gc branch January 30, 2025 01:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: properly garbage collecting orphaned network interfaces #642

fix: properly garbage collecting orphaned network interfaces #642

Bryce-Soghigian commented Jan 8, 2025 •

edited

Loading

Bryce-Soghigian commented Jan 8, 2025

tallaxes left a comment •

edited

Loading

coveralls commented Jan 10, 2025 •

edited

Loading

tallaxes left a comment

tallaxes Jan 20, 2025

Bryce-Soghigian Jan 28, 2025 •

edited

Loading

Bryce-Soghigian Jan 28, 2025

tallaxes Jan 29, 2025

johnthompson-ybor commented Jan 21, 2025

Bryce-Soghigian commented Jan 28, 2025

Bryce-Soghigian commented Jan 30, 2025

tallaxes left a comment

tallaxes Jan 30, 2025

tallaxes Jan 30, 2025 •

edited

Loading

tallaxes Jan 30, 2025

tallaxes Jan 30, 2025

tallaxes Jan 30, 2025

tallaxes Jan 30, 2025 •

edited

Loading

		nic := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})
		nic2 := test.Interface(test.InterfaceOptions{NodepoolName: nodePool.Name})

	It("the vm gc controller should remove the nic if there is an associated vm", func() {
	It("the VM GC controller should also delete the associated NIC", func() {

fix: properly garbage collecting orphaned network interfaces #642

fix: properly garbage collecting orphaned network interfaces #642

Conversation

Bryce-Soghigian commented Jan 8, 2025 • edited Loading

Bryce-Soghigian commented Jan 8, 2025

tallaxes left a comment • edited Loading

Choose a reason for hiding this comment

coveralls commented Jan 10, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12738183194

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

tallaxes left a comment

Choose a reason for hiding this comment

tallaxes Jan 20, 2025

Choose a reason for hiding this comment

Bryce-Soghigian Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Bryce-Soghigian Jan 28, 2025

Choose a reason for hiding this comment

tallaxes Jan 29, 2025

Choose a reason for hiding this comment

johnthompson-ybor commented Jan 21, 2025

Bryce-Soghigian commented Jan 28, 2025

Bryce-Soghigian commented Jan 30, 2025

tallaxes left a comment

Choose a reason for hiding this comment

tallaxes Jan 30, 2025

Choose a reason for hiding this comment

tallaxes Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

tallaxes Jan 30, 2025

Choose a reason for hiding this comment

tallaxes Jan 30, 2025

Choose a reason for hiding this comment

tallaxes Jan 30, 2025

Choose a reason for hiding this comment

tallaxes Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Bryce-Soghigian commented Jan 8, 2025 •

edited

Loading

tallaxes left a comment •

edited

Loading

coveralls commented Jan 10, 2025 •

edited

Loading

Bryce-Soghigian Jan 28, 2025 •

edited

Loading

tallaxes Jan 30, 2025 •

edited

Loading

tallaxes Jan 30, 2025 •

edited

Loading