-
Notifications
You must be signed in to change notification settings - Fork 629
✨ ROSA: Add ROSA-HCP AutoNode for karpenter auto scale #5686
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
Conversation
default: disable | ||
description: AutoNode mode allowed values are enable & disable | ||
enum: | ||
- enable |
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.
@serngawy enabled
and disabled
are the allowed values.
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.
the enum values are enabled & disabled. Changed it to match the values.
description: |- | ||
AutoNode role ARN that has the IAM Policy and cluster-specific Role that gives permissions to the Karpenter controller. | ||
IAM policy should be as below AND the role must be attached with the same OIDC-ID that is used with the ROSA-HCP cluster. | ||
{ |
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.
@serngawy The Policy for the IAM role is highly likely to change. For the private preview we are giving this to customers via documentation, so I would suggest not to embed it here.
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.
okay
AutoNode *AutoNode `json:"autoNode,omitempty"` | ||
} | ||
|
||
// AutoNode set the auto nde mode and auto node role ARN. |
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.
@serngawy set the AutoNode mode and AutoNode role ARN
If not set, audit log forwarding is disabled. | ||
type: string | ||
autoNode: | ||
description: AutoNode set the autoNode mode and autoNode role ARN. |
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.
@serngawy Minor typo here "A"utoNode rather than "a"utoNode.
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.
done, thanks
/label tide/merge-method-squash |
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.
Left some API suggestions. Thanks
// AutoNode set the AutoNode mode and AutoNode role ARN. | ||
// +optional | ||
AutoNode *AutoNode `json:"autoNode,omitempty"` |
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 think this doesn't need to be a pointer right?
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.
pointer is easier to check struct value.
// Test case 7: AutoNode update | ||
t.Run("Update Auto Node", func(t *testing.T) { | ||
rosaControlPlane := &rosacontrolplanev1.ROSAControlPlane{ | ||
Spec: rosacontrolplanev1.RosaControlPlaneSpec{ | ||
AutoNode: &rosacontrolplanev1.AutoNode{ | ||
Mode: rosacontrolplanev1.Enabled, | ||
RoleARN: "autoNodeARN", | ||
}, | ||
}, | ||
} | ||
|
||
mockCluster, _ := v1.NewCluster(). | ||
AutoNode(v1.NewClusterAutoNode().Mode("disabled")). | ||
AWS(v1.NewAWS().AutoNode(v1.NewAwsAutoNode().RoleArn("anyARN"))). | ||
Build() | ||
|
||
expectedOCMSpec := ocm.Spec{ | ||
AutoNodeMode: "enabled", | ||
AutoNodeRoleARN: "autoNodeARN", | ||
} | ||
|
||
reconciler := &ROSAControlPlaneReconciler{} | ||
ocmSpec, updated := reconciler.updateOCMClusterSpec(rosaControlPlane, mockCluster) | ||
|
||
g.Expect(updated).To(BeTrue()) | ||
g.Expect(ocmSpec).To(Equal(expectedOCMSpec)) | ||
}) |
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.
Once we have polished the API above let's define unit tests for the various cases where things are left empty.
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.
add more test for ValidateControlPlaneSpec below
/assign @nrb @damdo @richardcase |
/cherry-pick release-2.9 |
@damdo: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/test pull-cluster-api-provider-aws-test |
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 ran KAL against this PR and it found some issues that I've marked inline.
I used the CAPI KAL configuration for the analysis.
// +optional | ||
ClusterRegistryConfig *RegistryConfig `json:"clusterRegistryConfig,omitempty"` | ||
|
||
// AutoNode set the AutoNode mode and AutoNode role ARN. |
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.
Since this will be exposed as a YAML field, the doc should start with autoNodeMode
, rather than the usual Go convention.
Signed-off-by: serngawy <[email protected]>
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
Thanks @serngawy
LGTM label has been added. Git tree hash: ad84c2beaeb2f73718631eee048733984c56952b
|
@richardcase @nrb if you are happy with the latest changes, let's try and get this in :) TY |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: richardcase The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@damdo: #5686 failed to apply on top of branch "release-2.9":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/cherry-pick release-2.9 |
@serngawy: new pull request created: #5699 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
What this PR does / why we need it:
Adding auto node config for karpenter feature
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist:
Release note: