Fix an issue where changes in sequence of custom actions scripts were…

… not detected during cluster updates.

Signed-off-by: Hanwen <[email protected]>

CHANGELOG.md

@@ -10,7 +10,8 @@ CHANGELOG
 - The CLI commands `export-cluster-logs` and `export-image-logs` can now by default export the logs to the default ParallelCluster bucket or to the CustomS3Bucket if specified in the config.
 
 **BUG FIXES**
-- When mounting an external OpenZFS, it is no longer required to set the outbound rules for ports 111, 2049, 20001, 20002, 20003
+- When mounting an external OpenZFS, it is no longer required to set the outbound rules for ports 111, 2049, 20001, 20002, 20003.
+- Fix an issue where changes in sequence of custom actions scripts were not detected during cluster updates. 
 
 3.11.1
 ------

cli/src/pcluster/config/config_patch.py

@@ -137,15 +137,36 @@ def _compare_section(self, base_section: dict, target_section: dict, section_sch
                             )
             else:
                 # Simple param
-                target_value = target_section.get(data_key, None) if target_section else None
-                base_value = base_section.get(data_key, None) if base_section else None
+                target_value, target_data_key = self._get_value_from_section(data_key, target_section)
+                base_value, base_data_key = self._get_value_from_section(data_key, base_section)
+                if target_data_key != base_data_key:
+                    # So far, this only happens when custom actions scripts are changed across simple and sequence.
+                    data_key = param_path.pop()
+                    base_value = {base_data_key: base_value}
+                    target_value = {target_data_key: target_value}
+                else:
+                    # So far, target_data_key is different from data_key only when
+                    # custom actions scripts are in sequence.
+                    data_key = target_data_key
 
                 if target_value != base_value:
                     # Add param change information
                     self.changes.append(
                         Change(param_path, data_key, base_value, target_value, change_update_policy, is_list=False)
                     )
 
+    def _get_value_from_section(self, data_key, section):
+        value = None
+        if section:
+            value = section.get(data_key, None)
+            if data_key == "Script" and value is None:
+                # This handles special case when multiple installation scripts are provided.
+                # The schema of script sequence is customized. The following code correctly detect changes.
+                value = section.get("Sequence", None)
+                if value:
+                    data_key = "Sequence"
+        return value, data_key
+
     def _compare_nested_section(self, param_path, data_key, base_value, target_value, field_obj):
         # Compare nested sections and params
         nested_path = copy.deepcopy(param_path)

cli/tests/pcluster/config/test_config_patch.py

@@ -446,6 +446,35 @@ def _test_no_updatable_custom_actions(base_conf, target_conf):
     )
 
 
+def _test_no_updatable_custom_actions_sequence(base_conf, target_conf):
+    base_conf["HeadNode"].update(
+        {
+            "CustomActions": {
+                "OnNodeConfigured": {"Sequence": [{"Script": "test-to-edit.sh", "Args": ["1", "2"]}]},
+            }
+        }
+    )
+    target_conf["HeadNode"].update(
+        {"CustomActions": {"OnNodeConfigured": {"Sequence": [{"Script": "test-to-edit.sh", "Args": ["2"]}]}}}
+    )
+
+    _check_patch(
+        base_conf,
+        target_conf,
+        [
+            Change(
+                ["HeadNode", "CustomActions", "OnNodeConfigured"],
+                "Sequence",
+                [{"Script": "test-to-edit.sh", "Args": ["1", "2"]}],
+                [{"Script": "test-to-edit.sh", "Args": ["2"]}],
+                UpdatePolicy.UNSUPPORTED,
+                is_list=False,
+            ),
+        ],
+        UpdatePolicy.UNSUPPORTED,
+    )
+
+
 def _test_updatable_custom_actions_attributes(base_conf, target_conf):
     base_conf["HeadNode"].update(
         {"CustomActions": {"OnNodeUpdated": {"Script": "test-to-edit.sh", "Args": ["1", "2"]}}}
@@ -498,6 +527,94 @@ def _test_updatable_custom_actions(base_conf, target_conf):
     )
 
 
