Fix pcluster configure internal refresh flow

Before this patch the code was loading the configuration file by initializing `PclusterConfig`
with `auto_refresh=True`. This setting means that the internal parameters must be recalculated
every time there is a change in one of the internal sections.

The issue was that the calculation of internal parameters was using the values from the
initial config file and after that, the `pcluster configure` workflow wasn't changing
internal sections (parameters only) so the auto-refresh wasn't triggered anymore.

For this reason there was a piece of code to explicitly reload the config file again after
changing the region. We don't need it anymore because I'm instead explicitly calling the
`refresh` method.

Then, in the `no_awsbatch_*_with_config_file" test we were wrongly expecting the `AWSBatchFullAccess`
policy with `sge` scheduler(!). The test was clearly wrong and was hiding a wrong behaviour.

Now we're executing the `refresh` command right after collecting all the inputs from
the users, so the `additional_iam_policies` will contain the values related to the right
scheduler (`sge`) and not the original one (`awsbatch`).

We're also setting the `auto_refresh=True` before calling the `HitConverter.convert`
because this method can change some internal sections so the internal parameters must be
updated accordingly.

This commit also fixes cases on which `refresh` method was called with default/initial values
(e.g. t2.micro as instance type in regions on which this instance doesn't exist).
Right now we're instead validating the values coming from the user.

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

Author: Enrico Usai

CHANGELOG.md

@@ -76,6 +76,7 @@ CHANGELOG
   - Fix issue when the `cluster` section label changed.
   - Fix issue when `shared_dir` and `ebs_settings` are both configured in the `cluster` section.
   - Fix `cluster` and `cfncluster` compatibility in `extra_json` parameter.
+- Fix `pcluster configure` to avoid using default/initial values for internal parameter initialization.
 - Fix pre/post install script arguments management when using double quotes.
 - Fix a bug that was causing `clustermgtd` and `computemgtd` sleep interval to be incorrectly computed when
   system timezone is not set to UTC.

cli/pcluster/configure/easyconfig.py

@@ -118,7 +118,7 @@ def configure(args):
     if args.config_file and os.path.exists(args.config_file) and not os.path.isfile(args.config_file):
         error("Invalid configuration file path: {0}".format(args.config_file))
 
-    pcluster_config = PclusterConfig(config_file=args.config_file, fail_on_error=False)
+    pcluster_config = PclusterConfig(config_file=args.config_file, fail_on_error=False, auto_refresh=False)
 
     # FIXME: Overriding HIT config files is currently not supported.
     if pcluster_config.cluster_model == ClusterModel.HIT:
@@ -141,10 +141,7 @@ def configure(args):
         default_region = pcluster_config.get_section("aws").get_param_value("aws_region_name")
         aws_region_name = prompt_iterable("AWS Region ID", available_regions, default_value=default_region)
         # Set provided region into os environment for suggestions and validations from here on
-        if os.environ["AWS_DEFAULT_REGION"] != aws_region_name:
-            os.environ["AWS_DEFAULT_REGION"] = aws_region_name
-            # Read config file again, because region change can cause change of the initial values in PclusterConfig
-            pcluster_config = PclusterConfig(config_file=args.config_file, fail_on_error=False)
+        os.environ["AWS_DEFAULT_REGION"] = aws_region_name
     else:
         aws_region_name = args.region
 
@@ -192,6 +189,10 @@ def configure(args):
         param = vpc_section.get_param(param_key)
         param.value = param.get_value_from_string(param_value)
 
+    # Update internal params according to provided parameters and enable auto-refresh before eventual hit conversion
+    pcluster_config.refresh()
+    pcluster_config.auto_refresh = True
+
     # Convert file if needed
     HitConverter(pcluster_config).convert(prepare_to_file=True)
 

cli/tests/pcluster/configure/test_pcluster_configure.py

@@ -504,6 +504,31 @@ def test_region_env_overwrite_region_config(mocker, capsys, test_datadir):
     _run_input_test_with_config(mocker, config, old_config_file, error, output, capsys, with_input=False)
 
 
+def test_unexisting_instance_type(mocker, capsys, test_datadir):
+    """
+    Test configuration file with wrong values that must be overridden by user inputs.
+
+    This test verifies that the validation steps are not performed with initial or default values
+    (e.g. t2.micro as instance type in a region that doesn't support it).
+    """
+    config, error, output = get_file_path(test_datadir)
+    old_config_file = str(test_datadir / "original_config_file.ini")
+
+    MockHandler(mocker)
+
+    _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_available_no_input_no_automation_no_errors_with_config_file(mocker, capsys, test_datadir):
     """
     Testing easy config with user hitting return on all prompts.

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

@@ -13,7 +13,6 @@ 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

cli/tests/pcluster/configure/test_pcluster_configure/test_unexisting_instance_type/error.txt

@@ -0,0 +1,26 @@
+[aws]
+aws_region_name = eu-west-1
+
+[cluster default]
+key_name = key1
+base_os = ubuntu1604
+vpc_settings = default
+scheduler = slurm
+master_instance_type = WRONG-INSTANCE-TYPE
+compute_instance_type = WRONG-INSTANCE-TYPE
+max_queue_size = 14
+initial_queue_size = 13
+maintain_initial_size = true
+
+[vpc default]
+vpc_id = vpc-12345678
+master_subnet_id = subnet-12345678
+compute_subnet_id = subnet-23456789
+
+[global]
+cluster_template = default
+update_check = true
+sanity_check = true
+
+[aliases]
+ssh = ssh {CFN_USER}@{MASTER_IP} {ARGS}

cli/tests/pcluster/configure/test_pcluster_configure/test_unexisting_instance_type/original_config_file.ini

@@ -0,0 +1,61 @@
+WARNING: Configuration file {{ CONFIG_FILE }} will be overwritten.
+Press CTRL-C to interrupt the procedure.
+
+
+Allowed values for AWS Region ID:
+1. eu-north-1
+2. ap-south-1
+3. eu-west-3
+4. eu-west-2
+5. eu-west-1
+6. ap-northeast-2
+7. ap-northeast-1
+8. sa-east-1
+9. ca-central-1
+10. ap-southeast-1
+11. ap-southeast-2
+12. eu-central-1
+13. us-east-1
+14. us-east-2
+15. us-west-1
+16. us-west-2
+Allowed values for EC2 Key Pair Name:
+1. key1
+2. key2
+3. key3
+4. key4
+5. key5
+6. key6
+Allowed values for Scheduler:
+1. sge
+2. torque
+3. slurm
+4. awsbatch
+Allowed values for Operating System:
+1. alinux
+2. alinux2
+3. centos7
+4. centos8
+5. ubuntu1604
+6. ubuntu1804
+Allowed values for VPC ID:
+  #  id            name                                 number_of_subnets
+---  ------------  ---------------------------------  -------------------
+  1  vpc-12345678  ParallelClusterVPC-20190625135738                    2
+  2  vpc-23456789  ParallelClusterVPC-20190624105051                    0
+  3  vpc-34567891  default                                              3
+  4  vpc-45678912  ParallelClusterVPC-20190626095403                    1
+Allowed values for Master Subnet ID:
+  #  id               name      size  availability_zone
+---  ---------------  ------  ------  -------------------
+  1  subnet-34567891            4096  eu-west-1b
+  2  subnet-45678912            4096  eu-west-1a
+  3  subnet-56789123            4096  eu-west-1c
+Allowed values for Compute Subnet ID:
+  #  id               name      size  availability_zone
+---  ---------------  ------  ------  -------------------
+  1  subnet-34567891            4096  eu-west-1b
+  2  subnet-45678912            4096  eu-west-1a
+  3  subnet-56789123            4096  eu-west-1c
+Configuration file written to {{ CONFIG_FILE }}
+You can edit your configuration file or simply run 'pcluster create -c {{ CONFIG_FILE }} cluster-name' to create your cluster

cli/tests/pcluster/configure/test_pcluster_configure/test_unexisting_instance_type/output.txt

@@ -0,0 +1,33 @@
+[aws]
+aws_region_name = us-east-1
+
+[cluster default]
+key_name = key2
+base_os = ubuntu1604
+vpc_settings = default
+scheduler = slurm
+master_instance_type = m6g.xlarge
+queue_settings = compute
+
+[vpc default]
+vpc_id = vpc-34567891
+master_subnet_id = subnet-34567891
+compute_subnet_id = subnet-45678912
+
+[global]
+cluster_template = default
+update_check = true
+sanity_check = true
+
+[aliases]
+ssh = ssh {CFN_USER}@{MASTER_IP} {ARGS}
+
+[queue compute]
+enable_efa = false
+enable_efa_gdr = false
+compute_resource_settings = default
+
+[compute_resource default]
+instance_type = m6g.xlarge
+min_count = 7
+max_count = 18