Merge branch 'develop' into PCLUSTER-5187

Author: davprat

.gitignore

@@ -16,3 +16,4 @@ assets/
 report.html
 tests_outputs/
 .python-version
+test.yaml

.pre-commit-config.yaml

@@ -1,5 +1,5 @@
 repos:
-  - repo: git://github.com/pre-commit/pre-commit-hooks
+  - repo: https://github.com/pre-commit/pre-commit-hooks
     rev: v3.4.0
     hooks:
       - id: trailing-whitespace
@@ -37,7 +37,7 @@ repos:
         args: ['-rc', '-w 120', '--settings-path=cli/.isort.cfg']
 
   - repo: https://github.com/ambv/black
-    rev: 20.8b1
+    rev: 22.6.0
     hooks:
       - id: black
         args: ['-l 120']

CHANGELOG.md

@@ -5,7 +5,7 @@ CHANGELOG
 -----
 
 **ENHANCEMENTS**
-- Add new configuration parameter `DeletionPolicy` for EFs and FSx for Lustre shared storage 
+- Add new configuration parameter `DeletionPolicy` for EFs and FSx for Lustre shared storage
   to support storage retention on deletion.
 
 **CHANGES**
@@ -14,9 +14,10 @@ CHANGELOG
 - Move head node tags from Launch Template to instance definition to avoid head node replacement on tags updates.
 - Disable Multithreading through script executed by cloud-init and not through CpuOptions set into Launch Template.
 - Add support for multiple instance types in the same Compute Resource.
+- Add support for a Name field in PlacementGroup as the preferred naming method.
 
 **BUG FIXES**
-- Fix validation of parameter `SharedStorage/EfsSettings`: now validation fails when `FileSystemId` is specified 
+- Fix validation of parameter `SharedStorage/EfsSettings`: now validation fails when `FileSystemId` is specified
   along with other `SharedStorage/EfsSettings` parameters, whereas it was previously ignoring them.
 - Fix cluster update when changing the order of SharedStorage together with other changes in the configuration.
 - Avoid failing on DescribeCluster when cluster configuration is not available.
@@ -74,7 +75,7 @@ CHANGELOG
 - Fix file handle leak in `computemgtd`.
 - Fix race condition that was sporadically causing launched instances to be immediately terminated because not available yet in EC2 DescribeInstances response
 - Fix support for `DisableSimultaneousMultithreading` parameter on instance types with Arm processors.
-- Fix ParallelCluster API stack update failure when upgrading from a previus version. Add resource pattern used for the `ListImagePipelineImages` action in the `EcrImageDeletionLambdaRole`. 
+- Fix ParallelCluster API stack update failure when upgrading from a previus version. Add resource pattern used for the `ListImagePipelineImages` action in the `EcrImageDeletionLambdaRole`.
 - Fix ParallelCluster API adding missing permissions needed to import/export from S3 when creating an FSx for Lustre storage.
 
 3.1.4

cli/src/pcluster/config/cluster_config.py

@@ -119,7 +119,7 @@
     InstanceTypeMemoryInfoValidator,
     InstanceTypeValidator,
     KeyPairValidator,
