From c65ec5164735676ee074aea57182b92cb7588c29 Mon Sep 17 00:00:00 2001 From: Hanwen Date: Thu, 14 Nov 2024 10:41:00 -0800 Subject: [PATCH] Fix an issue where changes in sequence of custom actions scripts were not detected during cluster updates. Signed-off-by: Hanwen --- CHANGELOG.md | 3 +- cli/src/pcluster/config/config_patch.py | 25 +++- .../pcluster/config/test_config_patch.py | 122 ++++++++++++++++++ 3 files changed, 147 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77c0b802ef..9dc722249d 100644 --- a/CHANGELOG.md +++ b/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 ------ diff --git a/cli/src/pcluster/config/config_patch.py b/cli/src/pcluster/config/config_patch.py index 91513b5844..28b95bc1d6 100644 --- a/cli/src/pcluster/config/config_patch.py +++ b/cli/src/pcluster/config/config_patch.py @@ -137,8 +137,17 @@ 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 @@ -146,6 +155,18 @@ def _compare_section(self, base_section: dict, target_section: dict, section_sch 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) diff --git a/cli/tests/pcluster/config/test_config_patch.py b/cli/tests/pcluster/config/test_config_patch.py index 68f2ac85c6..c7c87fd70c 100644 --- a/cli/tests/pcluster/config/test_config_patch.py +++ b/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, ],