Skip to content

Commit ef8d27a

Browse files
gmarcianigmarciani
committed
[Validators] Add validator ExtraChefAttributesValidator to validate DevSettings/Cookbook/ExtraChefAttributes.
It fails with an error if the attribute `cluster.in_place_update_on_fleet_enabled` is not a boolean value. It fails with a warning if the attribute `cluster.in_place_update_on_fleet_enabled` is false, notifying the user about the consequences of disabling in-place updates.
1 parent c3ab37e commit ef8d27a

File tree

8 files changed

+275
-0
lines changed

8 files changed

+275
-0
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ CHANGELOG
44
3.15.0
55
------
66

7+
**CHANGES**
8+
- Add validator that warns against the downsides of disabling cfn-hup on compute and login nodes.
9+
710
**BUG FIXES**
811
- Reduce EFA installation time for Ubuntu by ~20 minutes by only holding kernel packages for the installed kernel.
912

‎cli/src/pcluster/config/common.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
from typing import List, Set
2222

2323
from pcluster.validators.common import AsyncValidator, FailureLevel, ValidationResult, Validator, ValidatorContext
24+
from pcluster.validators.dev_settings_validators import ExtraChefAttributesValidator
2425
from pcluster.validators.iam_validators import AdditionalIamPolicyValidator
2526
from pcluster.validators.networking_validators import LambdaFunctionsVpcConfigValidator
2627
from pcluster.validators.s3_validators import UrlValidator
@@ -333,6 +334,8 @@ def __init__(self, chef_cookbook: str = None, extra_chef_attributes: str = None)
333334
def _register_validators(self, context: ValidatorContext = None):
334335
if self.chef_cookbook is not None:
335336
self._register_validator(UrlValidator, url=self.chef_cookbook)
337+
if self.extra_chef_attributes:
338+
self._register_validator(ExtraChefAttributesValidator, extra_chef_attributes=self.extra_chef_attributes)
336339

337340

338341
class LambdaFunctionsVpcConfig(Resource):

‎cli/src/pcluster/validators/dev_settings_validators.py‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance
4+
# with the License. A copy of the License is located at
5+
#
6+
# http://aws.amazon.com/apache2.0/
7+
#
8+
# or in the "LICENSE.txt" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES
9+
# OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
10+
# limitations under the License.
11+
import json
12+
13+
from pcluster.validators.common import FailureLevel, Validator
14+
from pcluster.validators.utils import dig, is_boolean_string, str_to_bool
15+
16+
EXTRA_CHEF_ATTRIBUTES_PATH = "DevSettings/Cookbook/ExtraChefAttributes"
17+
ATTR_IN_PLACE_UPDATE_ON_FLEET_ENABLED = "in_place_update_on_fleet_enabled"
18+
19+
20+
class ExtraChefAttributesValidator(Validator):
21+
"""Validate DevSettings/Cookbook/ExtraChefAttributes."""
22+
23+
def _validate(self, extra_chef_attributes: str = None):
24+
"""Validate extra Chef attributes.
25+
26+
Args:
27+
extra_chef_attributes: JSON string containing Chef attributes.
28+
Schema validation ensures this is valid JSON.
29+
"""
30+
if not extra_chef_attributes:
31+
return
32+
else:
33+
self._validate_in_place_update_on_fleet_enabled(json.loads(extra_chef_attributes))
34+
35+
def _validate_in_place_update_on_fleet_enabled(self, extra_chef_attributes: dict = None):
36+
"""Validate attribute cluster.in_place_update_on_fleet_enabled.
37+
38+
It returns an error if the attribute is set to a non-boolean value.
39+
It returns a warning if the in-place update is disabled.
40+
41+
Args:
42+
extra_chef_attributes: Dictionary of Chef attributes to validate.
43+
"""
44+
in_place_update_on_fleet_enabled = dig(extra_chef_attributes, "cluster", ATTR_IN_PLACE_UPDATE_ON_FLEET_ENABLED)
45+
46+
if in_place_update_on_fleet_enabled is None:
47+
return
48+
49+
if not is_boolean_string(str(in_place_update_on_fleet_enabled)):
50+
self._add_failure(
51+
f"Invalid value in {EXTRA_CHEF_ATTRIBUTES_PATH}: "
52+
f"attribute '{ATTR_IN_PLACE_UPDATE_ON_FLEET_ENABLED}' must be a boolean value.",
53+
FailureLevel.ERROR,
54+
)
55+
return
56+
57+
if str_to_bool(str(in_place_update_on_fleet_enabled)) is False:
58+
self._add_failure(
59+
"When in-place updates are disabled, cluster updates are applied "
60+
"by replacing compute and login nodes according to the selected QueueUpdateStrategy.",
61+
FailureLevel.WARNING,
62+
)

‎cli/src/pcluster/validators/utils.py‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,54 @@
1212
# This module contains all the classes representing the Resources objects.
1313
# These objects are obtained from the configuration file through a conversion based on the Schema classes.
1414
#
15+
from typing import Any, Union
16+
17+
BOOLEAN_VALUES = ["true", "false"]
1518

1619

1720
def get_bucket_name_from_s3_url(import_path):
1821
return import_path.split("/")[2]
22+
23+
24+
def str_to_bool(string: str = None) -> bool:
25+
"""Convert string to boolean value.
26+
27+
Args:
28+
string: String to convert. Defaults to None.
29+
30+
Returns:
31+
True if string is "true" (case-insensitive), False otherwise.
32+
"""
33+
return str(string).lower() == "true"
34+
35+
36+
def is_boolean_string(value: str) -> bool:
37+
"""Check if value is a valid boolean string.
38+
39+
Args:
40+
value: String value to check.
41+
42+
Returns:
43+
True if value is "true" or "false" (case-insensitive), False otherwise.
44+
"""
45+
return str(value).lower() in BOOLEAN_VALUES
46+
47+
48+
def dig(dictionary: dict, *keys: str) -> Union[dict, None, Any]:
49+
"""Navigate nested dictionary using key path.
50+
51+
Args:
52+
dictionary: Dictionary to navigate.
53+
*keys: Sequence of keys to traverse.
54+
55+
Returns:
56+
Value at the specified key path, or None if path doesn't exist.
57+
"""
58+
if dictionary is None:
59+
return None
60+
value = dictionary
61+
for key in keys:
62+
if value is None or not isinstance(value, dict):
63+
return None
64+
value = value.get(key)
65+
return value