-    PlacementGroupIdValidator,
+    PlacementGroupNamingValidator,
 )
 from pcluster.validators.fsx_validators import (
     FsxAutoImportValidator,
@@ -539,13 +539,14 @@ def availability_zone(self):
 class PlacementGroup(Resource):
     """Represent the placement group for the Queue networking."""
 
-    def __init__(self, enabled: bool = None, id: str = None):
+    def __init__(self, enabled: bool = None, name: str = None, id: str = None):
         super().__init__()
         self.enabled = Resource.init_param(enabled, default=False)
-        self.id = Resource.init_param(id)
+        self.name = Resource.init_param(name)
+        self.id = Resource.init_param(id)  # Duplicate of name
 
     def _register_validators(self):
-        self._register_validator(PlacementGroupIdValidator, placement_group=self)
+        self._register_validator(PlacementGroupNamingValidator, placement_group=self)
 
 
 class _QueueNetworking(_BaseNetworking):

cli/src/pcluster/schemas/cluster_schema.py

@@ -630,6 +630,7 @@ class PlacementGroupSchema(BaseSchema):
 
     enabled = fields.Bool(metadata={"update_policy": UpdatePolicy.QUEUE_UPDATE_STRATEGY})
     id = fields.Str(metadata={"update_policy": UpdatePolicy.QUEUE_UPDATE_STRATEGY})
+    name = fields.Str(metadata={"update_policy": UpdatePolicy.QUEUE_UPDATE_STRATEGY})
 
     @post_load
     def make_resource(self, data, **kwargs):

cli/src/pcluster/templates/cluster_stack.py

@@ -1367,7 +1367,7 @@ def _add_placement_groups(self) -> Dict[str, ec2.CfnPlacementGroup]:
             if (
                 queue.networking.placement_group
                 and queue.networking.placement_group.enabled
-                and not queue.networking.placement_group.id
+                and not (queue.networking.placement_group.id or queue.networking.placement_group.name)
             ):
                 managed_placement_groups[queue.name] = ec2.CfnPlacementGroup(
                     self, f"PlacementGroup{create_hash_suffix(queue.name)}", strategy="cluster"
@@ -1382,10 +1382,10 @@ def _add_launch_templates(self, managed_placement_groups, instance_profiles):
 
             queue_placement_group = None
             if queue.networking.placement_group:
-                if queue.networking.placement_group.id and (
+                if (queue.networking.placement_group.id or queue.networking.placement_group.name) and (
                     queue.networking.placement_group.enabled or queue.networking.placement_group.is_implied("enabled")
-                ):  # Do not use `PlacementGroup/Id` when `PlacementGroup/Enabled` is explicitly set to `false`
-                    queue_placement_group = queue.networking.placement_group.id
+                ):
+                    queue_placement_group = queue.networking.placement_group.name or queue.networking.placement_group.id
                 elif queue.networking.placement_group.enabled:
                     queue_placement_group = managed_placement_groups[queue.name].ref
 

cli/src/pcluster/validators/ec2_validators.py

@@ -99,21 +99,27 @@ def _validate(self, key_name: str):
             )
 
 
-class PlacementGroupIdValidator(Validator):  # TODO: add tests
-    """Placement group id validator."""
+class PlacementGroupNamingValidator(Validator):  # TODO: add tests
+    """Placement group naming validator."""
 
     def _validate(self, placement_group):
-        if placement_group.id:
+        if placement_group.id and placement_group.name:
+            self._add_failure(
+                "PlacementGroup Id cannot be set when setting PlacementGroup Name.  Please "
+                "set either Id or Name but not both.",
+                FailureLevel.ERROR,
+            )
+        identifier = placement_group.name or placement_group.id
+        if identifier:
             if not placement_group.is_implied("enabled") and not placement_group.enabled:
                 self._add_failure(
-                    "PlacementGroup Id can not be set when setting 'Enabled: false' in queue's "
-                    "networking section. Please remove PlacementGroup Id if you don't want"
-                    " to use PlacementGroup.",
+                    "The PlacementGroup feature must be enabled (Enabled: true) in order "
+                    "to assign a Name or Id parameter",
                     FailureLevel.ERROR,
                 )
             else:
                 try:
-                    AWSApi.instance().ec2.describe_placement_group(placement_group.id)
+                    AWSApi.instance().ec2.describe_placement_group(identifier)
                 except AWSClientError as e:
                     self._add_failure(str(e), FailureLevel.ERROR)
 

cli/tests/pcluster/validators/test_ec2_validators.py

@@ -21,7 +21,7 @@
     InstanceTypeMemoryInfoValidator,
     InstanceTypeValidator,
     KeyPairValidator,
-    PlacementGroupIdValidator,
+    PlacementGroupNamingValidator,
 )
 from tests.pcluster.aws.dummy_aws_api import mock_aws_api
 from tests.pcluster.validators.utils import assert_failure_messages
