ci: Clean leaked VPC-resource-controller ENIs
#6158
Conversation
✅ Deploy Preview for karpenter-docs-prod ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Pull Request Test Coverage Report for Build 8970315796Details
💛 - Coveralls |
| @@ -154,3 +160,59 @@ func (e *ENI) Cleanup(ctx context.Context, ids []string) ([]string, error) { | |||
|
|
|||
| return deleted, errs | |||
| } | |||
|
|
|||
| // get-vpc-resource-controller-enis is to get leaked ENIs by the vpc-resource-controller | |||
There was a problem hiding this comment.
Is it created by the VPC resource controller or the VPC CNI?
| Values: []string{fmt.Sprintf("kubernetes.io/cluster/%s", lo.FromPtr(clusterName.Value))}, | ||
| }, | ||
| { | ||
| Name: lo.ToPtr("tag:eks:eni:owner"), |
There was a problem hiding this comment.
Which testing did you see NICs with this tag get created under? AFAIK, NICs with this tag are only created when using security groups for pods and branch ENIs (which I don't think we do in our testing). I thought the tags that the VPC CNI added for its NICs were: node.k8s.amazonaws.com/instance_id and node.k8s.amazonaws.com/createdAt
| return *tag.Key == karpenterTestingTag | ||
| }) | ||
| if found { | ||
| if slices.Contains(excludedClusters, lo.FromPtr(clusterName.Value)) { |
There was a problem hiding this comment.
Separate: Do we have a TODO comment around these excluded clusters to ensure that we eventually move this to a tagging mechanism? Minimally, do we have something that's tracked on GH?
| } | ||
|
|
||
| stacks := lo.Reject(out.Stacks, func(s cloudformationtypes.Stack, _ int) bool { | ||
| return s.StackStatus == cloudformationtypes.StackStatusDeleteComplete || |
There was a problem hiding this comment.
Why are we checking for the stack first here? Not immediately apparent. If it's needed, should we add a comment?
|
|
||
| // get-vpc-resource-controller-enis is to get leaked ENIs by the vpc-resource-controller | ||
| // Issue: https://github.com/aws/karpenter-provider-aws/issues/5582 | ||
| func (e *ENI) getVpcResourceControllerENIs(ctx context.Context, expirationTime time.Time, excludedClusters []string) (ids []string, err error) { |
There was a problem hiding this comment.
Are these ENIs specifically tied to the VPC resource controller or are these just ENIs that were launched by Karpenter's nodes with the VPC CNI? Is there a reason that we need to clean these up differently than our primary ENIs? I believe the VPC CNI has config that allows you to add tags to these ENIs. We could just add tags and then re-leverage our existing ENI cleanup if we wanted.
Fixes #N/A
Description
vpc-resource-controllerHow was this change tested?
Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.