+def _test_updatable_custom_actions_sequence_add(base_conf, target_conf):
+    target_conf["HeadNode"].update(
+        {"CustomActions": {"OnNodeUpdated": {"Sequence": [{"Script": "test-to-remove.sh"}]}}}
+    )
+
+    _check_patch(
+        base_conf,
+        target_conf,
+        [
+            Change(
+                ["HeadNode", "CustomActions"],
+                "OnNodeUpdated",
+                "-",
+                {"Sequence": [{"Script": "test-to-remove.sh"}]},
+                UpdatePolicy.SUPPORTED,
+                is_list=False,
+            ),
+        ],
+        UpdatePolicy.SUPPORTED,
+    )
+
+
+def _test_updatable_custom_actions_sequence_change(base_conf, target_conf):
+    base_conf["HeadNode"].update({"CustomActions": {"OnNodeUpdated": {"Sequence": [{"Script": "test-to-remove.sh"}]}}})
+    target_conf["HeadNode"].update(
+        {"CustomActions": {"OnNodeUpdated": {"Sequence": [{"Script": "another-script.sh"}]}}}
+    )
+
+    _check_patch(
+        base_conf,
+        target_conf,
+        [
+            Change(
+                ["HeadNode", "CustomActions", "OnNodeUpdated"],
+                "Sequence",
+                [{"Script": "test-to-remove.sh"}],
+                [{"Script": "another-script.sh"}],
+                UpdatePolicy.SUPPORTED,
+                is_list=False,
+            ),
+        ],
+        UpdatePolicy.SUPPORTED,
+    )
+
+
+def _test_updatable_custom_actions_sequence_remove(base_conf, target_conf):
+    base_conf["HeadNode"].update({"CustomActions": {"OnNodeUpdated": {"Sequence": [{"Script": "test-to-remove.sh"}]}}})
+
+    _check_patch(
+        base_conf,
+        target_conf,
+        [
+            Change(
+                ["HeadNode", "CustomActions"],
+                "OnNodeUpdated",
+                {"Sequence": [{"Script": "test-to-remove.sh"}]},
+                "-",
+                UpdatePolicy.SUPPORTED,
+                is_list=False,
+            ),
+        ],
+        UpdatePolicy.SUPPORTED,
+    )
+
+
+def _test_updatable_custom_actions_single_to_sequence(base_conf, target_conf):
+    base_conf["HeadNode"].update({"CustomActions": {"OnNodeUpdated": {"Script": "test-to-remove.sh"}}})
+    target_conf["HeadNode"].update(
+        {"CustomActions": {"OnNodeUpdated": {"Sequence": [{"Script": "another-script.sh"}]}}}
+    )
+
+    _check_patch(
+        base_conf,
+        target_conf,
+        [
+            Change(
+                ["HeadNode", "CustomActions"],
+                "OnNodeUpdated",
+                {"Script": "test-to-remove.sh"},
+                {"Sequence": [{"Script": "another-script.sh"}]},
+                UpdatePolicy.SUPPORTED,
+                is_list=False,
+            ),
+        ],
+        UpdatePolicy.SUPPORTED,
+    )
+
+
 def _test_less_target_sections(base_conf, target_conf):
     # Remove an ebs section in the target conf
     assert_that(_get_storage_by_name(target_conf, "ebs1")).is_not_none()
@@ -912,8 +1029,13 @@ def _test_iam(base_conf, target_conf):
         _test_compute_resources,
         _test_queues,
         _test_no_updatable_custom_actions,
+        _test_no_updatable_custom_actions_sequence,
         _test_updatable_custom_actions,
         _test_updatable_custom_actions_attributes,
+        _test_updatable_custom_actions_sequence_add,
+        _test_updatable_custom_actions_sequence_change,
+        _test_updatable_custom_actions_sequence_remove,
+        _test_updatable_custom_actions_single_to_sequence,
         _test_storage,
         _test_iam,
     ],