feat: add single zone configuration #984

zongzhidanielhu · 2025-03-17T19:26:36Z

Issue number:

Description of changes:
Added feature allows configuring a specific zone in Test.toml, so that EC2 instance(s) can be launched in this specific zone. Usually, two zones must specified, but this feature allows one specific single zone for EC2 instance to launch into.

Testing done:
5 rounds same testings have been launched to make sure us-west-2b is not randomly picked up:
Input Test.toml:

After EC2 instance launched:
[ (eksctl-x86-64-aws-k8s-129-nvidia-cluster/SubnetPublicUSWEST2B)]

Invalid input tests:
input:

output:

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

bottlerocket/agents/src/bin/ec2-resource-agent/ec2_provider.rs

gthao313 · 2025-03-17T20:52:35Z

bottlerocket/agents/src/bin/ec2-resource-agent/ec2_provider.rs

+        let mut flex_subnet_ids = spec.clone().configuration.subnet_ids;
+
+        if flex_subnet_ids.len() <= 2 {
+            flex_subnet_ids.remove(0);


I would explain why we should remove the first position value here and why this would be triggered under two subnet ids.

sure, Will do

What if someone provides ["us-west-2b", "us-west-2c"], wouldn't this now fail? I do not believe we want fixed offsets like this. We should look for the value we want and use that. I don't think it is safe to assume the input is sorted or in the order we expect.

gthao313 · 2025-03-17T20:56:54Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+    UsWest2b,
+    #[strum(serialize = "us-west-2c")]
+    UsWest2c,
+}


If we miss UsWest2d here?

It appears the test is flagging UsWest2d as an invalid input, which doesn't seem correct. Why should we consider this availability zone invalid when it's a legitimate zone in AWS US-West-2 region?

There are 3 valid zones a, b and c. but in Test.toml you basically can enter whatever you want to. This check just make sure input is valid, if not log out info in testsys. eksctl will through a error too if the input zone is invalid, but a concise code will do a check too, I would rather check it before the eksctl through.

Your account might only have 3 but there can be four https://docs.aws.amazon.com/global-infrastructure/latest/regions/aws-availability-zones.html

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

gthao313 · 2025-03-17T21:02:57Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+
+    if zones.is_some() && zones.iter().len() < 2 {
+        let zones_upwrap = zones.clone().unwrap().first().unwrap().to_string();
+        if AvailabilityZones::from_str(&zones_upwrap).is_err() {


I feel like we should run the zone input value validation at twoliter. Twoliter does the job to read test.toml file and pass it to testsys. so I think twoliter should own the job to validate it as well.

either way will works, but checking here is much easily due to the core logic is in the cluster creation, and the key purpose of give a input zone is for create a cluster with the instance we want to.

gthao313 · 2025-03-17T21:03:56Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+            .unwrap()
+            .as_ref()
+            .to_string();
+        eks_zones.insert(0, remain_zone);


I think I don't fully understand this method. The second position value will always be 2b ?

I think I understand the current implementation (although I haven't seen the twoliter logic directly). The system randomly adds an extra availability zone to the EKS cluster zone list if only one zone is provided by Test.toml and inserts its subnet ID at the start of the subnet list. When launching EC2 instances, it removes this first subnet ID and uses the remaining ones. I think this isn't the ideal solution that we want.

Instead, we should use the AWS describe-subnets API to identify the exact subnet IDs matching our target availability zones from Test.toml, then pass these specific subnet IDs to ec2 instance launch client. This would allow the EC2 agent to launch instances only in our intended subnets, providing a cleaner and more controlled solution.

I disagree of using AWS describe-subnets, this is just add another layer of unnecessary calling of other API, the hard part of this problem is when have 2 subnets, you don't know which on is which, here I just used the most simple way, the Vec index to identify the target subnet without calling anything API overhead, and remove the others. You don't have to describe the subnets cause you already known the index[1] is the target zone subnet id.

By adding up the AWS describe-subnets, I need to setup the awscli, and do a API call, get the response, then from the response, find the field need, compare what I did which is much simple and clean, this just too much overhead.

By adding up the AWS describe-subnets, I need to setup the awscli, and do a API call, get the response, then from the response, find the field need, compare what I did which is much simple and clean, this just too much overhead.

You should use the AWS Rust SDK to make these calls and I agree with @gthao313, we should do this. It is not too much overhead, you have to make this code functional in more than just this one situation, and these describes and dealing with dynamic changes is part of dealing with making this code re-useable/flexible.

You don't have to describe the subnets cause you already known the index[1] is the target zone subnet id.

You have hard coded the AZs above which is why I commented there. This code will only ever work while there are 3 AZs and only in us-west-2. Using describe calls with the AWS SDK would allow this code to run other places than us-west-2 and still retain the same functionality.

yeazelm · 2025-03-17T22:34:21Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+#[strum(serialize_all = "kebab-case")]
+enum AvailabilityZones {
+    #[strum(serialize = "us-west-2a")]
+    UsWest2a,


I'm not sure if we want to hard code this. Is there a way we can get the Availability Zones from the ec2 client at runtime?

This is the hard part of this issue, everything has to be done before the ec2 instances created, that's why I can not use ec2 client due to by then the instance(s) already created which is no way we can manipulate it. It is kind of not hardcode due to it is only has three zones, a, b, c, and it is just for define the valid zone boundary. This will give us more flexible of when input is c, you know you will add a or b to create cluster, when input is b, you know you will check a, c, not e, f, g cause the valid zone boundary already defined.

I think we need to go back to the approach and see if we need to create an EC2 client before the rest of this because this is a blocking issue, we can't merge hard-coding us-west-2 AZs as the only place this works.

I agree that we shouldn't hardcode since it would be broken if we pass the zone at other region. It already provides the region that cluster is running at and setup aws config based on the region, so we can programmatically list the available zones by using the ec2_client.

The issue here is to provide extra available zones if only one zone is provided via Test.toml, so we list all zones and randomly give one (not the input one) to eksctl.

This would help https://docs.rs/aws-sdk-ec2/latest/aws_sdk_ec2/struct.Client.html#method.describe_availability_zones

yeazelm · 2025-03-18T14:31:30Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+            .unwrap()
+            .as_ref()
+            .to_string();
+        eks_zones.insert(0, remain_zone);


By adding up the AWS describe-subnets, I need to setup the awscli, and do a API call, get the response, then from the response, find the field need, compare what I did which is much simple and clean, this just too much overhead.

You should use the AWS Rust SDK to make these calls and I agree with @gthao313, we should do this. It is not too much overhead, you have to make this code functional in more than just this one situation, and these describes and dealing with dynamic changes is part of dealing with making this code re-useable/flexible.

You don't have to describe the subnets cause you already known the index[1] is the target zone subnet id.

You have hard coded the AZs above which is why I commented there. This code will only ever work while there are 3 AZs and only in us-west-2. Using describe calls with the AWS SDK would allow this code to run other places than us-west-2 and still retain the same functionality.

yeazelm · 2025-03-18T14:33:34Z

bottlerocket/agents/src/bin/ec2-resource-agent/ec2_provider.rs

+        let mut flex_subnet_ids = spec.clone().configuration.subnet_ids;
+
+        if flex_subnet_ids.len() <= 2 {
+            flex_subnet_ids.remove(0);


What if someone provides ["us-west-2b", "us-west-2c"], wouldn't this now fail? I do not believe we want fixed offsets like this. We should look for the value we want and use that. I don't think it is safe to assume the input is sorted or in the order we expect.

yeazelm · 2025-03-18T14:35:04Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+#[strum(serialize_all = "kebab-case")]
+enum AvailabilityZones {
+    #[strum(serialize = "us-west-2a")]
+    UsWest2a,


I think we need to go back to the approach and see if we need to create an EC2 client before the rest of this because this is a blocking issue, we can't merge hard-coding us-west-2 AZs as the only place this works.

yeazelm · 2025-03-18T14:36:34Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+    UsWest2b,
+    #[strum(serialize = "us-west-2c")]
+    UsWest2c,
+}


Your account might only have 3 but there can be four https://docs.aws.amazon.com/global-infrastructure/latest/regions/aws-availability-zones.html

yeazelm · 2025-03-18T14:45:39Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

    desired_capacity: i32,
 }

+fn zone_to_exclude(exclude: String) -> Vec<AvailabilityZones> {


This feels like it should be swapped out for a function using the EC2 client to get the available availability zones for that account in that region.

nit: the name is not really helpful. You have named it what you are using it for, not what it does. This function returns the availability zones in us-west-2. If we must keep this function (which my above comment asks not to), then it should be named something more specific to its function, not what you want to use it for like all_us_west_2_zones() or something like that.

yeazelm · 2025-03-18T14:46:53Z

bottlerocket/agents/src/bin/eks-resource-agent/eks_provider.rs

+        }
+        let remain_zone = zone_to_exclude(zone_input.to_string())
+            .first()
+            .unwrap()


We don't use unwrap() for these situations. I like to think of unwrap() as a good indicator I might have a flaw in my approach. It makes me ask "why does Rust think this can fail?" and it helps shift my focus to how to avoid it being possible to fail. In this case, you likely need to handle the error since you are hard coding the output from this function, you should be able to enforce that it won't fail. But if this was output from a client to EC2, then you need to properly handle if the output of this function doesn't match your expectations of there being a return from first().

zongzhidanielhu requested review from gthao313 and yeazelm March 17, 2025 19:26

zongzhidanielhu force-pushed the 1.29_az branch 4 times, most recently from c7f92d7 to 08f5cb9 Compare March 17, 2025 20:18

gthao313 reviewed Mar 17, 2025

View reviewed changes

yeazelm reviewed Mar 17, 2025

View reviewed changes

zongzhidanielhu mentioned this pull request Mar 18, 2025

feat: add single zone configuration bottlerocket-os/twoliter#509

Closed

zongzhidanielhu force-pushed the 1.29_az branch 4 times, most recently from 859ba00 to 5853f87 Compare March 18, 2025 07:13

yeazelm requested changes Mar 18, 2025

View reviewed changes

yeazelm reviewed Mar 18, 2025

View reviewed changes

add config field

b7b9856

zongzhidanielhu force-pushed the 1.29_az branch from 5853f87 to b7b9856 Compare March 31, 2025 20:48

feat: add single zone configuration #984

Are you sure you want to change the base?

feat: add single zone configuration #984

Uh oh!

Conversation

zongzhidanielhu commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zongzhidanielhu Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zongzhidanielhu Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zongzhidanielhu commented Mar 17, 2025 •

edited

Loading

zongzhidanielhu Mar 18, 2025 •

edited

Loading

zongzhidanielhu Mar 18, 2025 •

edited

Loading