Merge branch 'develop' into develop

Author: hanwen-pcluste

CHANGELOG.md

@@ -3,9 +3,11 @@ CHANGELOG
 
 3.x.x
 -----
+**ENHANCEMENTS**
+- Add support for updating `SharedStorage` configuration.
 
 **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.
 - Enable server-side encryption for the EcrImageBuilder SNS topic created when deploying ParallelCluster API and used to notify on docker image build events.
 - Add support for on-demand capacity reservations.

cli/src/pcluster/config/update_policy.py

@@ -11,7 +11,7 @@
 from enum import Enum
 
 from pcluster.config.cluster_config import QueueUpdateStrategy
-from pcluster.constants import DEFAULT_MAX_COUNT
+from pcluster.constants import AWSBATCH, DEFAULT_MAX_COUNT, SLURM
 
 
 class UpdatePolicy:
@@ -125,6 +125,39 @@ def is_slurm_queues_change(change):
     return any(path.startswith("SlurmQueues[") for path in change.path)
 
 
+def is_slurm_scheduler(patch):
+    return patch.cluster.stack.scheduler == SLURM
+
+
+def is_awsbatch_scheduler(_, patch):
+    return patch.cluster.stack.scheduler == AWSBATCH
+
+
+def is_stop_required_for_shared_storage(change):
+    """
+    Cluster stop is required for unmount operation.
+
+    1. Remove managed/external EBS EFS and FSx sections, which indicates unmount operation.
+    2. Change MountDir, which indicates unmount operation.
+    """
+    if change.is_list and change.key == "SharedStorage" and change.old_value is not None and change.new_value is None:
+        return True
+    elif not change.is_list and change.key == "MountDir":
+        return True
+    return False
+
+
+def fail_reason_shared_storage_update_policy(change, patch):
+    if is_awsbatch_scheduler(change, patch):
+        return f"Update actions are not currently supported for the '{change.key}' parameter"
+    reason = "All compute nodes must be stopped"
+    # QueueUpdateStrategy can override UpdatePolicy of parameters under SlurmQueues
+    if not is_stop_required_for_shared_storage(change) and is_slurm_scheduler(patch):
+        reason += " or QueueUpdateStrategy must be set"
+
+    return reason
+
+
 def fail_reason_queue_update_strategy(change, _):
     reason = "All compute nodes must be stopped"
     # QueueUpdateStrategy can override UpdatePolicy of parameters under SlurmQueues
@@ -149,11 +182,48 @@ def condition_checker_queue_update_strategy(change, patch):
     return result
 
 
+def actions_needed_shared_storage_update(change, patch):
+    if is_awsbatch_scheduler(change, patch):
+        return (
+            f"Restore '{change.key}' value to '{change.old_value}'"
+            if change.old_value is not None
+            else "{0} the parameter '{1}'".format("Restore" if change.is_list else "Remove", change.key)
+            + ". If you need this change, please consider creating a new cluster instead of updating the existing one."
+        )
+    actions = "Stop the compute fleet with the pcluster update-compute-fleet command"
+    if not is_stop_required_for_shared_storage(change) and is_slurm_scheduler(patch):
+        actions += ", or set QueueUpdateStrategy in the configuration used for the 'update-cluster' operation"
+
+    return actions
+
+
+def condition_checker_shared_storage_update_policy(change, patch):
+    """
+    Check different requirements for different schedulers.
+
+    Compute fleet stop is required for plugin scheduler.
+    Update for awsbatch scheduler is not supported.
+    QueueUpdateStrategy can override UpdatePolicy of parameters under SlurmQueues for slurm scheduler.
+    """
+    if is_awsbatch_scheduler(change, patch):
+        return False
+    result = not patch.cluster.has_running_capacity()
+    if is_slurm_scheduler(patch) and not is_stop_required_for_shared_storage(change):
+        result = (
+            result
+            or patch.target_config.get("Scheduling")
+            .get("SlurmSettings", {})
+            .get("QueueUpdateStrategy", QueueUpdateStrategy.COMPUTE_FLEET_STOP.value)
+            != QueueUpdateStrategy.COMPUTE_FLEET_STOP.value
+        )
+
+    return result
+
+
 # Common fail_reason messages
 UpdatePolicy.FAIL_REASONS = {
     "ebs_volume_resize": "Updating the file system after a resize operation requires commands specific to your "
     "operating system.",
-    "shared_storage_change": "Shared Storage cannot be added or removed during a 'pcluster update-cluster' operation",
     "cookbook_update": lambda change, patch: (
         "Updating cookbook related parameter is not supported because it only "
         "applies updates to compute nodes. If you still want to proceed, first stop the compute fleet with the "
@@ -169,6 +239,7 @@ def condition_checker_queue_update_strategy(change, patch):
     ),
     "pcluster_stop": lambda change, patch: "Stop the compute fleet with the pcluster update-compute-fleet command",
     "pcluster_stop_conditional": actions_needed_queue_update_strategy,
+    "shared_storage_update_conditional": actions_needed_shared_storage_update,
 }
 
 # Base policies
@@ -220,6 +291,15 @@ def condition_checker_queue_update_strategy(change, patch):
     condition_checker=condition_checker_queue_update_strategy,
 )
 
+# Update policy for updating SharedStorage
+UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY = UpdatePolicy(
+    name="SHARED_STORAGE_UPDATE_POLICY",
+    level=6 if not is_awsbatch_scheduler else 1000,
+    fail_reason=fail_reason_shared_storage_update_policy,
+    action_needed=UpdatePolicy.ACTIONS_NEEDED["shared_storage_update_conditional"],
+    condition_checker=condition_checker_shared_storage_update_policy,
+)
+
 # Update supported on new addition or on removal only with all compute nodes down
 UpdatePolicy.COMPUTE_FLEET_STOP_ON_REMOVE = UpdatePolicy(
     name="COMPUTE_FLEET_STOP_ON_REMOVE",

cli/src/pcluster/constants.py

@@ -25,6 +25,8 @@
 DELETION_POLICIES_WITH_SNAPSHOT = DELETION_POLICIES + ["Snapshot"]
 SUPPORTED_ARCHITECTURES = ["x86_64", "arm64"]
 SUPPORTED_OSES_FOR_ARCHITECTURE = {"x86_64": SUPPORTED_OSES, "arm64": SUPPORTED_OSES}
+SLURM = "slurm"
+AWSBATCH = "awsbatch"
 
 OS_MAPPING = {
     "centos7": {"user": "centos", "root-device": "/dev/sda1"},

cli/src/pcluster/schemas/cluster_schema.py

@@ -384,11 +384,13 @@ class FsxLustreSettingsSchema(BaseSchema):
     )
     per_unit_storage_throughput = fields.Int(metadata={"update_policy": UpdatePolicy.UNSUPPORTED})
     backup_id = fields.Str(
-        validate=validate.Regexp("^(backup-[0-9a-f]{8,})$"), metadata={"update_policy": UpdatePolicy.UNSUPPORTED}
+        validate=validate.Regexp("^(backup-[0-9a-f]{8,})$"),
+        metadata={"update_policy": UpdatePolicy.UNSUPPORTED},
     )
     kms_key_id = fields.Str(metadata={"update_policy": UpdatePolicy.UNSUPPORTED})
     file_system_id = fields.Str(
-        validate=validate.Regexp(r"^fs-[0-9a-z]{17}$"), metadata={"update_policy": UpdatePolicy.UNSUPPORTED}
+        validate=validate.Regexp(r"^fs-[0-9a-z]{17}$"),
+        metadata={"update_policy": UpdatePolicy.UNSUPPORTED},
     )
     auto_import_policy = fields.Str(
         validate=validate.OneOf(["NEW", "NEW_CHANGED", "NEW_CHANGED_DELETED"]),
@@ -469,7 +471,9 @@ class SharedStorageSchema(BaseSchema):
     """Represent the generic SharedStorage schema."""
 
     mount_dir = fields.Str(
-        required=True, validate=get_field_validator("file_path"), metadata={"update_policy": UpdatePolicy.UNSUPPORTED}
+        required=True,
+        validate=get_field_validator("file_path"),
+        metadata={"update_policy": UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY},
     )
     name = fields.Str(required=True, metadata={"update_policy": UpdatePolicy.UNSUPPORTED})
     storage_type = fields.Str(
@@ -1935,9 +1939,7 @@ class ClusterSchema(BaseSchema):
         SharedStorageSchema,
         many=True,
         metadata={
-            "update_policy": UpdatePolicy(
-                UpdatePolicy.UNSUPPORTED, fail_reason=UpdatePolicy.FAIL_REASONS["shared_storage_change"]
-            ),
+            "update_policy": UpdatePolicy(UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY),
             "update_key": "Name",
         },
     )

cli/src/pcluster/templates/cdk_builder_utils.py

@@ -491,7 +491,7 @@ def _build_policy(self) -> List[iam.PolicyStatement]:
             ),
             iam.PolicyStatement(
                 sid="Ec2TagsAndVolumes",
-                actions=["ec2:AttachVolume", "ec2:CreateTags"],
+                actions=["ec2:AttachVolume", "ec2:CreateTags", "ec2:DetachVolume"],
                 effect=iam.Effect.ALLOW,
                 resources=[
                     self._format_arn(

cli/tests/pcluster/config/test_config_patch.py

@@ -423,15 +423,19 @@ def _test_less_target_sections(base_conf, target_conf):
                 _get_storage_by_name(base_conf, "ebs1"),
                 None,
                 UpdatePolicy(
-                    UpdatePolicy.UNSUPPORTED,
-                    name="UNSUPPORTED",
-                    fail_reason=(
-                        "Shared Storage cannot be added or removed during a 'pcluster update-cluster' operation"
-                    ),
+                    UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY,
+                    name="SHARED_STORAGE_UPDATE_POLICY",
                 ),
                 is_list=True,
             ),
-            Change(["SharedStorage[ebs2]"], "MountDir", "vol2", "vol1", UpdatePolicy.UNSUPPORTED, is_list=False),
+            Change(
+                ["SharedStorage[ebs2]"],
+                "MountDir",
+                "vol2",
+                "vol1",
+                UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY,
+                is_list=False,
+            ),
             Change(["SharedStorage[ebs2]", "EbsSettings"], "Iops", None, 20, UpdatePolicy.SUPPORTED, is_list=False),
             Change(
                 ["SharedStorage[ebs2]", "EbsSettings"],
@@ -524,14 +528,18 @@ def _test_more_target_sections(base_conf, target_conf):
                 None,
                 _get_storage_by_name(target_conf, "ebs1"),
                 UpdatePolicy(
-                    UpdatePolicy.UNSUPPORTED,
-                    fail_reason=(
-                        "Shared Storage cannot be added or removed during a 'pcluster update-cluster' operation"
-                    ),
+                    UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY,
                 ),
                 is_list=True,
             ),
-            Change(["SharedStorage[ebs2]"], "MountDir", "vol2", "vol1", UpdatePolicy.UNSUPPORTED, is_list=False),
+            Change(
+                ["SharedStorage[ebs2]"],
+                "MountDir",
+                "vol2",
+                "vol1",
+                UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY,
+                is_list=False,
+            ),
             Change(["SharedStorage[ebs2]", "EbsSettings"], "Iops", None, 20, UpdatePolicy.SUPPORTED, is_list=False),
             Change(
                 ["SharedStorage[ebs2]", "EbsSettings"],
@@ -588,10 +596,15 @@ def _test_incompatible_ebs_sections(base_conf, target_conf):
         target_conf,
         [
             Change(
-                ["SharedStorage[ebs1]"], "MountDir", "vol1", "new_value", UpdatePolicy(UpdatePolicy.UNSUPPORTED), False
+                ["SharedStorage[ebs1]"],
+                "MountDir",
+                "vol1",
+                "new_value",
+                UpdatePolicy(UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY),
+                False,
             )
         ],
-        UpdatePolicy.UNSUPPORTED,
+        UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY,
     )
 
 
@@ -615,9 +628,8 @@ def _test_different_names(base_conf, target_conf):
     assert_that(_get_storage_by_name(target_conf, "ebs1")).is_none()
     assert_that(_get_storage_by_name(target_conf, "ebs-")).is_none()
 
-    unsupported_update_policy = UpdatePolicy(
-        UpdatePolicy.UNSUPPORTED,
-        fail_reason="Shared Storage cannot be added or removed during a 'pcluster update-cluster' operation",
+    shared_storage_update_policy = UpdatePolicy(
+        UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY,
     )
 
     # The patch should contain 5 differences:
@@ -627,12 +639,12 @@ def _test_different_names(base_conf, target_conf):
         base_conf,
         target_conf,
         [
-            Change([], "SharedStorage", None, target_ebs_1_section, unsupported_update_policy, is_list=True),
-            Change([], "SharedStorage", None, target_ebs_2_section, unsupported_update_policy, is_list=True),
-            Change([], "SharedStorage", base_ebs_1_section, None, unsupported_update_policy, is_list=True),
-            Change([], "SharedStorage", base_ebs_2_section, None, unsupported_update_policy, is_list=True),
+            Change([], "SharedStorage", None, target_ebs_1_section, shared_storage_update_policy, is_list=True),
+            Change([], "SharedStorage", None, target_ebs_2_section, shared_storage_update_policy, is_list=True),
+            Change([], "SharedStorage", base_ebs_1_section, None, shared_storage_update_policy, is_list=True),
+            Change([], "SharedStorage", base_ebs_2_section, None, shared_storage_update_policy, is_list=True),
         ],
-        UpdatePolicy.UNSUPPORTED,
+        UpdatePolicy.SHARED_STORAGE_UPDATE_POLICY,
     )