Change network interface setup logic to account for gb200 #6930

hgreebe · 2025-08-06T18:56:41Z

Description of changes

gb200 requires the following setup:
- if EfaEnabled: False, all even indices are set up with the regular interface, the odd indices are not used
- if EfaEnabled: True, all even indices need an efa interface type and all odd indicies need an efa-only interface type
This change sets up the network interface configuration for gb200 without altering the exisitng configuration for other instance types
Adds unit test that tests the network configuration for the launch template

Tests

Ran test_multiple_nics and test_efa integration tests

References

Cookbook change: Remove device name and ip address from required parameters of configure_nw_interfaces.sh aws-parallelcluster-cookbook#2999
Public documentation: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/efa-acc-inst-types.html

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

cli/tests/pcluster/templates/test_queues_stack.py

gmarciani · 2025-08-07T13:12:08Z

Please add in the Pr description a reference to official documentation driving this choice.

gmarciani · 2025-08-07T13:10:29Z

cli/src/pcluster/templates/queues_stack.py

+    """Generate launch template network interfaces list"""
+
+    is_gb200 = compute_resource.instance_types[0].split(".")[0] == P6E_GB200
+    interface = "efa" if compute_resource.efa and compute_resource.efa.enabled and not is_gb200 else None


[minor] Here and below: this is not an interface, but an interface type. What about renaming it to interface_type?

gmarciani · 2025-08-07T13:13:45Z

cli/src/pcluster/templates/queues_stack.py

+    for network_card in compute_resource.network_cards_list[1:]:
+        efa_enabled = True if compute_resource.efa and compute_resource.efa.enabled else False
+        even = network_card.network_card_index() % 2 == 0
+        # if efa is disabled, and we have a gb200 instance we skip configuring odd numbered indexes


Please, include in the comment the reason why we skip odd indexes.

gmarciani · 2025-08-07T13:15:19Z

cli/src/pcluster/templates/queues_stack.py

+        if is_gb200 and not efa_enabled and not even:
+            continue
+
+        interface = "efa" if compute_resource.efa and compute_resource.efa.enabled else None


[minor] can be = "efa" if efa_enabled else None"

gmarciani · 2025-08-07T13:18:40Z

cli/tests/pcluster/templates/test_queues_stack.py

+    [
+        (
+            True,
+            "p6e-gb200.36xlarge",


Our logic does not check for a specific size, but for whatever GB200 size. In this case it is better in unit test to use as instance_type value p6e.gb200.WHATEVER.

This approach has the following advantages:

(functional) it tests the actual logic, where the size is ignored

(documentation) it uses unit test to document our behaviour, making more explicit that we ignore the size

gmarciani · 2025-08-07T13:20:15Z

cli/tests/pcluster/templates/test_queues_stack.py

+        ),
+        (
+            True,
+            "c6in.32xlarge",


Same as above. In this case it would be better to use as instance_Type NOTp6e-gb200.WHATEVER_SIZE

It makes more sense to keep the instance type so that we have more confidence on what we are testing rather than keeping a generalized name like NOTp6e-gb200.WHATEVER_SIZE

I don't think so.

If you keep a specific value, your test verifies that the logic works on that specific value. If you use general values instead you cover the actual logic. This increases the reliability of the test and its value as a way to document behaviors.

Example to demonstrate the advantages:

(functional) if in your logic you're taking the expected decision only for c6in.32xlarge, the test with the specific value will succeed, even if the logic is incorrect. If you use a generic value that is clearly not used in the logic to take decisions, your test will be able to capture bugs.

(documentation) tests are meant to validate the correctness of behaviors, but also to document them. Ideally, we should be able to infer our behavior from the test, rather than looking at the underlying source code. If the test uses a specific value c6in.32xlarge, I may think that you're taking a decision around that specific value. If you use NOTp6e-gb200.WHATEVER_SIZE it is clear that the described behavior is for whatever instance type that is not p6e-gb200.

we are using NOTp6e-gb200.WHATEVER_SIZE or p6e-gb200.WHATEVER_SIZE which basically indicates that we are using p6e-gb200 for our logic. I dont really care about the naming convention here we could essentially specify the instance type as a comment, but i want the test to cover the different setup of NICs from the major multi-nic instances like hpc6id or p4d.
If in future, there is an immediate need to check for this specific logic working for these specific instance type I dont want to go and check the InstanceTypeData of these instances and compare it with the test cases here to see that it works, which is why the instance type (or a comment) as the name gives more confidence rather than a generic name.

cli/tests/pcluster/templates/test_queues_stack.py

himani2411 · 2025-08-07T14:20:05Z

cli/src/pcluster/templates/queues_stack.py

+):
+    """Generate launch template network interfaces list."""
+    is_gb200 = compute_resource.instance_types[0].split(".")[0] == P6E_GB200
+    interface_type = "efa" if compute_resource.efa and compute_resource.efa.enabled and not is_gb200 else None


I think we should mention that we skip the ODD indices if EFA is disabled for Gb200 instance in a validator as well as the public documentation?

In documentation for sure, but I would not include it in a validator as it is a too low level detail for a validator.

cli/src/pcluster/templates/queues_stack.py

himani2411 · 2025-08-07T14:31:33Z

cli/src/pcluster/templates/queues_stack.py

+    ]
+
+    for network_card in compute_resource.network_cards_list[1:]:
+        efa_enabled = compute_resource.efa and compute_resource.efa.enabled


