Prevent writing architecture parameter to config

Previously the architecture parameter was being written back to the
config files produced by `pcluster configure` when the original
`master_instance_type` supported a different architecture than the one
supported by the `master_instance_type` provided via the interactive
prompt. This change prevents that from occurring.

The same is done for the cluster_config_metadata parameter, although it
was not as critical in this case because its presence in a config file
does not cause it to fail validation.

A test case was added to the corresponding unit tests. To ensure the
test case fails when the fix is not in place, an extra check was added
to ensure that only the expected configuration keys are in the resulting
config file.

Signed-off-by: Tim Lane <[email protected]>

Author: Tim Lane

cli/pcluster/config/param_types.py

@@ -888,6 +888,15 @@ def create_section_resources(self, section_key, num_labels, max_resources):
         LOGGER.debug("Automatic labels generated: {0}".format(str(section_labels)))
         return self.__section_resources.resources(section_key)
 
+    def to_file(self, config_parser, write_defaults=False):
+        """Ensure cluster_config_metadata_param is not exposed in the config file."""
+        LOGGER.debug("Ensuring cluster_config_metadata_param parameter is not written to configuration file.")
+        section_name = get_file_section_name(self.section_key, self.section_label)
+        try:
+            config_parser.remove_option(section_name, self.key)
+        except NoSectionError:
+            pass
+
 
 class ArchitectureParam(Param):
     """
@@ -911,6 +920,15 @@ def from_file(self, config_parser):
 
         return self
 
+    def to_file(self, config_parser, write_defaults=False):
+        """Do nothing because architecture is not exposed in the config file."""
+        LOGGER.debug("Ensuring architecture parameter is not written to configuration file.")
+        section_name = get_file_section_name(self.section_key, self.section_label)
+        try:
+            config_parser.remove_option(section_name, self.key)
+        except NoSectionError:
+            pass
+
     @staticmethod
     def get_master_instance_type_architecture(cluster_section):
         """Compute cluster's 'Architecture' CFN parameter based on its master server instance type."""

cli/tests/pcluster/configure/test_pcluster_configure.py

@@ -138,16 +138,22 @@ def _side_effect_function(config, parameters):
 
 
 def _mock_parallel_cluster_config(mocker):
-    supported_instance_types = ["t2.nano", "t2.micro", "t2.large", "c5.xlarge", "g3.8xlarge"]
+    supported_instance_types = ["t2.nano", "t2.micro", "t2.large", "c5.xlarge", "g3.8xlarge", "m6g.xlarge"]
     mocker.patch("pcluster.configure.easyconfig.get_supported_instance_types", return_value=supported_instance_types)
     mocker.patch(
         "pcluster.configure.easyconfig.get_supported_compute_instance_types", return_value=supported_instance_types
     )
     mocker.patch("pcluster.config.param_types.get_avail_zone", return_value="mocked_avail_zone")
-    mocker.patch("pcluster.config.param_types.get_supported_architectures_for_instance_type", return_value=["x86_64"])
+    mocker.patch(
+        "pcluster.config.param_types.get_supported_architectures_for_instance_type",
+        side_effect=lambda instance: ["arm64"] if instance == "m6g.xlarge" else ["x86_64"],
+    )
     # NOTE: the following shouldn't be needed given that easyconfig doesn't validate the config file,
     #       but it's being included in case that changes in the future.
-    mocker.patch("pcluster.config.validators.get_supported_architectures_for_instance_type", return_value=["x86_64"])
+    mocker.patch(
+        "pcluster.config.validators.get_supported_architectures_for_instance_type",
+        side_effect=lambda instance: ["arm64"] if instance == "m6g.xlarge" else ["x86_64"],
+    )
 
 
 def _launch_config(mocker, path, remove_path=True):
@@ -158,43 +164,16 @@ def _launch_config(mocker, path, remove_path=True):
     configure(args)
 
 
-def _are_configurations_equals(path_config_expected, path_config_after_input):
-    if not os.path.isfile(path_config_expected):
-        return False
-    if not os.path.isfile(path_config_after_input):
-        return False
+def _assert_configurations_are_equal(path_config_expected, path_config_after_input):
+    assert_that(path_config_expected).exists().is_file()
+    assert_that(path_config_after_input).exists().is_file()
     config_expected = ConfigParser()
     config_expected.read(path_config_expected)
-    dict1 = {s: dict(config_expected.items(s)) for s in config_expected.sections()}
-    config_temp = ConfigParser()
-    config_temp.read(path_config_after_input)
-    dict2 = {s: dict(config_temp.items(s)) for s in config_temp.sections()}
-    for section_name, section in dict1.items():
-        for key, value in section.items():
-            try:
-                if dict2[section_name][key] != value:
-                    print(
-                        (
-                            "Expected parameters are: {0}\n"
-                            "Actual parameters after running pcluster configure are: {1}"
-                        ).format(dict1, dict2)
-                    )
-                    print(
-                        "\nTest failed: Parameter '{0}' in section '{1}' is different from the expected one.".format(
-                            key, section_name
-                        )
-                    )
-                    print("The actual value is '{0}' but expected is '{1}'".format(dict2[section_name][key], value))
-                    return False
-            except KeyError:
-                print(
-                    "Expected parameters are: {0}\nActual parameters after running pcluster configure are: {1}".format(
-                        dict1, dict2
-                    )
-                )
-                print("\nTest failed: Parameter '{0}' doesn't exist in section '{1}'.".format(key, section_name))
-                return False
-    return True
+    config_expected_dict = {s: dict(config_expected.items(s)) for s in config_expected.sections()}
+    config_actual = ConfigParser()
+    config_actual.read(path_config_after_input)
+    config_actual_dict = {s: dict(config_actual.items(s)) for s in config_actual.sections()}
+    assert_that(config_actual_dict).is_equal_to(config_expected_dict)
 
 
 def _are_output_error_correct(capsys, output, error, config_path):
@@ -271,7 +250,7 @@ def _verify_test(
         _launch_config(mocker, temp_path_for_config, remove_path=False)
     else:
         _launch_config(mocker, temp_path_for_config)
-    assert_that(_are_configurations_equals(expected_config, temp_path_for_config)).is_true()
+    _assert_configurations_are_equal(expected_config, temp_path_for_config)
     _are_output_error_correct(capsys, output, error, temp_path_for_config)
     os.remove(temp_path_for_config)
 
@@ -292,11 +271,25 @@ def test_no_automation_no_awsbatch_no_errors(mocker, capsys, test_datadir):
     _verify_test(mocker, capsys, output, error, config, TEMP_PATH_FOR_CONFIG)
 
 
-def _run_input_test_with_config(mocker, config, old_config_file, error, output, capsys, with_input=False):
+def _run_input_test_with_config(
+    mocker,
+    config,
+    old_config_file,
+    error,
+    output,
+    capsys,
+    with_input=False,
+    master_instance="c5.xlarge",
+    compute_instance="g3.8xlarge",
+):
     if with_input:
         input_composer = ComposeInput(aws_region_name="us-east-1", key="key2", scheduler="slurm")
         input_composer.add_first_flow(
-            op_sys="ubuntu1604", min_size="7", max_size="18", master_instance="c5.xlarge", compute_instance="g3.8xlarge"
+            op_sys="ubuntu1604",
+            min_size="7",
+            max_size="18",
+            master_instance=master_instance,
+            compute_instance=compute_instance,
         )
         input_composer.add_no_automation_no_empty_vpc(
             vpc_id="vpc-34567891", master_id="subnet-34567891", compute_id="subnet-45678912"
@@ -352,7 +345,17 @@ def test_with_input_no_automation_no_errors_with_config_file(mocker, capsys, tes
 
     MockHandler(mocker)
 
-    _run_input_test_with_config(mocker, config, old_config_file, error, output, capsys, with_input=True)
+    _run_input_test_with_config(
+        mocker,
+        config,
+        old_config_file,
+        error,
+        output,
+        capsys,
+        with_input=True,
+        master_instance="m6g.xlarge",
+        compute_instance="m6g.xlarge",
+    )
 
 
 def test_no_automation_yes_awsbatch_no_errors(mocker, capsys, test_datadir):

cli/tests/pcluster/configure/test_pcluster_configure/test_bad_config_file/pcluster.config.ini

@@ -12,7 +12,7 @@ key_name = key1
 vpc_settings = default
 #Invalid param value
 # Implied value
-# scheduler = sge
+scheduler = sge
 base_os = centos6
 # Implied value
 # compute_instance_type = t2.micro

cli/tests/pcluster/configure/test_pcluster_configure/test_no_automation_no_awsbatch_no_errors/pcluster.config.ini

@@ -9,6 +9,7 @@ master_instance_type = t2.nano
 max_queue_size = 14
 initial_queue_size = 13
 maintain_initial_size = true
+base_os = alinux
 
 [vpc default]
 vpc_id = vpc-12345678

cli/tests/pcluster/configure/test_pcluster_configure/test_no_automation_yes_awsbatch_no_errors/pcluster.config.ini

@@ -4,7 +4,7 @@ aws_region_name = eu-west-1
 [cluster default]
 key_name = key1
 # Implied value
-# base_os = alinux
+base_os = alinux2
 vpc_settings = default
 scheduler = awsbatch
 # Implied value

cli/tests/pcluster/configure/test_pcluster_configure/test_no_available_no_input_no_automation_no_errors_with_config_file/pcluster.config.ini

@@ -10,6 +10,8 @@ compute_instance_type = t2.nano
 max_queue_size = 14
 initial_queue_size = 13
 maintain_initial_size = true
+base_os = alinux2
+additional_iam_policies = arn:aws:iam::aws:policy/CloudWatchAgentServerPolicy
 
 [vpc default]
 vpc_id = vpc-abcdefgh

cli/tests/pcluster/configure/test_pcluster_configure/test_no_input_no_automation_no_errors_with_config_file/pcluster.config.ini

@@ -10,6 +10,7 @@ compute_instance_type = t2.nano
 max_queue_size = 14
 initial_queue_size = 13
 maintain_initial_size = true
+base_os = alinux2
 
 [vpc default]
 vpc_id = vpc-12345678

cli/tests/pcluster/configure/test_pcluster_configure/test_subnet_automation_no_awsbatch_no_errors/pcluster.config.ini

@@ -5,7 +5,7 @@ aws_region_name = eu-west-1
 key_name = key1
 vpc_settings = default
 # Implied value
-# scheduler = sge
+scheduler = sge
 base_os = centos6
 # Implied value
 # compute_instance_type = t2.micro

cli/tests/pcluster/configure/test_pcluster_configure/test_subnet_automation_no_awsbatch_no_errors_empty_vpc/pcluster.config.ini

@@ -4,8 +4,7 @@ aws_region_name = eu-west-1
 [cluster default]
 key_name = key1
 vpc_settings = default
-# Implied value
-# scheduler = sge
+scheduler = sge
 base_os = centos6
 # Implied value
 # compute_instance_type = t2.micro

cli/tests/pcluster/configure/test_pcluster_configure/test_subnet_automation_no_awsbatch_no_errors_with_config_file/pcluster.config.ini

@@ -5,14 +5,15 @@ aws_region_name = eu-west-1
 key_name = key1
 vpc_settings = default
 # Implied value
-# scheduler = sge
+scheduler = sge
 base_os = centos6
 # Implied value
 # compute_instance_type = t2.micro
 master_instance_type = t2.nano
 max_queue_size = 14
 initial_queue_size = 13
 maintain_initial_size = true
+additional_iam_policies = arn:aws:iam::aws:policy/AWSBatchFullAccess
 
 [vpc default]
 vpc_id = vpc-12345678