Skip to content

Conversation

@cartermckinnon
Copy link
Contributor

Description of changes:

Adds support for creating unmanaged nodegroup(s).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cartermckinnon cartermckinnon force-pushed the unmanaged-nodegroups branch 2 times, most recently from 777d772 to f9be768 Compare March 1, 2025 00:28
@cartermckinnon cartermckinnon changed the title [WIP] Add unmanaged nodegroup support Add unmanaged nodegroup support Mar 4, 2025
shvbsle
shvbsle previously approved these changes Mar 20, 2025
Copy link
Contributor

@shvbsle shvbsle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hakuna-matatah
Copy link
Contributor

Has this been tested on experimental infra ?

@shvbsle shvbsle dismissed their stale review March 24, 2025 21:35

modifications may be needed before review

Copy link
Contributor

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need a teardown task as well to clean up the ASG

echo "$node_group_name is "CREATING" at $(date)"
sleep 2
done
# TODO: do this for unmanaged nodes as well
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the TODO is for unmanaged node group, we should add it in the else clause below.

kubectl apply -f aws-auth-cm.yaml
echo "Created aws-auth ConfigMap"
# Wait for the config map to be ready
echo "Verifying aws-auth ConfigMap..."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kubectl apply exit with 0, then the aws-auth CM should have been applied successfully.
Is there a scenario that is not covered by that?

--arg SecurityGroup "$(jq -r '.cluster.resourcesVpcConfig.clusterSecurityGroupId' cluster.json)" \
--arg VpcId $(jq -r '.cluster.resourcesVpcConfig.vpcId' cluster.json) \
'$ARGS.named | to_entries | map({"ParameterKey": .key, "ParameterValue": .value})' \
> parameters.json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to cat this file if it's not super long.

@mengqiy
Copy link
Contributor

mengqiy commented Apr 2, 2025

My comments don't have to be blocking. I'm OK merging this as is and address the comments in a follow-up PR.

@hakuna-matatah
Copy link
Contributor

LGTM

@mengqiy mengqiy merged commit 8b25d8a into awslabs:main Apr 2, 2025
4 checks passed
shvbsle added a commit that referenced this pull request Apr 4, 2025
mengqiy pushed a commit that referenced this pull request Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants