fix: unify nodeclass status and termination controllers to prevent ra… #7597
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ce conditions from leaking instance profiles As part of a recent investigation, I found that the nodeclass status and termination controllers race against each other at times, and depending on who wins the race, the instance profile can be leaked. Discussed options with the team and merging these two controllers is the most straightforward thing to do at the moment. All other solutions are not a 100% failsafe with the exception of adding new finalizers for instance profiles but they have their own issues due to being backwards incompatible. The primary downside of this change is that nodeclaim delete events can trigger a reconciliation now if the nodeclass is not deleted/deleting but looking through the current list of reconcilers, the impact of that should be minimal since all we're doing is making a few additional calls to EC2 for subnets and security groups and the additional calls for Instance Profile reconciliation only when nodeclaims are getting deleted.
✅ 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 12776979373Details
💛 - Coveralls |
|
Snapshot successfully published to |
engedaam
reviewed
Jan 14, 2025
…inations controllers are merged into a single nodeclass controller for 1.2.0+
saurav-agarwalla
force-pushed
the
instance-profile-leak
branch
from
6935fcb to
c45d481
Compare
|
Snapshot successfully published to |
edibble21
pushed a commit
to edibble21/karpenter-provider-aws
that referenced
this pull request
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #N/A
Description
As part of a recent investigation, I found that the nodeclass status and termination controllers race against each other at times, and depending on who wins the race, the instance profile can be leaked. Discussed options with the team and merging these two controllers is the most straightforward thing to do at the moment. All other solutions are not a 100% failsafe with the exception of adding new finalizers for instance profiles but they have their own issues due to being backwards incompatible.
The primary downside of this change is that nodeclaim delete events can trigger a reconciliation now if the nodeclass is not deleted/deleting but looking through the current list of reconcilers, the impact of that should be minimal since all we're doing is making a few additional calls to EC2 for subnets and security groups and the additional calls for Instance Profile reconciliation only when nodeclaims are getting deleted.
How was this change tested?
make presubmitand E2E testing via CI as part of this PR.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.