Skip to content

Commit 5379ca3

Browse files
gmarcianigmarciani
committed
[Validation] Fix bug that caused unhelpful validation errors in case DevSettings is null, DevSettings/InstanceTypesData is not a valid JSON or DevSettings/InstanceTypesData is missing required fields.
With this change we replaced the validation error message "'NoneType' object has no attribute 'get'", with a more actionable "Could not determine network cards for instance type ${instanceType}"
1 parent 7f28a4b commit 5379ca3

File tree

5 files changed

+139
-12
lines changed

5 files changed

+139
-12
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ CHANGELOG
1212
**BUG FIXES**
1313
- Reduce EFA installation time for Ubuntu by ~20 minutes by only holding kernel packages for the installed kernel.
1414
- Add GetFunction and GetPolicy permissions to PClusterBuildImageCleanupRole to prevent AccessDenied errors during build image stack deletion.
15+
- Fix validation error messages when `DevSettings` is null or `DevSettings/InstanceTypesData` is missing required fields.
1516

1617
3.14.1
1718
------

‎cli/src/pcluster/aws/aws_resources.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -255,17 +255,21 @@ def cores_count(self) -> int:
255255

256256
return cores
257257

258+
def network_cards(self) -> list:
259+
"""Return the list of network cards."""
260+
try:
261+
return self.instance_type_data["NetworkInfo"]["NetworkCards"]
262+
except KeyError:
263+
instance_type = self.instance_type_data.get("InstanceType", "unknown")
264+
raise RuntimeError(f"Could not determine network cards for instance type {instance_type}.")
265+
258266
def max_network_cards(self) -> int:
259267
"""Max number of NICs for the instance."""
260-
return len(self.instance_type_data.get("NetworkInfo", {}).get("NetworkCards"))
268+
return len(self.network_cards())
261269

262270
def network_cards_list(self) -> list:
263271
"""List of NICs for the instance."""
264-
return [
265-
NetworkCardInfo(nic)
266-
for nic in self.instance_type_data.get("NetworkInfo", {}).get("NetworkCards")
267-
if nic.get("NetworkCardIndex") is not None
268-
]
272+
return [NetworkCardInfo(nic) for nic in self.network_cards() if nic.get("NetworkCardIndex") is not None]
269273

270274
def default_threads_per_core(self):
271275
"""Return the default threads per core for the given instance type."""

‎cli/src/pcluster/models/cluster.py‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@
6161
from pcluster.models.login_nodes_status import LoginNodesStatus
6262
from pcluster.models.s3_bucket import S3Bucket, S3BucketFactory, S3FileFormat, create_s3_presigned_url
6363
from pcluster.schemas.cluster_schema import ClusterSchema
64+
from pcluster.schemas.common_schema import validate_json_format
6465
from pcluster.templates.cdk_builder import CDKTemplateBuilder
6566
from pcluster.templates.import_cdk import start as start_cdk_import
6667
from pcluster.utils import (
@@ -506,11 +507,11 @@ def _validate_and_parse_config(
506507

507508
@staticmethod
508509
def _load_additional_instance_type_data(cluster_config_dict):
509-
if "DevSettings" in cluster_config_dict:
510-
instance_types_data = cluster_config_dict["DevSettings"].get("InstanceTypesData")
511-
if instance_types_data:
512-
# Set additional instance types data in AWSApi. Schema load will use the information.
513-
AWSApi.instance().ec2.additional_instance_types_data = json.loads(instance_types_data)
510+
instance_types_data = (cluster_config_dict.get("DevSettings") or {}).get("InstanceTypesData")
511+
if instance_types_data:
512+
if not validate_json_format(instance_types_data):
513+
raise ValidationError(message="DevSettings/InstanceTypesData is not a valid JSON.")
514+
AWSApi.instance().ec2.additional_instance_types_data = json.loads(instance_types_data)
514515

515516
def _upload_config(self):
516517
"""Upload source config and save config version."""

‎cli/tests/pcluster/aws/test_aws_resources.py‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
import pytest
1717
from assertpy import assert_that
1818

19-
from pcluster.aws.aws_resources import CapacityReservationInfo
19+
from pcluster.aws.aws_resources import CapacityReservationInfo, InstanceTypeInfo
2020

2121

2222
@pytest.fixture()
@@ -77,3 +77,96 @@ def test_incremental_requested_quantity(self, capacity_reservation_data, expecte
7777
)
7878
def test_reservation_type(self, capacity_reservation_data, expected_value):
7979
assert_that(CapacityReservationInfo(capacity_reservation_data).reservation_type()).is_equal_to(expected_value)
80+
81+
82+
class TestInstanceTypeInfo:
83+
@pytest.mark.parametrize(
84+
("instance_type_data", "expected_network_cards", "expected_error"),
85+
[
86+
# Success cases
87+
({"InstanceType": "aInstanceType", "NetworkInfo": {"NetworkCards": []}}, [], None),
88+
(
89+
{
90+
"InstanceType": "aInstanceType",
91+
"NetworkInfo": {"NetworkCards": [{"aField": "aValue"}, {"anotherField": "anotherValue"}]},
92+
},
93+
[{"aField": "aValue"}, {"anotherField": "anotherValue"}],
94+
None,
95+
),
96+
# Error cases
97+
({}, None, "Could not determine network cards for instance type unknown."),
98+
({"NetworkInfo": {}}, None, "Could not determine network cards for instance type unknown."),
99+
(
100+
{"InstanceType": "aInstanceType"},
101+
None,
102+
"Could not determine network cards for instance type aInstanceType.",
103+
),
104+
(
105+
{"InstanceType": "aInstanceType", "NetworkInfo": {}},
106+
None,
107+
"Could not determine network cards for instance type aInstanceType.",
108+
),
109+
],
110+
)
111+
def test_network_cards(self, instance_type_data, expected_network_cards, expected_error):
112+
if expected_error:
113+
with pytest.raises(RuntimeError, match=expected_error):
114+
InstanceTypeInfo(instance_type_data).network_cards()
115+
else:
116+
assert_that(InstanceTypeInfo(instance_type_data).network_cards()).is_equal_to(expected_network_cards)
117+
118+
@pytest.mark.parametrize(
119+
("instance_type_data", "expected_count", "expected_error"),
120+
[
121+
({"NetworkInfo": {"NetworkCards": []}}, 0, None),
122+
({"NetworkInfo": {"NetworkCards": [{"NetworkCardIndex": 0}, {"NetworkCardIndex": 1}]}}, 2, None),
123+
(
124+
{
125+
"NetworkInfo": {
126+
"NetworkCards": [
127+
{"NetworkCardIndex": 0},
128+
{"NetworkCardIndex": 1},
129+
{"NetworkCardIndex": None},
130+
{"aField": "aValue"},
131+
]
132+
}
133+
},
134+
4,
135+
None,
136+
),
137+
],
138+
)
139+
def test_max_network_cards(self, instance_type_data, expected_count, expected_error):
140+
if expected_error:
141+
with pytest.raises(expected_error):
142+
InstanceTypeInfo(instance_type_data).max_network_cards()
143+
else:
144+
assert_that(InstanceTypeInfo(instance_type_data).max_network_cards()).is_equal_to(expected_count)
145+
146+
@pytest.mark.parametrize(
147+
("instance_type_data", "expected_count", "expected_error"),
148+
[
149+
({"NetworkInfo": {"NetworkCards": []}}, 0, None),
150+
({"NetworkInfo": {"NetworkCards": [{"NetworkCardIndex": 0}, {"NetworkCardIndex": 1}]}}, 2, None),
151+
(
152+
{
153+
"NetworkInfo": {
154+
"NetworkCards": [
155+
{"NetworkCardIndex": 0},
156+
{"NetworkCardIndex": 1},
157+
{"NetworkCardIndex": None},
158+
{"aField": "aValue"},
159+
]
160+
}
161+
},
162+
2,
163+
None,
164+
),
165+
],
166+
)
167+
def test_network_cards_list(self, instance_type_data, expected_count, expected_error):
168+
if expected_error:
169+
with pytest.raises(expected_error):
170+
InstanceTypeInfo(instance_type_data).network_cards_list()
171+
else:
172+
assert_that(InstanceTypeInfo(instance_type_data).network_cards_list()).is_length(expected_count)

‎cli/tests/pcluster/models/test_cluster.py‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
from pcluster.models.cluster_resources import ClusterStack
3030
from pcluster.models.s3_bucket import S3Bucket, S3FileFormat
3131
from pcluster.schemas.cluster_schema import ClusterSchema
32+
from pcluster.schemas.common_schema import ValidationError
3233
from tests.pcluster.aws.dummy_aws_api import mock_aws_api
3334
from tests.pcluster.config.dummy_cluster_config import dummy_slurm_cluster_config
3435
from tests.pcluster.models.dummy_s3_bucket import mock_bucket, mock_bucket_object_utils, mock_bucket_utils
@@ -842,6 +843,33 @@ def test_login_nodes_on_batch(self, mocker, cluster):
842843
lns = cluster.login_nodes_status
843844
assert_that(lns.get_login_nodes_pool_available()).is_false()
844845

846+
@pytest.mark.parametrize(
847+
"cluster_config_dict, expected_error",
848+
[
849+
({}, None),
850+
({"DevSettings": {}}, None),
851+
({"DevSettings": {"InstanceTypesData": '{"valid": "json"}'}}, None),
852+
(
853+
{"DevSettings": {"InstanceTypesData": "invalid json"}},
854+
"DevSettings/InstanceTypesData is not a valid JSON.",
855+
),
856+
],
857+
)
858+
def test_load_additional_instance_type_data(self, mocker, cluster_config_dict, expected_error):
859+
mock_aws_api(mocker)
860+
mock_api_instance = mocker.patch("pcluster.models.cluster.AWSApi.instance")
861+
862+
if expected_error:
863+
with pytest.raises(ValidationError, match=expected_error):
864+
Cluster._load_additional_instance_type_data(cluster_config_dict)
865+
else:
866+
Cluster._load_additional_instance_type_data(cluster_config_dict)
867+
868+
if cluster_config_dict.get("DevSettings", {}).get("InstanceTypesData"):
869+
# Verify that additional_instance_types_data was set when valid JSON provided
870+
expected_data = json.loads(cluster_config_dict["DevSettings"]["InstanceTypesData"])
871+
mock_api_instance.return_value.ec2.additional_instance_types_data = expected_data
872+
845873

846874
OLD_CONFIGURATION = """
847875
Image:

0 commit comments

Comments
 (0)