‎cli/tests/pcluster/validators/test_all_validators.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
from pcluster.validators import (
2020
cluster_validators,
2121
database_validators,
22+
dev_settings_validators,
2223
ebs_validators,
2324
ec2_validators,
2425
feature_validators,
@@ -67,6 +68,7 @@ async def _validate_async(*args, **kwargs):
6768
modules = [
6869
cluster_validators,
6970
database_validators,
71+
dev_settings_validators,
7072
ebs_validators,
7173
ec2_validators,
7274
fsx_validators,
@@ -282,6 +284,10 @@ def test_slurm_validators_are_called_with_correct_argument(test_datadir, mocker)
282284
compute_resource_tags_validator = mocker.patch(
283285
tags_validators + ".ComputeResourceTagsValidator._validate", return_value=[]
284286
)
287+
dev_settings_validators = validators_path + ".dev_settings_validators"
288+
extra_chef_attributes_validator = mocker.patch(
289+
dev_settings_validators + ".ExtraChefAttributesValidator._validate", return_value=[]
290+
)
285291
mocker.patch(
286292
"pcluster.config.cluster_config.HeadNode.architecture", new_callable=PropertyMock(return_value="x86_64")
287293
)
@@ -451,6 +457,8 @@ def test_slurm_validators_are_called_with_correct_argument(test_datadir, mocker)
451457
any_order=True,
452458
)
453459

460+
extra_chef_attributes_validator.assert_has_calls([call(extra_chef_attributes='{"attr1": {"attr2": "value"}}')])
461+
454462
# No assertion on the argument for minor validators
455463
name_validator.assert_called()
456464
feature_region_validator.assert_called()

‎cli/tests/pcluster/validators/test_all_validators/test_slurm_validators_are_called_with_correct_argument/slurm.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,7 @@ Tags:
101101
Value: String
102102
- Key: cluster_tag2
103103
Value: String
104+
DevSettings:
105+
Cookbook:
106+
ExtraChefAttributes: |
107+
{"attr1": {"attr2": "value"}}

‎cli/tests/pcluster/validators/test_dev_settings_validators.py‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance
4+
# with the License. A copy of the License is located at
5+
#
6+
# http://aws.amazon.com/apache2.0/
7+
#
8+
# or in the "LICENSE.txt" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES
9+
# OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
10+
# limitations under the License.
11+
import pytest
12+
13+
from pcluster.validators.common import FailureLevel
14+
from pcluster.validators.dev_settings_validators import ExtraChefAttributesValidator
15+
from tests.pcluster.validators.utils import assert_failure_level, assert_failure_messages
16+
17+
18+
@pytest.mark.parametrize(
19+
"extra_chef_attributes, expected_message, expected_failure_level",
20+
[
21+
pytest.param(None, None, None, id="No extra chef attributes"),
22+
pytest.param('{"other_attribute": "value"}', None, None, id="in_place_update_on_fleet_enabled not set"),
23+
pytest.param('{"cluster": {"in_place_update_on_fleet_enabled": "true"}}', None, None, id="in_place_update_true_string 'true' passes"),
24+
pytest.param('{"cluster": {"in_place_update_on_fleet_enabled": true}}', None, None, id="in_place_update_true_string true passes"),
25+
pytest.param(
26+
'{"cluster": {"in_place_update_on_fleet_enabled": "false"}}',
27+
"When in-place updates are disabled, cluster updates are applied "
28+
"by replacing compute and login nodes according to the selected QueueUpdateStrategy.",
29+
FailureLevel.WARNING,
30+
id="in_place_update_true_string 'false' throws a warning"
31+
),
32+
pytest.param(
33+
'{"cluster": {"in_place_update_on_fleet_enabled": false}}',
34+
"When in-place updates are disabled, cluster updates are applied "
35+
"by replacing compute and login nodes according to the selected QueueUpdateStrategy.",
36+
FailureLevel.WARNING,
37+
id="in_place_update_true_string false throws a warning"
38+
),
39+
pytest.param(
40+
'{"cluster": {"in_place_update_on_fleet_enabled": "invalid"}}',
41+
"Invalid value in DevSettings/Cookbook/ExtraChefAttributes: "
42+
"attribute 'in_place_update_on_fleet_enabled' must be a boolean value.",
43+
FailureLevel.ERROR,
44+
id="in_place_update_true_string invalid (string) throws an error"
45+
),
46+
pytest.param(
47+
'{"cluster": {"in_place_update_on_fleet_enabled": 123}}',
48+
"Invalid value in DevSettings/Cookbook/ExtraChefAttributes: "
49+
"attribute 'in_place_update_on_fleet_enabled' must be a boolean value.",
50+
FailureLevel.ERROR,
51+
id="n_place_update_true_string invalid (number) throws an error"
52+
),
53+
],
54+
)
55+
def test_extra_chef_attributes_validator(extra_chef_attributes, expected_message, expected_failure_level):
56+
actual_failures = ExtraChefAttributesValidator().execute(extra_chef_attributes=extra_chef_attributes)
57+
assert_failure_messages(actual_failures, expected_message)
58+
if expected_failure_level:
59+
assert_failure_level(actual_failures, expected_failure_level)

‎cli/tests/pcluster/validators/test_utils.py‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
# Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance
4+
# with the License. A copy of the License is located at
5+
#
6+
# http://aws.amazon.com/apache2.0/
7+
#
8+
# or in the "LICENSE.txt" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES
9+
# OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
10+
# limitations under the License.
11+
import pytest
12+
13+
from pcluster.validators.utils import dig, is_boolean_string, str_to_bool
14+
15+
16+
@pytest.mark.parametrize(
17+
"string_value, expected_result",
18+
[
19+
# Valid cases
20+
("true", True),
21+
("True", True),
22+
("TRUE", True),
23+
("false", False),
24+
("False", False),
25+
("FALSE", False),
26+
27+
# Invalid cases
28+
("yes", False),
29+
("no", False),
30+
("1", False),
31+
("0", False),
32+
("", False),
33+
(None, False),
34+
],
35+
)
36+
def test_str_to_bool(string_value, expected_result):
37+
assert str_to_bool(string_value) == expected_result
38+
39+
40+
@pytest.mark.parametrize(
41+
"value, expected_result",
42+
[
43+
# Valid cases
44+
("true", True),
45+
("True", True),
46+
("TRUE", True),
47+
("false", True),
48+
("False", True),
49+
("FALSE", True),
50+
(True, True),
51+
(False, True),
52+
53+
# Invalid cases
54+
(None, False),
55+
("", False),
56+
("yes", False),
57+
("no", False),
58+
("1", False),
59+
("0", False),
60+
(1, False),
61+
(0, False),
62+
],
63+
)
64+
def test_is_boolean_string(value, expected_result):
65+
assert is_boolean_string(value) == expected_result
66+
67+
68+
@pytest.mark.parametrize(
69+
"dictionary, keys, expected_result",
70+
[
71+
# Cases where value is found
72+
({"a": {"b": {"c": "value"}}}, ("a", "b", "c"), "value"),
73+
({"a": {"b": "value"}}, ("a", "b"), "value"),
74+
({"a": "value"}, ("a",), "value"),
75+
({"a": {"b": {"c": "value"}}}, ("a", "b"), {"c": "value"}),
76+
77+
# Cases where value is not found
78+
({"a": {"b": {"c": "value"}}}, ("a", "nonexistent"), None),
79+
({"a": {"b": {"c": "value"}}}, ("nonexistent",), None),
80+
({"a": {"b": {"c": "value"}}}, ("a", "b", "c", "d"), None),
81+
({}, ("a",), None),
82+
(None, ("a",), None),
83+
({"a": None}, ("a", "b"), None),
84+
({"a": "not_dict"}, ("a", "b"), None),
85+
({"a": {"b": None}}, ("a", "b", "c"), None),
86+
],
87+
)
88+
def test_dig(dictionary, keys, expected_result):
89+
assert dig(dictionary, *keys) == expected_result

0 commit comments

Comments
 (0)