Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner #505

sushrk · 2024-12-24T23:52:04Z

Description of changes:

PR 2, adding support for Centralize leaked ENI cleanup in the controller to delete leaked ENIs provisioned by the controller and VPC-CNI:

Refactor periodic cleanup routine and add support for node termination cleaner. The node termination is invoked upon a node deletion from the CNINode controller as part of the finalizer routine. The finalizer was added in PR 1 on CNINode objects.
Added metrics for leaked ENIs
Add support for DisassociateTrunkInterface, call DisassociateTrunkInterface to remove association between branch and trunk ENI before deleting the branch ENI as per recommendation by EC2 team.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…termination cleaner

yash97 · 2025-01-07T19:38:16Z

pkg/aws/ec2/api/cleanup/eni_cleanup.go

+func (e *ClusterENICleaner) Start(ctx context.Context) error {
+	e.Log.Info("starting eni clean up routine")
+
+	// Start routine to listen for shut down signal, on receiving the signal it set shutdown to true


don't think we need this comments which just describes code.

Makes sense, this was just copied over from the existing eni_cleanup file.

yash97 · 2025-01-07T20:02:13Z

pkg/aws/ec2/api/wrapper.go

+	VpcCniAvailableClusterENICnt = prometheus.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "vpc_cni_created_available_eni_count",
+			Help: "The number of available ENIs created by VPC-CNI that will tried to be deleted by the controller",


can we rename this to vpc_cni_created_pending_deletion_eni_count?. here it looks like just the number of eni created by vpc_cni.

available here indicates the state of the network interface created by the corresponding component. "pending delete" is not accurate as the ENI may not be necessarily deleted if it is re-attached to an instance.

yash97 · 2025-01-07T20:03:53Z

pkg/aws/ec2/api/wrapper.go

+	VpcRcAvailableClusterENICnt = prometheus.NewGauge(
+		prometheus.GaugeOpts{
+			Name: "vpc_rc_created_available_eni_count",
+			Help: "The number of available ENIs created by VPC-RC that will tried to be deleted by the controller",


yash97 · 2025-01-08T05:52:51Z

pkg/aws/ec2/api/cleanup/eni_cleanup.go

+	}()
+	// Perform ENI cleanup after fixed time intervals till shut down variable is set to true on receiving the shutdown
+	// signal
+	for !e.shutdown {


This can still result in race condition.
Sequence for loop check can happen first and then shutdown gets sets to true.
It would be better if we use for select pattern here.

yash97 · 2025-01-08T06:24:38Z

pkg/aws/ec2/api/cleanup/eni_cleanup.go

+				})
+				if err != nil {
+					if !strings.Contains(err.Error(), ec2Errors.NotFoundInterfaceID) { // ignore InvalidNetworkInterfaceID.NotFound error
+						// append err and continue, we will retry deletion in the next period/reconcile


hopefully we move to comparing error type in sdkgo v2

yash97 · 2025-01-08T06:26:51Z

pkg/aws/ec2/api/cleanup/eni_cleanup.go

+					if !strings.Contains(err.Error(), ec2Errors.NotFoundInterfaceID) { // ignore InvalidNetworkInterfaceID.NotFound error
+						// append err and continue, we will retry deletion in the next period/reconcile
+						leakedENICount += 1
+						errors = append(errors, fmt.Errorf("failed to delete leaked network interface %v:%v", *nwInterface.NetworkInterfaceId, err))


This will count eni that are being re tried too.

Can you expand on this? In each periodic cleanup routine, this is the number of leaked ENIs that failed to be deleted.

If these ENIs are still in leaked state, then yes, deletion will be re-tried on the next cleanup. And if it still fails deletion, it will be counted as leaked ENI which is the intended behavior. Do you have any concerns on this?

ok I see we set metric instead of addition after every run. We should be good then. It will just count leaked eni for particular run only.

haouc · 2025-01-08T17:34:00Z

pkg/provider/branch/provider.go

@@ -238,12 +238,14 @@ func (b *branchENIProvider) ProcessAsyncJob(job interface{}) (ctrl.Result, error

 // DeleteNode deletes all the cached branch ENIs associated with the trunk and removes the trunk from the cache.
 func (b *branchENIProvider) DeleteNode(nodeName string) (ctrl.Result, error) {
-	trunkENI, isPresent := b.getTrunkFromCache(nodeName)
+	_, isPresent := b.getTrunkFromCache(nodeName)
 	if !isPresent {
 		return ctrl.Result{}, fmt.Errorf("failed to find node %s", nodeName)


Will this error be requeued? If yes, I wonder if this is a recoverable error?

It will be re-queued since it returns an error, but jobs are re-tried max 5 times and then forgotten.

I wonder if this is a recoverable error

That's a good point, if the trunk is removed from cache, it will not be added back unless the node is initialized again. Will check if this is a valid case. Btw this is existing logic, the only change is we omit calling DeleteAllBranchENIs in this workflow as finalizer routine will delete the ENIs on node termination.

haouc · 2025-01-08T17:44:01Z

pkg/aws/ec2/api/wrapper.go

@@ -822,3 +909,19 @@ func (e *ec2Wrapper) getRegionalStsEndpoint(partitionID, region string) (endpoin
 	}
 	return res, nil
 }
+
+func (e *ec2Wrapper) DisassociateTrunkInterface(input *ec2.DisassociateTrunkInterfaceInput) error {


hmm, the naming is a bit confusing. Looks like it is about disassociating branch interface from trunk interface?

That's right, this is the API call to remove association between branch and trunk network interfaces. Adding this per EC2 team recommendation to call DisassociateTrunkInterface before deleting the ENIs.

haouc · 2025-01-08T17:46:30Z

pkg/provider/branch/trunk/trunk.go

@@ -126,6 +124,8 @@ type trunkENI struct {
 	uidToBranchENIMap map[string][]*ENIDetails
 	// deleteQueue is the queue of ENIs that are being cooled down before being deleted
 	deleteQueue []*ENIDetails
+	// nodeName tag is the tag added to trunk and branch ENIs created on the node
+	nodeNameTag []*awsEC2.Tag


Can you remind me why we need node name tag for trunk ENI?

In the node termination cleaner (finalizer routine), we use the tag name to reduce the scope of ENIs to delete per node.

Per offline discussion, this should be updated to use subnet IDs instead and we can remove the nodeName tag.

haouc · 2025-01-08T17:49:11Z

pkg/aws/ec2/api/wrapper.go

+	start := time.Now()
+	// Using the instance role
+	_, err := e.instanceServiceClient.DisassociateTrunkInterface(input)
+	ec2APICallLatencies.WithLabelValues("disassociate_branch_from_trunk").Observe(timeSinceMs(start))


We should be able to get SDK's metrics for free. Maybe a feature for v2?

haouc · 2025-01-08T19:40:09Z

main.go

-		VPCID:       vpcID,
-	}).SetupWithManager(ctx, mgr, healthzHandler); err != nil {
+	}
+	cleaner.ENICleaner = &eniCleaner.ENICleaner{


Is cluster ENICleaner for cluster wide 30 minutes cleanup? I am trying to understand the pattern here.

That is right. ClusterENICleaner is used in the periodic cleanup routine.

In the next PR, I will add the NodeTerminationCleaner which is used in the finalizer routine.

sushrk force-pushed the eni-cleanup branch from 7053082 to a94f4ed Compare December 24, 2024 23:56

Centralized leaked ENI cleanup- refactor periodic cleanup & add node …

ef0591a

…termination cleaner

sushrk force-pushed the eni-cleanup branch from a94f4ed to ef0591a Compare December 25, 2024 00:01

sushrk marked this pull request as ready for review December 26, 2024 22:02

sushrk requested a review from a team as a code owner December 26, 2024 22:02

sushrk requested a review from yash97 January 2, 2025 08:46

yash97 reviewed Jan 7, 2025

View reviewed changes

yash97 reviewed Jan 8, 2025

View reviewed changes

haouc reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner #505

Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner #505

sushrk commented Dec 24, 2024 •

edited

Loading

yash97 Jan 7, 2025

sushrk Jan 8, 2025

yash97 Jan 7, 2025

sushrk Jan 8, 2025

yash97 Jan 7, 2025

yash97 Jan 8, 2025 •

edited

Loading

yash97 Jan 8, 2025

yash97 Jan 8, 2025

sushrk Jan 8, 2025

yash97 Jan 10, 2025 •

edited

Loading

haouc Jan 8, 2025

sushrk Jan 9, 2025 •

edited

Loading

haouc Jan 8, 2025

sushrk Jan 8, 2025

haouc Jan 8, 2025

sushrk Jan 9, 2025

haouc Jan 8, 2025

haouc Jan 8, 2025

sushrk Jan 9, 2025

Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner #505

Are you sure you want to change the base?

Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner #505

Conversation

sushrk commented Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yash97 Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yash97 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushrk Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushrk commented Dec 24, 2024 •

edited

Loading

yash97 Jan 8, 2025 •

edited

Loading

yash97 Jan 10, 2025 •

edited

Loading

sushrk Jan 9, 2025 •

edited

Loading