Skip to content
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

Remove InstancesDistribution from unmanaged nodegroup cfn #448

Merged
merged 3 commits into from
Jun 27, 2024

Conversation

ndbaker1
Copy link
Contributor

@ndbaker1 ndbaker1 commented Jun 12, 2024

Issue #, if available:

N/A

Description of changes:

Remove InstancesDistribution from unmanaged nodegroup cloudformation templates since it may not be supported in certain regions.

Testing:

# built container
docker build -f Dockerfile.kubetest2 . --build-arg=KUBERNETES_MINOR_VERSION=1.29 -t kubetest2-1.29
docker run --rm -it kubetest2-1.29

# from kubetest2 image context, run test on amazon/amazon-eks-node-al2023-x86_64-standard-1.29-v20240615
AWS_REGION=us-west-2 \
kubetest2 eksapi \
--up \
--down \
--generate-ssh-key \
--expected-ami=ami-03cda10c26c283085 \
--addons=vpc-cni:latest \
--tune-vpc-cni \
--node-name-strategy=EC2PrivateDNSName \
--user-data-format=nodeadm \
--unmanaged-nodes \
--ami=ami-03cda10c26c283085 \
--test=multi \
-- \
--fail-fast=false \
-- \
ginkgo \
--use-binaries-from-path \
--parallel=6 \
--ginkgo-args='--no-color' \
--focus-regex="\\[Conformance\\]" \
--skip-regex="\\[Serial\\]|\\[Disruptive\\]|\\[Slow\\]|Garbage.collector" \
-- \
ginkgo \
--use-binaries-from-path \
--parallel=1 \
--flake-attempts=3 \
--ginkgo-args='--no-color' \
--focus-regex="(\\[Serial\\]|\\[Disruptive\\]|\\[Slow\\]|Garbage.collector).*\\[Conformance\\]" \
--skip-regex=Daemon.set.+should.rollback.without.unnecessary.restarts

...

Ran 40 of 7407 Specs in 2851.458 seconds
SUCCESS! -- 40 Passed | 0 Failed | 0 Pending | 7367 Skipped
PASS

Ginkgo ran 1 suite in 47m32.351613431s
Test Suite Passed

@@ -317,8 +317,10 @@ Resources:
Properties:
AutoScalingGroupName: !Ref ResourceId
MixedInstancesPolicy:
{{- if .Features.InstancesDistribution }}
Copy link
Member

Choose a reason for hiding this comment

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

do we even need the InstancedDistribution? I just copied this from an unmanaged nodegroup example to begin with, wr can probably just remove it entirely if it's not available everywhere.

or we can just use a CFN intrinsic func to switch this off in certain partitions, if that's what we're getting at. Would be easier than pushing that logic up to the user of the deployer IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in that case i think i'd rather pull it out because it doesn't seem required 🤔

not sure if the parameters will affect runtimes based on capacity of Spot vs OnDemand, but the instance type prioritization seems like the only real different behavior

Copy link
Member

Choose a reason for hiding this comment

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

ack, can you test and make sure the nodes still come up without this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran a test successfully and updated the overview with the steps

@ndbaker1 ndbaker1 changed the title Add features flag to eksapi deployer Remove InstancesDistribution from unmanaged nodegroup cfn Jun 12, 2024
@cartermckinnon cartermckinnon merged commit ad35704 into aws:main Jun 27, 2024
3 checks passed
@ndbaker1 ndbaker1 deleted the features branch June 27, 2024 19:41
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.

3 participants