-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Karpenter Addon: adding AMI Selector, refactor to use NodeTemplate #736
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Could we also update the doc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@shapirov103 FYI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of tactical/code cleanliness comments.
lib/addons/karpenter/index.ts
Outdated
/** | ||
* Taints for the provisioned nodes - Taints may prevent pods from scheduling if they are not tolerated by the pod. | ||
*/ | ||
taints?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use this type https://docs.aws.amazon.com/cdk/api/v1/docs/@aws-cdk_aws-eks.TaintSpec.html?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't work - the provisioner CRD doesn't recognize TaintEffect interface and will cause an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed to only change the API layer for convenience. The backend will remain the same, you will need to convert TaintEffect to string. However, this functionality is already released, so my only comment here is: let's extracts the TaintType, put it in the spi types and reuse in the current form.
lib/addons/karpenter/index.ts
Outdated
/** | ||
* Labels applied to all nodes | ||
*/ | ||
labels?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have type Values for this and annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed.
/do-e2e-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end to end tests passed
8847e02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please see my comment on taints.
lib/addons/karpenter/index.ts
Outdated
/** | ||
* Taints for the provisioned nodes - Taints may prevent pods from scheduling if they are not tolerated by the pod. | ||
*/ | ||
taints?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed to only change the API layer for convenience. The backend will remain the same, you will need to convert TaintEffect to string. However, this functionality is already released, so my only comment here is: let's extracts the TaintType, put it in the spi types and reuse in the current form.
addressed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue #, if available: #726
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.