helper tool to add new security groups to FSxL ENIs #871

paragao · 2025-10-03T12:07:27Z

Issue #, if available: N/A

Description of changes:
Customers that want to share a single FSx for Lustre filesytem to different Sagemaker Hyperpod clusters need to update the FSx for Lustre ENIs with the new security group. FSx for Lustre scale the number of ENI depending on the size of storage it uses. For small filesystems (few TB), you may have just a few ENIs. For larger filesystems (hundreds of TB or PB), FSx for Lustre scale the number of ENIs (you can have dozens or hundreds).

This helper tool will automate adding the new security group to the existing file system ENIs.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

mayankgupta14 · 2025-10-14T08:40:14Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+
+# First get one network interface then describe the network interface to get existing Security Groups attached
+fsx_id_enis=$(aws fsx describe-file-systems "${awscli_args[@]}" --query 'FileSystems[0].NetworkInterfaceIds' --output text)
+existing_sg=$(aws ec2 describe-network-interfaces "${awscli_args[@]}" --network-interface-ids $temp_eni_id --query 'NetworkInterfaces[0].Groups[*].GroupId' --output text)


$temp_eni_id is undefined and is referenced here, it'll fail.

Need to define "temp_eni_id"

it needs to be fsx_id_enis (as we define in line 83 above).

mayankgupta14 · 2025-10-14T08:50:05Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+            ;;
+        *)
+            [[ "$fsx_id" == "" ]] \
+                && $fsx_id="$key" \


shouldn't this be fsx_id="$key"?

you have to check for both. The syntax do:

Test if fsx_id is empty

If it is true, then it assigns key to fsx_id.

If the user does not provide a fsx_id as part of the arguments, then the test will be FALSE. The fsx_id won't get assigned a value which will cause the script to exit.

mayankgupta14 · 2025-10-14T08:52:14Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+fi 
+
+echo -e "Amazon FSx for Lustre filesystem: ${GREEN}${fsx_id}${NC}"
+echo -e "Existing security groups attached on the filesystem: ${GREEN}${$existing_sg}${NC}"


incorrect variable syntax: ${$existing_sg} -> ${existing_sg}

mayankgupta14 · 2025-10-14T08:52:40Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+
+echo -e "Amazon FSx for Lustre filesystem: ${GREEN}${fsx_id}${NC}"
+echo -e "Existing security groups attached on the filesystem: ${GREEN}${$existing_sg}${NC}"
+echo -e "Adding security group ID: ${GREEN}${$security_group}${NC}"


Same here, incorrect variable syntax: ${$security_group} -> ${security_group}

mayankgupta14 · 2025-10-14T08:54:57Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+
+# Finally update the ENI to add the new security groups plus the existing security groups
+for i in $fsx_id_enis; do 
+    echo -e "Adding ${GREEN}${$security_group} to ENI $GREEN}${$i}"


