diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dc33a8646..9671ffe29a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,6 @@ CHANGELOG - Add validation to block updates that change tag order. Blocking such change prevents update failures. - Fix LoginNodes NLB not being publicly accessible when using public subnet with implicit main route table association. See https://github.com/aws/aws-parallelcluster/issues/7173 -- Fix cluster update failure when changing LoginNodes subnet by including subnet hash in target group logical ID and name. - Fix cluster creation failure without Internet access when GPU instances and DCV are used. 3.14.1 diff --git a/cli/src/pcluster/templates/login_nodes_stack.py b/cli/src/pcluster/templates/login_nodes_stack.py index 91a10ac37d..97e3de8d39 100644 --- a/cli/src/pcluster/templates/login_nodes_stack.py +++ b/cli/src/pcluster/templates/login_nodes_stack.py @@ -404,10 +404,9 @@ def _get_launching_lifecycle_hook_specification(self): ) def _add_login_nodes_pool_target_group(self): - subnet_hash = create_hash_suffix(self._pool.networking.subnet_ids[0])[:8] return elbv2.NetworkTargetGroup( self, - f"{self._pool.name}TargetGroup{subnet_hash}", + f"{self._pool.name}TargetGroup", health_check=elbv2.HealthCheck( port="22", protocol=elbv2.Protocol.TCP, @@ -416,14 +415,11 @@ def _add_login_nodes_pool_target_group(self): protocol=elbv2.Protocol.TCP, target_type=elbv2.TargetType.INSTANCE, vpc=self._vpc, - # AWS Target Group name limit is 32 characters. - # With 3 args: (7 chars * 3) + (2 hyphens) + (1 hyphen) + (8 char hash) = 32 chars target_group_name=_get_resource_combination_name( self._config.cluster_name, self._pool.name, - subnet_hash, partial_length=7, - hash_length=8, + hash_length=16, ), ) diff --git a/cli/tests/pcluster/templates/test_cluster_stack.py b/cli/tests/pcluster/templates/test_cluster_stack.py index ac201478b7..be707d6b6d 100644 --- a/cli/tests/pcluster/templates/test_cluster_stack.py +++ b/cli/tests/pcluster/templates/test_cluster_stack.py @@ -876,7 +876,7 @@ def test_login_nodes_traffic_management_resources_values_properties( auto_scaling_group_id = "clusternametestloginnodespool1clusternametestloginnodespool1AutoScalingGroup5EBA3937" load_balancer_id = "clusternametestloginnodespool1testloginnodespool1LoadBalancerE1D4FCC7" - target_group_id = "clusternametestloginnodespool1testloginnodespool1TargetGroup4592cff378B1099C" + target_group_id = "clusternametestloginnodespool1testloginnodespool1TargetGroup713F5EC5" listener_id = ( "clusternametestloginnodespool1testloginnodespool1LoadBalancerLoginNodesListenertestloginnodespool165B4D3DC" ) @@ -1357,18 +1357,19 @@ def test_custom_munge_key_iam_policy(mocker, test_datadir, config_file_name): @pytest.mark.parametrize( - "resource_names, partial_length, hash_length, expected_combination_name", + "resource_name_1, resource_name_2, partial_length, hash_length, expected_combination_name", [ - (["test-cluster", "test-pool"], 7, 16, "test-cl-test-po-18c74b16dfbc78ac"), - (["abcdefghijk", "lmnopqrst"], 8, 14, "abcdefgh-lmnopqrs-dd65eea0329dcb"), - (["a", "b"], 7, 16, "a-b-fb8e20fc2e4c3f24"), - # Test with 3 resource names (like target group: cluster_name, pool_name, subnet_hash) - (["clustername", "loginpool", "092512032a1df0b2c"], 7, 8, "cluster-loginpo-0925120-fde21041"), + ("test-cluster", "test-pool", 7, 16, "test-cl-test-po-18c74b16dfbc78ac"), + ("abcdefghijk", "lmnopqrst", 8, 14, "abcdefgh-lmnopqrs-dd65eea0329dcb"), + ("a", "b", 7, 16, "a-b-fb8e20fc2e4c3f24"), ], ) -def test_resource_combination_name(resource_names, partial_length, hash_length, expected_combination_name): +def test_resource_combination_name( + resource_name_1, resource_name_2, partial_length, hash_length, expected_combination_name +): combination_name = _get_resource_combination_name( - *resource_names, + resource_name_1, + resource_name_2, partial_length=partial_length, hash_length=hash_length, ) diff --git a/cli/tests/pcluster/templates/test_login_nodes_stack.py b/cli/tests/pcluster/templates/test_login_nodes_stack.py index 7c636431e0..e1dacee96c 100644 --- a/cli/tests/pcluster/templates/test_login_nodes_stack.py +++ b/cli/tests/pcluster/templates/test_login_nodes_stack.py @@ -78,86 +78,3 @@ def render_join(elem: dict): else: raise ValueError("Found unsupported item type while rendering Fn::Join") return sep.join(rendered_body) - - -@pytest.mark.parametrize( - "config_file_name, keep_original_subnet", - [ - ("config-1.yaml", True), - ("config-2.yaml", False), - ], -) -def test_target_group_with_config_files(mocker, test_datadir, config_file_name, keep_original_subnet): - """ - Test target group logical ID and name change when subnet is modified. - - This test verifies that: - 1. Target group logical ID and name are generated correctly - 2. When subnet changes, both logical ID and target group name change - 3. Target group name stays within AWS 32 char limit - - This ensures that cluster updates with subnet changes create new target groups, - avoiding "target group cannot be associated with more than one load balancer" - and "target group name already exists" errors. - """ - mock_aws_api(mocker) - mock_bucket_object_utils(mocker) - - def get_target_group_info(cdk_assets): - """Extract target group logical ID and name from CDK assets.""" - for asset in cdk_assets: - asset_content = asset.get("content") - if asset_content and "Resources" in asset_content: - for resource_name, resource in asset_content["Resources"].items(): - if resource.get("Type") == "AWS::ElasticLoadBalancingV2::TargetGroup": - return resource_name, resource["Properties"]["Name"] - return None, None - - def build_template(config_yaml): - """Build CDK template from config yaml.""" - cluster_config = ClusterSchema(cluster_name="clustername").load(config_yaml) - _, cdk_assets = CDKTemplateBuilder().build_cluster_template( - cluster_config=cluster_config, bucket=dummy_cluster_bucket(), stack_name="clustername" - ) - return cdk_assets - - def assert_target_group_info(logical_id, tg_name): - """Verify target group logical ID and name are not None and within AWS 32 char limit.""" - assert_that(logical_id).is_not_none() - assert_that(tg_name).is_not_none() - assert_that(len(tg_name)).is_less_than_or_equal_to(32) - - # Read yaml config - input_yaml = load_yaml_dict(test_datadir / config_file_name) - original_subnet = input_yaml["LoginNodes"]["Pools"][0]["Networking"]["SubnetIds"][0] - - # Build template with original subnet - cdk_assets_original = build_template(input_yaml) - logical_id_original, tg_name_original = get_target_group_info(cdk_assets_original) - - # Verify original target group was created correctly - assert_target_group_info(logical_id_original, tg_name_original) - - # Modify subnet to a new value - new_subnet = "subnet-12345678901234567" - new_pool_count = 0 - if keep_original_subnet: - new_subnet = original_subnet - new_pool_count = 1 - input_yaml["LoginNodes"]["Pools"][0]["Networking"]["SubnetIds"][0] = new_subnet - input_yaml["LoginNodes"]["Pools"][0]["Count"] = new_pool_count - - # Build template with new subnet - cdk_assets_new = build_template(input_yaml) - logical_id_new, tg_name_new = get_target_group_info(cdk_assets_new) - - if keep_original_subnet: - assert_that(logical_id_new).is_equal_to(logical_id_original) - assert_that(tg_name_new).is_equal_to(tg_name_original) - else: - # Verify that both logical ID and target group name changed when subnet changed - assert_that(logical_id_new).is_not_equal_to(logical_id_original) - assert_that(tg_name_new).is_not_equal_to(tg_name_original) - - # Verify new target group was created correctly - assert_target_group_info(logical_id_new, tg_name_new) diff --git a/cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-1.yaml b/cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-1.yaml deleted file mode 100644 index 23d07ddf58..0000000000 --- a/cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-1.yaml +++ /dev/null @@ -1,32 +0,0 @@ -Region: us-east-1 -Image: - Os: alinux2 -LoginNodes: - Pools: - - Name: login - InstanceType: t3.micro - Count: 1 - Networking: - SubnetIds: - - subnet-12345678 - Dcv: - Enabled: true - Port: 8444 -HeadNode: - InstanceType: t3.micro - Networking: - SubnetId: subnet-12345678 - Ssh: - KeyName: ec2-key-name -Scheduling: - Scheduler: slurm - SlurmQueues: - - Name: queue1 - ComputeResources: - - Name: cr1 - InstanceType: c4.xlarge - MinCount: 0 - MaxCount: 10 - Networking: - SubnetIds: - - subnet-12345678 diff --git a/cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-2.yaml b/cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-2.yaml deleted file mode 100644 index 11a5a38363..0000000000 --- a/cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-2.yaml +++ /dev/null @@ -1,31 +0,0 @@ -Region: us-east-1 -Image: - Os: alinux2 -LoginNodes: - Pools: - - Name: login - InstanceType: t3.micro - Count: 1 - Networking: - SubnetIds: - - subnet-12345678 - Dcv: - Enabled: false -HeadNode: - InstanceType: t3.micro - Networking: - SubnetId: subnet-12345678 - Ssh: - KeyName: ec2-key-name -Scheduling: - Scheduler: slurm - SlurmQueues: - - Name: queue1 - ComputeResources: - - Name: cr1 - InstanceType: c4.xlarge - MinCount: 0 - MaxCount: 10 - Networking: - SubnetIds: - - subnet-12345678