Skip to content
Open
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
37 changes: 30 additions & 7 deletions cli/src/pcluster/config/imagebuilder_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# These objects are obtained from the configuration file through a conversion based on the Schema classes.
#

import json
from typing import List

from pcluster.aws.common import get_region
Expand All @@ -27,6 +28,7 @@
)
from pcluster.imagebuilder_utils import ROOT_VOLUME_TYPE
from pcluster.validators.common import ValidatorContext
from pcluster.validators.dev_settings_validators import CliAttributeOverridesValidator
from pcluster.validators.ebs_validators import EbsVolumeTypeSizeValidator
from pcluster.validators.ec2_validators import InstanceTypeBaseAMICompatibleValidator
from pcluster.validators.iam_validators import IamPolicyValidator, InstanceProfileValidator, RoleValidator
Expand Down Expand Up @@ -240,8 +242,7 @@ def __init__(
distribution_configuration: DistributionConfiguration = None,
terminate_instance_on_failure: bool = None,
disable_validate_and_test: bool = None,
cinc_installer_url: str = None,
cinc_version: str = None,
cli_attribute_overrides: str = None,
disable_kernel_update: bool = None,
slurm_patches_s3_archive: str = None,
**kwargs
Expand All @@ -251,14 +252,36 @@ def __init__(
self.distribution_configuration = distribution_configuration
self.terminate_instance_on_failure = Resource.init_param(terminate_instance_on_failure, default=True)
self.disable_validate_and_test = Resource.init_param(disable_validate_and_test, default=True)
self.cinc_installer_url = Resource.init_param(cinc_installer_url, default="")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removing this parameter from DevSettings schema is ok because we do not guarantee backward compatibility for DevSettings by definition. However, I suggest to document the removal in the changelog. the fact that we do not guarantee backward compatibility means it is ok to make this change, but does not mean we should not surface it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IF we never surface the addition of DevSettings in chnagelog; why should we surface their removal?

# Optional override for the CINC client version baked into the AMI
# (CfnParamCincVersion -> ChefVersion in parallelcluster.yaml). When
# left empty the YAML-rendered default ChefVersion is used.
self.cinc_version = Resource.init_param(cinc_version, default="")
# JSON string of CLI-side build-image overrides (e.g. cinc_version,
# cinc_installer_url), unfurled by the accessors below. Mirrors
# Cookbook.ExtraChefAttributes: freeform JSON, well-formedness only.
self.cli_attribute_overrides = Resource.init_param(cli_attribute_overrides, default="")
self.disable_kernel_update = Resource.init_param(disable_kernel_update, default=False)
self.slurm_patches_s3_archive = Resource.init_param(slurm_patches_s3_archive, default="")

def _cli_attribute_override(self, key):
"""Unfurl a single value from the cli_attribute_overrides JSON blob."""
if not self.cli_attribute_overrides:
return ""
return json.loads(self.cli_attribute_overrides).get(key, "")

@property
def cinc_installer_url(self):
"""CINC installer URL override (from cli_attribute_overrides)."""
return self._cli_attribute_override("cinc_installer_url")

@property
def cinc_version(self):
"""CINC client version override (from cli_attribute_overrides)."""
return self._cli_attribute_override("cinc_version")

def _register_validators(self, context: ValidatorContext = None):
super()._register_validators(context)
if self.cli_attribute_overrides:
self._register_validator(
CliAttributeOverridesValidator, cli_attribute_overrides=self.cli_attribute_overrides
)


class ImagebuilderDeploymentSettings(BaseDeploymentSettings):
"""Represent the deployment settings for the ImageBuilder."""
Expand Down
3 changes: 1 addition & 2 deletions cli/src/pcluster/schemas/imagebuilder_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,7 @@ class ImagebuilderDevSettingsSchema(BaseDevSettingsSchema):
distribution_configuration = fields.Nested(DistributionConfigurationSchema)
terminate_instance_on_failure = fields.Bool()
disable_validate_and_test = fields.Bool()
cinc_installer_url = fields.Str()
cinc_version = fields.Str()
cli_attribute_overrides = fields.Str()
disable_kernel_update = fields.Bool()

@post_load
Expand Down
27 changes: 27 additions & 0 deletions cli/src/pcluster/validators/dev_settings_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,33 @@
ATTR_CLUSTER_READINESS_CHECK_IGNORE_FAILURE = "cluster.cluster_readiness_check_ignore_failure"
MIN_SLURM_RECONFIGURE_TIMEOUT = 300

CLI_ATTRIBUTE_OVERRIDES_PATH = "DevSettings/CliAttributeOverrides"
# Keys the CLI currently unfurls from the blob. Kept for an optional future
# strict check; today validation is well-formedness only (like ExtraChefAttributes).
KNOWN_CLI_ATTRIBUTE_OVERRIDES = {"cinc_version", "cinc_installer_url"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess we can remove this constant because we are only interested in enforcing the json formatting of the CliAttributeOverrides without making eny assumption or enforcement about its field.

In Cookbook Chef Attribute we introduced the validation of specific attributes only because we decided to vend those attributes publicly to mitigate an impact, but in general we should not do it and keep it free form.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will do



class CliAttributeOverridesValidator(Validator):
"""Validate DevSettings/CliAttributeOverrides is a well-formed JSON object."""

def _validate(self, cli_attribute_overrides: str = None):
if not cli_attribute_overrides:
return
try:
attrs = json.loads(cli_attribute_overrides)
except ValueError:
self._add_failure(
f"Invalid value in {CLI_ATTRIBUTE_OVERRIDES_PATH}: must be a valid JSON string.",
FailureLevel.ERROR,
)
return
if not isinstance(attrs, dict):
self._add_failure(
f"Invalid value in {CLI_ATTRIBUTE_OVERRIDES_PATH}: must be a JSON object.",
FailureLevel.ERROR,
)
# TODO: optionally validate keys against KNOWN_CLI_ATTRIBUTE_OVERRIDES here.

@himani2411 himani2411 Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NOTE FOR REVIEWER: Keeping this as a future improvement since we do not validate all JSON content for ExtraChefAttributes as of today and the cinc attributes that I am replacing also do no have any validators. Any un-related attribute apart from the cinc* one's in the JSON will be a no-op.



class ExtraChefAttributesValidator(Validator):
"""Validate DevSettings/Cookbook/ExtraChefAttributes."""
Expand Down
43 changes: 43 additions & 0 deletions cli/tests/pcluster/config/test_imagebuilder_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Copyright 2026 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 pytest
from assertpy import assert_that

from pcluster.config.imagebuilder_config import ImagebuilderDevSettings


@pytest.mark.parametrize(
"cli_attribute_overrides, expected_cinc_version, expected_cinc_installer_url",
[
pytest.param(None, "", "", id="unset blob -> empty overrides"),
pytest.param("", "", "", id="empty blob -> empty overrides"),
pytest.param(
'{"cinc_version": "18.8.54", "cinc_installer_url": "https://omnitruck.cinc.sh/install.sh"}',
"18.8.54",
"https://omnitruck.cinc.sh/install.sh",
id="both keys unfurled",
),
pytest.param('{"cinc_version": "18.8.54"}', "18.8.54", "", id="only cinc_version present"),
pytest.param(
'{"cinc_installer_url": "https://omnitruck.cinc.sh/install.sh"}',
"",
"https://omnitruck.cinc.sh/install.sh",
id="only cinc_installer_url present",
),
pytest.param('{"unrelated": "value"}', "", "", id="unknown keys ignored"),
],
)
def test_dev_settings_cli_attribute_overrides_unfurl(
cli_attribute_overrides, expected_cinc_version, expected_cinc_installer_url
):
dev_settings = ImagebuilderDevSettings(cli_attribute_overrides=cli_attribute_overrides)
assert_that(dev_settings.cinc_version).is_equal_to(expected_cinc_version)
assert_that(dev_settings.cinc_installer_url).is_equal_to(expected_cinc_installer_url)
3 changes: 3 additions & 0 deletions cli/tests/pcluster/validators/test_all_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ def test_slurm_all_validators_are_called(test_datadir, mocker):
if m["name"] in ["TagKeyValidator", "ClusterNameValidator", "InstanceProfileValidator", "RoleValidator"]:
# ToDo: Reserved tag keys to be aligned between cluster and image builder
continue
if m["name"] in ["CliAttributeOverridesValidator"]:
# Image-builder-only validator (DevSettings/CliAttributeOverrides); not run on cluster config.
continue
print(f"Checking validator of class \"{m['name']}\" is called")
m["mocker"].assert_called()

Expand Down
44 changes: 43 additions & 1 deletion cli/tests/pcluster/validators/test_dev_settings_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
import pytest

from pcluster.validators.common import FailureLevel
from pcluster.validators.dev_settings_validators import ExtraChefAttributesValidator
from pcluster.validators.dev_settings_validators import (
CliAttributeOverridesValidator,
ExtraChefAttributesValidator,
)
from tests.pcluster.validators.utils import assert_failure_level, assert_failure_messages


Expand Down Expand Up @@ -219,3 +222,42 @@ def test_extra_chef_attributes_validator_cluster_readiness_check_ignore_failure(
assert_failure_messages(actual_failures, expected_message)
if expected_failure_level:
assert_failure_level(actual_failures, expected_failure_level)


@pytest.mark.parametrize(
"cli_attribute_overrides, expected_message, expected_failure_level",
[
pytest.param("", None, None, id="empty string is valid"),
pytest.param(None, None, None, id="None is valid"),
pytest.param(
'{"cinc_version": "18.8.54", "cinc_installer_url": "https://omnitruck.cinc.sh/install.sh"}',
None,
None,
id="valid JSON object",
),
pytest.param("{}", None, None, id="empty JSON object is valid"),
pytest.param(
"{not valid json",
"Invalid value in DevSettings/CliAttributeOverrides: must be a valid JSON string.",
FailureLevel.ERROR,
id="malformed JSON throws error",
),
pytest.param(
'["cinc_version"]',
"Invalid value in DevSettings/CliAttributeOverrides: must be a JSON object.",
FailureLevel.ERROR,
id="JSON array (not object) throws error",
),
pytest.param(
'"just a string"',
"Invalid value in DevSettings/CliAttributeOverrides: must be a JSON object.",
FailureLevel.ERROR,
id="JSON scalar (not object) throws error",
),
],
)
def test_cli_attribute_overrides_validator(cli_attribute_overrides, expected_message, expected_failure_level):
actual_failures = CliAttributeOverridesValidator().execute(cli_attribute_overrides=cli_attribute_overrides)
assert_failure_messages(actual_failures, expected_message)
if expected_failure_level:
assert_failure_level(actual_failures, expected_failure_level)
Loading