If you are already using this as a constant why not use if above at line 372?

himani2411 · 2025-08-07T14:41:18Z

Can you also describe the test done for testing Gb200 launch template generation if you have done it?

himani2411 · 2025-08-07T14:44:00Z

cli/src/pcluster/templates/queues_stack.py

+
+        compute_lt_nw_interfaces.append(
+            ec2.CfnLaunchTemplate.NetworkInterfaceProperty(
+                device_index=0 if network_card.maximum_network_interfaces() == 1 else 1,


Is this true for p4d's too?or gb200?

Maximum Network interfaces of Gb200 is 17 so the device Index would be 1.

MaximumNetworkInterfaces for the network card is 1 for odd indexes and 2 for even indexes. The maximum network cards is 17.

This logic also works for all other instance types.

Change network interface setup logic to account for gb200

a59f9c9

hgreebe mentioned this pull request Aug 6, 2025

Remove device name and ip address from required parameters of configure_nw_interfaces.sh aws/aws-parallelcluster-cookbook#2999

Merged

github-advanced-security bot found potential problems Aug 6, 2025

View reviewed changes

cli/tests/pcluster/templates/test_queues_stack.py Fixed Show fixed Hide fixed

cli/tests/pcluster/templates/test_queues_stack.py Fixed Show fixed Hide fixed

cli/tests/pcluster/templates/test_queues_stack.py Fixed Show fixed Hide fixed

hgreebe force-pushed the develop branch from 032b285 to 58b513d Compare August 6, 2025 18:58

hgreebe marked this pull request as ready for review August 6, 2025 18:59

hgreebe requested review from a team as code owners August 6, 2025 18:59

hgreebe force-pushed the develop branch from 58b513d to 2a99978 Compare August 6, 2025 18:59

gmarciani reviewed Aug 7, 2025

View reviewed changes

Add unit test for network interface setup

a3df6c4

hgreebe force-pushed the develop branch from 2a99978 to a3df6c4 Compare August 7, 2025 13:22

himani2411 reviewed Aug 7, 2025

View reviewed changes

cli/src/pcluster/templates/queues_stack.py Show resolved Hide resolved

himani2411 reviewed Aug 7, 2025

View reviewed changes

hgreebe force-pushed the develop branch from fd046a1 to 79dc376 Compare August 7, 2025 15:13

Make unit test more explicit that we ignore the size of the instance

11c2917

hgreebe force-pushed the develop branch from 66f5fc9 to 11c2917 Compare August 7, 2025 15:36

Merge branch 'develop' into develop

30dcfef

himani2411 mentioned this pull request Aug 8, 2025

[SlurmTopo] Add support for slurm Block Topology aws/aws-parallelcluster-cookbook#3002

Merged

gmarciani previously approved these changes Aug 8, 2025

View reviewed changes

hgreebe added the skip-changelog-update Disables the check that enforces changelog updates in PRs label Aug 8, 2025

hgreebe dismissed gmarciani’s stale review via ce2fe54 August 8, 2025 15:54

hgreebe enabled auto-merge (rebase) August 8, 2025 15:55

Add extra test case to network configuration unit test

1110ee8

hgreebe force-pushed the develop branch from ce2fe54 to 1110ee8 Compare August 8, 2025 15:56

hgreebe disabled auto-merge August 8, 2025 15:56

hgreebe enabled auto-merge (rebase) August 8, 2025 15:56

himani2411 approved these changes Aug 8, 2025

View reviewed changes

hgreebe merged commit d764202 into aws:develop Aug 8, 2025
24 checks passed

Change network interface setup logic to account for gb200 #6930

Change network interface setup logic to account for gb200 #6930

Uh oh!

Conversation

hgreebe commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Tests

References

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmarciani commented Aug 7, 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!

himani2411 Aug 8, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himani2411 commented Aug 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hgreebe commented Aug 6, 2025 •

edited

Loading

himani2411 Aug 8, 2025 •

edited

Loading