same here, ${$security_group} -> ${security_group} and ${$i} -> ${i}
Additionally, missing { before GREEN, should be ${GREEN}

mayankgupta14 · 2025-10-14T09:02:20Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+# Finally update the ENI to add the new security groups plus the existing security groups
+for i in $fsx_id_enis; do 
+    echo -e "Adding ${GREEN}${$security_group} to ENI $GREEN}${$i}"
+    $(aws ec2 modify-network-interface-attribute "${awscli_args[@]}" --network-interface-id $i --groups $existing_sg $security_group)


Do we need the command substitution here?
We just executing the command, right? We don't need to capture the output?

shouldn't be below instead?:
aws ec2 modify-network-interface-attribute "${awscli_args[@]}" --network-interface-id $i --groups $existing_sg $security_group

Either work, but you are right. I'm not capturing the output; I care about the exit signal only.

mayankgupta14 · 2025-10-14T09:04:46Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+YELLOW='\033[1;33m'
+NC='\033[0m' # No Color
+
+escape_spaces() {


Where is this function being used?

can be removed.

mayankgupta14 · 2025-10-14T09:16:46Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+security groups to the FSx for Lustre ENIs"
+
+# First get one network interface then describe the network interface to get existing Security Groups attached
+fsx_id_enis=$(aws fsx describe-file-systems "${awscli_args[@]}" --query 'FileSystems[0].NetworkInterfaceIds' --output text)


This is getting ENIs for the first fsx found, and not really for defined $fsx_id. If there are multiple fsx, this will cause issues and might cause modifying wrong SG.

We should be using $fsx_id here to make sure we are getting ENIs for correct fsx.

nope, this is getting the ENIs for the $fsx_id defined on the arguments passed to the script. If the user does not pass an fsx_id on the arguments, the script doesn't run.

when the user pass the required arguments, then the parse_args function add the $fsx_id variable to the $awscli_args variable. Then on this CLI bash will expand ${awscli_args[@]} and use the required fsx_id as part of the command. It will then query the ENIs for the fsx_id passed as an argument to this function.

This is the right syntax.

paragao

addressed PR feedback and committed changes.

paragao · 2025-10-15T10:36:02Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+security groups to the FSx for Lustre ENIs"
+
+# First get one network interface then describe the network interface to get existing Security Groups attached
+fsx_id_enis=$(aws fsx describe-file-systems "${awscli_args[@]}" --query 'FileSystems[0].NetworkInterfaceIds' --output text)


nope, this is getting the ENIs for the $fsx_id defined on the arguments passed to the script. If the user does not pass an fsx_id on the arguments, the script doesn't run.

when the user pass the required arguments, then the parse_args function add the $fsx_id variable to the $awscli_args variable. Then on this CLI bash will expand ${awscli_args[@]} and use the required fsx_id as part of the command. It will then query the ENIs for the fsx_id passed as an argument to this function.

This is the right syntax.

paragao · 2025-10-15T10:36:58Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+YELLOW='\033[1;33m'
+NC='\033[0m' # No Color
+
+escape_spaces() {


can be removed.

paragao · 2025-10-15T10:40:07Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+            ;;
+        *)
+            [[ "$fsx_id" == "" ]] \
+                && $fsx_id="$key" \


you have to check for both. The syntax do:

Test if fsx_id is empty

If it is true, then it assigns key to fsx_id.

If the user does not provide a fsx_id as part of the arguments, then the test will be FALSE. The fsx_id won't get assigned a value which will cause the script to exit.

paragao · 2025-10-15T10:44:07Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+# Finally update the ENI to add the new security groups plus the existing security groups
+for i in $fsx_id_enis; do 
+    echo -e "Adding ${GREEN}${$security_group} to ENI $GREEN}${$i}"
+    $(aws ec2 modify-network-interface-attribute "${awscli_args[@]}" --network-interface-id $i --groups $existing_sg $security_group)


Either work, but you are right. I'm not capturing the output; I care about the exit signal only.

paragao · 2025-10-15T10:44:16Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+
+# Finally update the ENI to add the new security groups plus the existing security groups
+for i in $fsx_id_enis; do 
+    echo -e "Adding ${GREEN}${$security_group} to ENI $GREEN}${$i}"


paragao · 2025-10-15T10:44:22Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+
+echo -e "Amazon FSx for Lustre filesystem: ${GREEN}${fsx_id}${NC}"
+echo -e "Existing security groups attached on the filesystem: ${GREEN}${$existing_sg}${NC}"
+echo -e "Adding security group ID: ${GREEN}${$security_group}${NC}"


paragao · 2025-10-15T10:44:29Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+fi 
+
+echo -e "Amazon FSx for Lustre filesystem: ${GREEN}${fsx_id}${NC}"
+echo -e "Existing security groups attached on the filesystem: ${GREEN}${$existing_sg}${NC}"


paragao · 2025-10-15T10:45:50Z

1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh

+
+# First get one network interface then describe the network interface to get existing Security Groups attached
+fsx_id_enis=$(aws fsx describe-file-systems "${awscli_args[@]}" --query 'FileSystems[0].NetworkInterfaceIds' --output text)
+existing_sg=$(aws ec2 describe-network-interfaces "${awscli_args[@]}" --network-interface-ids $temp_eni_id --query 'NetworkInterfaces[0].Groups[*].GroupId' --output text)


it needs to be fsx_id_enis (as we define in line 83 above).

nghtm

Approving - this is a utility script, as long as it is tested and works, LGTM!

helper tool to add new security groups to FSxL ENIs

e6a07a1

mayankgupta14 reviewed Oct 14, 2025

View reviewed changes

address PR feedback

97cb9b7

paragao commented Oct 15, 2025

View reviewed changes

nghtm approved these changes Oct 30, 2025

View reviewed changes

helper tool to add new security groups to FSxL ENIs #871

Are you sure you want to change the base?

helper tool to add new security groups to FSxL ENIs #871

Uh oh!

Conversation

paragao commented Oct 3, 2025

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!

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!

paragao left a comment

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!

nghtm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants