Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ CHANGELOG
- Pull Amazon Linux Docker images from ECR when building docker image for `awsbatch` scheduler. This only applies to
images built for `x86` architecture.
- Use inclusive language in user facing messages and internal naming convention.
- Change the default of instance types from the hardcoded `t2.micro` to the free tier instance type
(`t2.micro` or `t3.micro` dependent on region). In regions without free tier, the default is `t3.micro`.


**BUG FIXES**

Expand Down
29 changes: 29 additions & 0 deletions cli/src/pcluster/config/cfn_param_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
error,
get_availability_zone_of_subnet,
get_cfn_param,
get_default_instance_type,
get_ebs_snapshot_info,
get_efs_mount_target_id,
get_file_section_name,
Expand Down Expand Up @@ -854,6 +855,34 @@ def to_cfn(self):
return cfn_params


class ComputeInstanceTypeCfnParam(CfnParam):
"""
Class to manage the compute instance type parameter.

We need this class in order to set the default instance type from a boto3 call.
"""

def refresh(self):
"""Get default value from a boto3 call for free tier instance type."""
if not self.value:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a shorter syntax like this:

self.value = get_default_instance_type() if pcluster_config.get_section("cluster").get_param_value("scheduler") != "awsbatch" else "optimal"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

scheduler = self.pcluster_config.get_section("cluster").get_param_value("scheduler")
if scheduler:
self.value = "optimal" if scheduler == "awsbatch" else get_default_instance_type()


class HeadNodeInstanceTypeCfnParam(CfnParam):
"""
Class to manage the head node instance type parameter.

We need this class in order to set the default instance type from a boto3 call.
"""

def refresh(self):
"""Get default value from a boto3 call for free tier instance type."""
if not self.value:
self.value = get_default_instance_type()


class TagsParam(JsonCfnParam):
"""
Class to manage the tags json configuration parameter.
Expand Down
24 changes: 12 additions & 12 deletions cli/src/pcluster/config/mappings.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@
ClusterCfnSection,
ClusterConfigMetadataCfnParam,
ComputeAvailabilityZoneCfnParam,
ComputeInstanceTypeCfnParam,
DisableHyperThreadingCfnParam,
EBSSettingsCfnParam,
EFSCfnSection,
ExtraJsonCfnParam,
FloatCfnParam,
HeadNodeAvailabilityZoneCfnParam,
HeadNodeInstanceTypeCfnParam,
IntCfnParam,
MaintainInitialSizeCfnParam,
NetworkInterfacesCountCfnParam,
Expand Down Expand Up @@ -753,14 +755,6 @@
"validators": [ec2_key_pair_validator],
"update_policy": UpdatePolicy.UNSUPPORTED
}),
("base_os", {
"type": BaseOSCfnParam,
"cfn_param_mapping": "BaseOS",
"allowed_values": ["alinux", "alinux2", "ubuntu1604", "ubuntu1804", "centos7", "centos8"],
"validators": [base_os_validator, architecture_os_validator],
"required": True,
"update_policy": UpdatePolicy.UNSUPPORTED
}),
("scheduler", {
"cfn_param_mapping": "Scheduler",
"allowed_values": ["awsbatch", "sge", "slurm", "torque"],
Expand All @@ -770,7 +764,7 @@
}),
# Head node
("master_instance_type", {
"default": "t2.micro",
"type": HeadNodeInstanceTypeCfnParam,
"cfn_param_mapping": "MasterInstanceType",
"validators": [head_node_instance_type_validator, ec2_instance_type_validator],
"update_policy": UpdatePolicy.UNSUPPORTED,
Expand All @@ -786,6 +780,14 @@
action_needed=UpdatePolicy.ACTIONS_NEEDED["ebs_volume_update"]
)
}),
("base_os", {
"type": BaseOSCfnParam,
"cfn_param_mapping": "BaseOS",
"allowed_values": ["alinux", "alinux2", "ubuntu1604", "ubuntu1804", "centos7", "centos8"],
"validators": [base_os_validator, architecture_os_validator],
"required": True,
"update_policy": UpdatePolicy.UNSUPPORTED
}),
# Compute fleet
("compute_root_volume_size", {
"type": IntCfnParam,
Expand Down Expand Up @@ -1024,9 +1026,7 @@
}),
# Compute fleet
("compute_instance_type", {
"default":
lambda section:
"optimal" if section and section.get_param_value("scheduler") == "awsbatch" else "t2.micro",
"type": ComputeInstanceTypeCfnParam,
"cfn_param_mapping": "ComputeInstanceType",
"validators": [compute_instance_type_validator, instances_architecture_compatibility_validator],
"update_policy": UpdatePolicy.COMPUTE_FLEET_STOP
Expand Down
3 changes: 2 additions & 1 deletion cli/src/pcluster/config/param_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import re
import sys
from abc import abstractmethod
from collections import OrderedDict
from enum import Enum

from configparser import NoSectionError
Expand Down Expand Up @@ -441,7 +442,7 @@ def __init__(self, section_definition, pcluster_config, section_label=None, pare
self.parent_section = parent_section

# initialize section parameters with default values
self.params = {}
self.params = OrderedDict({})
self._from_definition()

@property
Expand Down
13 changes: 10 additions & 3 deletions cli/src/pcluster/configure/easyconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
from pcluster.configure.utils import get_regions, get_resource_tag, handle_client_exception, prompt, prompt_iterable
from pcluster.utils import (
error,
get_default_instance_type,
get_region,
get_supported_az_for_multi_instance_types,
get_supported_az_for_one_instance_type,
Expand Down Expand Up @@ -122,7 +123,7 @@ def configure(args):
if pcluster_config.cluster_model == ClusterModel.HIT:
error(
"Configuration in file {0} cannot be overwritten. Please specify a different file path".format(
args.config_file
pcluster_config.config_file
)
)

Expand Down Expand Up @@ -367,16 +368,22 @@ def prompt_os(self):

def prompt_instance_types(self):
"""Ask for head_node_instance_type and compute_instance_type (if necessary)."""
default_head_node_instance_type = self.cluster_section.get_param_value("master_instance_type")
if not default_head_node_instance_type:
default_head_node_instance_type = get_default_instance_type()
self.head_node_instance_type = prompt(
"Head node instance type",
lambda x: _is_instance_type_supported_for_head_node(x) and x in get_supported_instance_types(),
default_value=self.cluster_section.get_param_value("master_instance_type"),
default_value=default_head_node_instance_type,
)
if not self.is_aws_batch:
default_compute_instance_type = self.cluster_section.get_param_value("compute_instance_type")
if not default_compute_instance_type:
default_compute_instance_type = get_default_instance_type()
self.compute_instance_type = prompt(
"Compute instance type",
lambda x: x in get_supported_compute_instance_types(self.scheduler),
default_value=self.cluster_section.get_param_value("compute_instance_type"),
default_value=default_compute_instance_type,
)
# Cache availability zones offering the selected instance type(s) for later use
self.cache_qualified_az()
Expand Down
9 changes: 0 additions & 9 deletions cli/src/pcluster/createami.py
Original file line number Diff line number Diff line change
Expand Up @@ -358,12 +358,3 @@ def create_ami(args):
_print_create_ami_results(results)
if "tmp_dir" in locals() and tmp_dir:
rmtree(tmp_dir)


def _get_default_template_url(region):
return (
"https://{REGION}-aws-parallelcluster.s3.{REGION}.amazonaws.com{SUFFIX}/templates/"
"aws-parallelcluster-{VERSION}.cfn.json".format(
REGION=region, SUFFIX=".cn" if region.startswith("cn") else "", VERSION=utils.get_installed_version()
)
)
4 changes: 2 additions & 2 deletions cli/src/pcluster/examples/config
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ key_name = mykey
# (defaults to https://s3.amazonaws.com/<aws_region_name>-aws-parallelcluster/templates/aws-parallelcluster-<version>.cfn.json)
#template_url = https://s3.amazonaws.com/<aws_region_name>-aws-parallelcluster/templates/aws-parallelcluster-<version>.cfn.json
# EC2 instance type for head node
# (defaults to t2.micro)
# (defaults to the free tier instance type of the region. If the region does not have free tier, default to t3.micro)
#master_instance_type = t2.micro
# EC2 instance type for compute nodes
# (defaults to t2.micro , 'optimal' when scheduler is awsbatch)
# (defaults to the free tier instance type of the region, 'optimal' when scheduler is awsbatch)
#compute_instance_type = t2.micro
# Initial number of EC2 instances to launch as compute nodes in the cluster for schedulers other than awsbatch.
# (defaults to 2)
Expand Down
20 changes: 20 additions & 0 deletions cli/src/pcluster/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1170,6 +1170,26 @@ def get_ebs_snapshot_info(ebs_snapshot_id, raise_exceptions=False):
)


def get_default_instance_type():
"""If current region support free tier, return the free tier instance type. Otherwise, return t3.micro ."""
if not hasattr(get_default_instance_type, "cache"):
get_default_instance_type.cache = {}
cache = get_default_instance_type.cache
region = os.environ.get("AWS_DEFAULT_REGION")
if region not in cache:
free_tier_instance_type = []
for page in paginate_boto3(
boto3.client("ec2").describe_instance_types,
Filters=[
{"Name": "free-tier-eligible", "Values": ["true"]},
{"Name": "current-generation", "Values": ["true"]},
],
):
free_tier_instance_type.append(page)
cache[region] = free_tier_instance_type[0]["InstanceType"] if free_tier_instance_type else "t3.micro"
return cache[region]


class Cache:
"""Simple utility class providing a cache mechanism for expensive functions."""

Expand Down
14 changes: 14 additions & 0 deletions cli/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ def clear_env():
del os.environ["AWS_DEFAULT_REGION"]


@pytest.fixture(autouse=True)
def mock_default_instance(mocker, request):
"""
Mock get_default_instance_type for all tests.

To disable the mock for certain tests, add annotation `@pytest.mark.nomockdefaultinstance` to the tests.
To disable the mock for an entire file, declare global var `pytestmark = pytest.mark.noassertnopendingresponses`
"""
if "nomockdefaultinstance" in request.keywords:
# skip mocking
return
mocker.patch("pcluster.config.cfn_param_types.get_default_instance_type", return_value="t2.micro")


@pytest.fixture
def failed_with_message(capsys):
"""Assert that the command exited with a specific error message."""
Expand Down
6 changes: 3 additions & 3 deletions cli/tests/pcluster/config/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@
"shared_dir": "/shared",
"placement_group": None,
"placement": "compute",
"master_instance_type": "t2.micro",
"master_instance_type": None,
"master_root_volume_size": 25,
"compute_instance_type": "t2.micro",
"compute_instance_type": None,
"compute_root_volume_size": 25,
"initial_queue_size": 0,
"max_queue_size": 10,
Expand Down Expand Up @@ -155,7 +155,7 @@
"base_os": None, # base_os does not have a default, but this is here to make testing easier
"scheduler": None, # The cluster does not have a default, but this is here to make testing easier
"shared_dir": "/shared",
"master_instance_type": "t2.micro",
"master_instance_type": None,
"master_root_volume_size": 25,
"compute_root_volume_size": 25,
"proxy_server": None,
Expand Down
12 changes: 9 additions & 3 deletions cli/tests/pcluster/config/test_section_cluster.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
"base_os": "alinux2",
"scheduler": "slurm",
"cluster_config_metadata": {"sections": {"cluster": ["custom_cluster_label"]}},
"master_instance_type": "t2.micro",
"compute_instance_type": "t2.micro",
},
),
"custom_cluster_label",
Expand All @@ -50,6 +52,8 @@
"additional_iam_policies": ["arn:aws:iam::aws:policy/CloudWatchAgentServerPolicy"],
"base_os": "alinux2",
"scheduler": "slurm",
"master_instance_type": "t2.micro",
"compute_instance_type": "t2.micro",
},
),
"default",
Expand Down Expand Up @@ -86,6 +90,8 @@
"arn:aws:iam::aws:policy/AWSBatchFullAccess",
"arn:aws:iam::aws:policy/CloudWatchAgentServerPolicy",
],
"master_instance_type": "t2.micro",
"compute_instance_type": "t2.micro",
},
),
"default",
Expand Down Expand Up @@ -267,7 +273,6 @@ def test_hit_cluster_section_from_file(mocker, config_parser_dict, expected_dict
("placement", "cluster", "cluster", None),
# Head node
# TODO add regex for master_instance_type
("master_instance_type", None, "t2.micro", None),
("master_instance_type", "", "", None),
("master_instance_type", "test", "test", None),
("master_instance_type", "NONE", "NONE", None),
Expand All @@ -281,7 +286,6 @@ def test_hit_cluster_section_from_file(mocker, config_parser_dict, expected_dict
("master_root_volume_size", "31", 31, None),
# Compute fleet
# TODO add regex for compute_instance_type
("compute_instance_type", None, "t2.micro", None),
("compute_instance_type", "", "", None),
("compute_instance_type", "test", "test", None),
("compute_instance_type", "NONE", "NONE", None),
Expand Down Expand Up @@ -543,7 +547,6 @@ def test_sit_cluster_param_from_file(
("shared_dir", "NONE", "NONE", None), # NONE is evaluated as a valid path
# Head node
# TODO add regex for master_instance_type
("master_instance_type", None, "t2.micro", None),
("master_instance_type", "", "", None),
("master_instance_type", "test", "test", None),
("master_instance_type", "NONE", "NONE", None),
Expand Down Expand Up @@ -801,6 +804,9 @@ def test_sit_cluster_section_to_file(mocker, section_dict, expected_config_parse
def test_cluster_section_to_cfn(
mocker, cluster_section_definition, section_dict, expected_cfn_params, default_threads_per_core
):
section_dict["master_instance_type"] = "t2.micro"
if cluster_section_definition == CLUSTER_SIT:
section_dict["compute_instance_type"] = "t2.micro"
utils.set_default_values_for_required_cluster_section_params(section_dict)
utils.mock_pcluster_config(mocker)
mocker.patch("pcluster.config.cfn_param_types.get_efs_mount_target_id", return_value="valid_mount_target_id")
Expand Down
59 changes: 59 additions & 0 deletions cli/tests/pcluster/config/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance
# with the License. A copy of the License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "LICENSE.txt" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES
# OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
# limitations under the License.
import os

import pytest
from assertpy import assert_that

from pcluster.utils import get_default_instance_type
from tests.common import MockedBoto3Request


@pytest.fixture()
def boto3_stubber_path():
return "pcluster.utils.boto3"


@pytest.mark.parametrize(
"region, free_tier_instance_type, default_instance_type, stub_boto3",
[
("us-east-1", "t2.micro", "t2.micro", True),
("eu-north-1", "t3.micro", "t3.micro", True),
("us-gov-east-1", None, "t3.micro", True),
# Retrieving free tier instance type again should use cache to reduce boto3 call
("us-east-1", "t2.micro", "t2.micro", False),
("eu-north-1", "t3.micro", "t3.micro", False),
("us-gov-east-1", None, "t3.micro", False),
],
)
@pytest.mark.nomockdefaultinstance
def test_get_default_instance(boto3_stubber, region, free_tier_instance_type, default_instance_type, stub_boto3):
os.environ["AWS_DEFAULT_REGION"] = region
if free_tier_instance_type:
response = {"InstanceTypes": [{"InstanceType": free_tier_instance_type}]}
else:
response = {"InstanceTypes": []}
if stub_boto3:
mocked_requests = [
MockedBoto3Request(
method="describe_instance_types",
response=response,
expected_params={
"Filters": [
{"Name": "free-tier-eligible", "Values": ["true"]},
{"Name": "current-generation", "Values": ["true"]},
]
},
)
]

boto3_stubber("ec2", mocked_requests)
assert_that(get_default_instance_type()).is_equal_to(default_instance_type)
Loading