@@ -408,6 +408,16 @@ def test_compute_ami_os_compatible_validator(mocker, image_id, os, ami_info, exp
             None,
             None,
         ),
+        (
+            PlacementGroup(enabled=True, name="test"),
+            {
+                "PlacementGroups": [
+                    {"GroupName": "test", "State": "available", "Strategy": "cluster", "GroupId": "pg-0123"}
+                ]
+            },
+            None,
+            None,
+        ),
         (
             PlacementGroup(enabled=True),
             {
@@ -428,12 +438,28 @@ def test_compute_ami_os_compatible_validator(mocker, image_id, os, ami_info, exp
             None,
             None,
         ),
+        (
+            PlacementGroup(name="test"),
+            {
+                "PlacementGroups": [
+                    {"GroupName": "test", "State": "available", "Strategy": "cluster", "GroupId": "pg-0123"}
+                ]
+            },
+            None,
+            None,
+        ),
         (
             PlacementGroup(id="test"),
             None,
             AWSClientError(function_name="describe_placement_group", message="The Placement Group 'test' is unknown"),
             "The Placement Group 'test' is unknown",
         ),
+        (
+            PlacementGroup(name="test"),
+            None,
+            AWSClientError(function_name="describe_placement_group", message="The Placement Group 'test' is unknown"),
+            "The Placement Group 'test' is unknown",
+        ),
         (
             PlacementGroup(enabled=False, id="test"),
             {
@@ -442,7 +468,28 @@ def test_compute_ami_os_compatible_validator(mocker, image_id, os, ami_info, exp
                 ]
             },
             None,
-            "PlacementGroup Id can not be set when setting",
+            "The PlacementGroup feature must be enabled (Enabled: true) in order to assign a Name or Id parameter",
+        ),
+        (
+            PlacementGroup(enabled=False, name="test"),
+            {
+                "PlacementGroups": [
+                    {"GroupName": "test", "State": "available", "Strategy": "cluster", "GroupId": "pg-0123"}
+                ]
+            },
+            None,
+            "The PlacementGroup feature must be enabled (Enabled: true) in order to assign a Name or Id parameter",
+        ),
+        (
+            PlacementGroup(enabled=True, id="test", name="test2"),
+            {
+                "PlacementGroups": [
+                    {"GroupName": "test", "State": "available", "Strategy": "cluster", "GroupId": "pg-0123"}
+                ]
+            },
+            None,
+            "PlacementGroup Id cannot be set when setting PlacementGroup Name.  Please "
+            "set either Id or Name but not both.",
         ),
     ],
 )
@@ -455,5 +502,5 @@ def test_placement_group_validator(
         return_value=describe_placement_group_return,
         side_effect=side_effect,
     )
-    actual_failures = PlacementGroupIdValidator().execute(placement_group=placement_group)
+    actual_failures = PlacementGroupNamingValidator().execute(placement_group=placement_group)
     assert_failure_messages(actual_failures, expected_message)

cli/tests/requirements.txt

@@ -1,11 +1,12 @@
 assertpy
+aws-lambda-powertools
 freezegun
 jinja2
+munch
 pytest
 pytest-cov
 pytest-datadir
 pytest-html
 pytest-mock
 pytest-xdist
 recordclass
-munch

tests/integration-tests/benchmarks/test_scaling_performance/test_scaling_performance/pcluster.config.yaml

@@ -14,7 +14,8 @@ Scheduling:
     - Name: queue-0
       ComputeResources:
         - Name: compute-resource-0
-          InstanceType: {{ instance }}
+          InstanceTypeList:
+            - InstanceType: {{ instance }}
           MaxCount: {{ scaling_target }}
       Networking:
         SubnetIds: