Add logic to cleanup pcluster_config into the convert utility

Before of this patch we were cleaning up the config_parser
only when executin the pcluster configure utility.

By adding the prepare_to_file function we're sure to have
the pcluster_config ready to be written in the file.

## Conversion logic
When a default is equal to default should not be written in the file.
It means: disable_hyperthreading = false, initial_count = min_count and min_count = 0
Instead enable_efa in the queue will be always written because the default is None

## Tests
Improved unit tests by covering all the enable_efa and dysable_hyperthreading cases.

Signed-off-by: Enrico Usai <[email protected]>

cli/pcluster/config/hit_converter.py

@@ -25,7 +25,7 @@ class HitConverter:
     def __init__(self, pcluster_config):
         self.pcluster_config = pcluster_config
 
-    def convert(self):
+    def convert(self, prepare_to_file=False):
         """
         Convert the pcluster_config instance from pre-HIT to HIT configuration model.
 
@@ -138,6 +138,9 @@ def convert(self):
                 self.pcluster_config.auto_refresh = auto_refresh
 
                 self.clean_config_parser(hit_cluster_section)
+                if prepare_to_file:
+                    self._prepare_to_file()
+
                 LOGGER.debug("Conversion to HIT completed successfully.")
                 conversion_done = True
 
@@ -147,6 +150,28 @@ def _copy_param_value(self, old_param, new_param, new_value=None):
         """Copy the value from the old param to the new one."""
         new_param.value = new_value if new_value is not None else old_param.value
 
+    @staticmethod
+    def _reset_config_params(section, parameters_to_remove):
+        for param_key in parameters_to_remove:
+            param = section.get_param(param_key)
+            param.value = param.get_default_value()
+
+    def _prepare_to_file(self):
+        """Reset some parameters from config file since their values can be inferred at runtime."""
+        # disable_hyperthreading will get its value from cluster section
+        queue_section = self.pcluster_config.get_section("queue", "compute")
+        self._reset_config_params(queue_section, ["disable_hyperthreading"])
+
+        # Remove initial_count if not needed
+        compute_section = self.pcluster_config.get_section("compute_resource", "default")
+        if compute_section.get_param_value("initial_count") == compute_section.get_param_value("min_count"):
+            self._reset_config_params(compute_section, ["initial_count"])
+
+        # cluster's disable_hyperthreading's HIT default is None instead of False
+        cluster_section = self.pcluster_config.get_section("cluster")
+        if not cluster_section.get_param_value("disable_hyperthreading"):
+            self._reset_config_params(cluster_section, ["disable_hyperthreading"])
+
     def _store_original_sections(self):
         """
         Store original default sections from configuration.

cli/pcluster/configure/easyconfig.py

@@ -186,7 +186,7 @@ def configure(args):
         param.value = param.get_value_from_string(param_value)
 
     # Convert file if needed
-    _convert_config(pcluster_config)
+    HitConverter(pcluster_config).convert(prepare_to_file=True)
 
     # Update config file by overriding changed settings
     pcluster_config.to_file()
@@ -318,28 +318,6 @@ def _prompt_for_subnet(default_subnet, all_subnets, qualified_subnets, message):
     return prompt_iterable(message, qualified_subnets, default_value=default_subnet)
 
 
-def _convert_config(pcluster_config):
-    """Convert the generated SIT configuration to HIT model if scheduler is Slurm."""
-    if pcluster_config.cluster_model == ClusterModel.SIT:
-        HitConverter(pcluster_config).convert()
-
-        if pcluster_config.cluster_model == ClusterModel.HIT:
-            # Conversion occurred: reset some parameters from config file since their values can be inferred at runtime
-
-            # enable_efa and disable_hyperthreading will get their value from cluster section
-            queue_section = pcluster_config.get_section("queue", "compute")
-            _reset_config_params(queue_section, ("enable_efa", "disable_hyperthreading"))
-
-            # initial_count will take its value from min_count
-            compute_resource_section = pcluster_config.get_section("compute_resource", "default")
-            _reset_config_params(compute_resource_section, ["initial_count"])
-
-            # cluster's disable_hyperthreading's HIT default is None instead of False
-            cluster_section = pcluster_config.get_section("cluster")
-            if not cluster_section.get_param_value("disable_hyperthreading"):
-                _reset_config_params(cluster_section, ["disable_hyperthreading"])
-
-
 class ClusterConfigureHelper:
     """Handle prompts for cluster section."""
 

cli/pcluster_config/cli.py

@@ -81,7 +81,7 @@ def convert(args=None):
         )
 
         # Automatic SIT -> HIT conversion, if needed
-        conversion_done, reason = HitConverter(pcluster_config).convert()
+        conversion_done, reason = HitConverter(pcluster_config).convert(prepare_to_file=True)
         if conversion_done:
             if args.output_file:
                 if os.path.isfile(args.output_file):

cli/tests/pcluster/configure/test_pcluster_configure.py

@@ -334,6 +334,7 @@ def _assert_configurations_are_equal(path_config_expected, path_config_after_inp
     config_expected = ConfigParser()
     config_expected.read(path_config_expected)
     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()}

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

@@ -24,6 +24,7 @@ compute_subnet_id = subnet-23456789
 use_public_ips = false
 
 [queue compute]
+enable_efa = false
 compute_resource_settings = default
 
 [compute_resource default]

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

@@ -22,6 +22,7 @@ vpc_id = vpc-12345678
 master_subnet_id = subnet-12345678
 
 [queue compute]
+enable_efa = false
 compute_resource_settings = default
 
 [compute_resource default]

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

@@ -24,6 +24,7 @@ sanity_check = true
 ssh = ssh {CFN_USER}@{MASTER_IP} {ARGS}
 
 [queue compute]
+enable_efa = false
 compute_resource_settings = default
 
 [compute_resource default]

cli/tests/pcluster_config/test_pcluster_config_convert/test_slurm_sit_full/expected_output.ini

@@ -78,19 +78,16 @@ raid_settings = settings1
 fsx_settings = settings1
 additional_iam_policies = arn:aws:iam::aws:policy/CloudWatchFullAccess,arn:aws:iam::aws:policy/CloudWatchReadAccess
 queue_settings = compute
-disable_hyperthreading = false
 
 [queue compute]
 compute_type = spot
 enable_efa = true
-disable_hyperthreading = false
 placement_group = DYNAMIC
 compute_resource_settings = default
 
 [compute_resource default]
 instance_type = t2.large
 min_count = 1
 max_count = 2
-initial_count = 1
 spot_price = 5.0
 

cli/tests/pcluster_config/test_pcluster_config_convert/test_slurm_sit_simple/expected_output.ini

@@ -22,15 +22,12 @@ shared_dir = /test
 vpc_settings = public
 additional_iam_policies = arn:aws:iam::aws:policy/CloudWatchFullAccess
 queue_settings = compute
-disable_hyperthreading = false
 
 [queue compute]
 enable_efa = false
-disable_hyperthreading = false
 compute_resource_settings = default
 
 [compute_resource default]
 instance_type = g4dn.12xlarge
 max_count = 100
-initial_count = 0
 

cli/tests/pcluster_config/test_pcluster_config_convert/test_slurm_unrelated_sections/expected_output.ini

@@ -78,18 +78,15 @@ raid_settings = settings1
 fsx_settings = settings1
 additional_iam_policies = arn:aws:iam::aws:policy/CloudWatchFullAccess,arn:aws:iam::aws:policy/CloudWatchReadAccess
 queue_settings = compute
-disable_hyperthreading = false
 
 [queue compute]
 compute_type = spot
 enable_efa = true
-disable_hyperthreading = false
 placement_group = DYNAMIC
 compute_resource_settings = default
 
 [compute_resource default]
 instance_type = t2.large
-min_count = 1
 max_count = 2
 initial_count = 1
 spot_price = 5.0

cli/tests/pcluster_config/test_pcluster_config_convert/test_slurm_unrelated_sections/pcluster.config.ini

@@ -22,7 +22,7 @@ compute_instance_type = t2.large
 compute_root_volume_size = 30
 initial_queue_size = 1
 max_queue_size = 2
-maintain_initial_size = true
+maintain_initial_size = false
 cluster_type = spot
 spot_price = 5
 proxy_server = proxy
@@ -48,6 +48,7 @@ ebs_settings = settings1,settings2
 efs_settings = settings1
 raid_settings = settings1
 fsx_settings = settings1
+disable_hyperthreading = compute
 
 [scaling settings1]
 scaledown_idletime = 12