-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
support metrics collection in scaling config for unmanaged node groups #2055
Conversation
@saada understand that this is still in draft condition, are you planning to add the mapping between configuration struct to cloudformation for metric collection as well ? |
@sayboras , thanks for the feedback! Where can I do the mapping to cloudformation? Which directory? |
You can find it here https://github.com/weaveworks/eksctl/blob/master/pkg/cfn/builder/nodegroup.go#L263. I just have a quick look, seems like this feature will be only for un-managed node group. CF for managed nodegroup didn't support metric related attribute yet https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-eks-nodegroup-scalingconfig.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.
I left some comments but otherwise LGTM. Thanks for the PR!
9631ca4
to
10a8a3b
Compare
Can we add some tests for this? :) I think you can probably extend one of the existing tests for this. |
bb44d8a
to
933ea47
Compare
4979d1f
to
4626ece
Compare
@saada LGTM 💯 |
pkg/cfn/builder/nodegroup.go
Outdated
@@ -274,6 +274,9 @@ func nodeGroupResource(launchTemplateName *gfn.Value, vpcZoneIdentifier interfac | |||
if ng.MaxSize != nil { | |||
ngProps["MaxSize"] = fmt.Sprintf("%d", *ng.MaxSize) | |||
} | |||
if len(ng.ASGMetricsCollection) > 0 { | |||
ngProps["MetricsCollection"] = ng.ASGMetricsCollection |
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 we shouldn't couple the API schema of eksctl with the cloudformation template directly. It could happen in the future that the schema is extended or changed to a new version and that would break the backend part of eksctl.
Even though it's verbose I think the right thing to do is to recreate the CFN struct like we do in line 287 with mixedInstancesPolicy()
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.
@martina-if , can you please verify the latest changes?
183c3ff
to
9dd2817
Compare
Once rebased on master, this should pass the link checker again. |
Successfully tested this manually with the following config:
|
add name to humans.txt reset go.mod make metrics optional add cloudformation mapping regenerate fix if statement apply pr review changes apply pr review changes WIP update schema and generated deepcopy add tests and update docs cleanup test comments
9dd2817
to
b099403
Compare
Description
closes #827
Checklist
README.md
, or thesite/content
directory)area/nodegroup
), target version (e.g.version/0.12.0
) and kind (e.g.kind/improvement
)