-
Notifications
You must be signed in to change notification settings - Fork 315
[Efs encryption][Draft] Support HeadNode Efs SharedStorage Encryption #6914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
f36a98e
230d6a8
ac5f9fb
df0da09
a942241
a4ae51e
1931dc8
63bfa91
cbb5415
be5ee63
91047d8
ea3b32a
bb4e1e4
88b3781
7508cfc
944762a
b7936e4
90b2691
3fc1d57
5ba7bbd
c27abc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -53,10 +53,9 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SharedEbs, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SharedEfs, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SharedFsxLustre, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SharedStorageType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SlurmClusterConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from pcluster.config.common import DefaultUserHomeType | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from pcluster.config.common import DefaultUserHomeType, SharedStorageType | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from pcluster.constants import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ALL_PORTS_RANGE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
CW_ALARM_DATAPOINTS_TO_ALARM_DEFAULT, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -264,12 +263,9 @@ def _add_resources(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Add the internal use shared storage to the stack | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# This FS will be mounted, the shared dirs will be added, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# then it will be unmounted and the shared dirs will be | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# mounted. We need to create the additional mount points first. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# mounted. We need to create the additional mount points first. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.config.head_node.shared_storage_type.lower() == SharedStorageType.EFS.value: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
internal_efs_storage_shared = SharedEfs( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mount_dir="/opt/parallelcluster/init_shared", name="internal_pcluster_shared", throughput_mode="elastic" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self._add_shared_storage(internal_efs_storage_shared) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self._add_internal_efs_shared_storage() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Add user configured shared storage | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.config.shared_storage: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -335,6 +331,19 @@ def _add_resources(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
head_node_alarms=self.head_node_alarms, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _add_internal_efs_shared_storage(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.config.head_node.shared_storage_efs_settings: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
encrypted = self.config.head_node.shared_storage_efs_settings.encrypted | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
encrypted = False | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Single Responsibility / Maintainability] The resource |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
internal_efs_storage_shared = SharedEfs( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mount_dir="/opt/parallelcluster/init_shared", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
name="internal_pcluster_shared", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throughput_mode="elastic", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
encrypted=encrypted, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Test] We should capture this property in a unit test. the unit test should verify that when encrypted is specified in the cluster config, then the cluster template has the EFS resource with that property set to the expected value. See example: aws-parallelcluster/cli/tests/pcluster/templates/test_cluster_stack.py Lines 217 to 249 in bf22eba
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self._add_shared_storage(internal_efs_storage_shared) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _cw_metric_head_node( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self, namespace, metric_name, statistic="Maximum", period_seconds=CW_ALARM_PERIOD_DEFAULT, extra_dimensions=None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ | |
from pcluster.aws.aws_resources import InstanceTypeInfo | ||
from pcluster.aws.common import AWSClientError | ||
from pcluster.cli.commands.dcv_util import get_supported_dcv_os | ||
from pcluster.config.common import CapacityType | ||
from pcluster.config.common import CapacityType, SharedStorageType | ||
from pcluster.constants import ( | ||
CIDR_ALL_IPS, | ||
DELETE_POLICY, | ||
|
@@ -676,7 +676,7 @@ def _check_file_storage(self, security_groups_by_nodes, file_storages, subnet_id | |
self._add_failure( | ||
f"The current security group settings on file storage '{file_storage_id}' does not" | ||
" satisfy mounting requirement. The file storage must be associated to a security group" | ||
f" that allows {direction } {protocol.upper()} traffic through ports {ports}. " | ||
f" that allows {direction} {protocol.upper()} traffic through ports {ports}. " | ||
f"Missing ports: {missing_ports}", | ||
FailureLevel.ERROR, | ||
) | ||
|
@@ -1336,6 +1336,24 @@ def _validate(self, head_node_instance_type: str, total_max_compute_nodes: int): | |
) | ||
|
||
|
||
class SharedStorageEfsSettingsValidator(Validator): | ||
""" | ||
HeadNode SharedStorageEfsSettings Validator. | ||
|
||
Verify HeadNode SharedStorageEfsSettings can only be used with Efs SharedStorageType. | ||
""" | ||
|
||
def _validate(self, shared_storage_type: str, shared_storage_efs_settings): | ||
if shared_storage_efs_settings and shared_storage_type.lower() != SharedStorageType.EFS.value: | ||
self._add_failure( | ||
"SharedStorageEfsSettings is specified " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message can be simplified to I suggest to use the capitalize because, even if we are able to handle both |
||
f"but the SharedStorageType is set to {shared_storage_type}. " | ||
"SharedStorageEfsSettings can only be used when SharedStorageType " | ||
f"is specified as {SharedStorageType.EFS.value}.", | ||
FailureLevel.ERROR, | ||
) | ||
|
||
|
||
class SharedEbsPerformanceBottleNeckValidator(Validator): | ||
"""Warn potential performance bottleneck of using Shared EBS.""" | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -198,6 +198,9 @@ def test_slurm_validators_are_called_with_correct_argument(test_datadir, mocker) | |
number_of_storage_validator = mocker.patch( | ||
cluster_validators + ".NumberOfStorageValidator._validate", return_value=[] | ||
) | ||
shared_storage_efs_settings_validator = mocker.patch( | ||
cluster_validators + ".SharedStorageEfsSettingsValidator._validate", return_value=[] | ||
) | ||
deletion_policy_validator = mocker.patch(cluster_validators + ".DeletionPolicyValidator._validate", return_value=[]) | ||
root_volume_encryption_consistency_validator = mocker.patch( | ||
cluster_validators + ".RootVolumeEncryptionConsistencyValidator._validate", return_value=[] | ||
|
@@ -392,6 +395,10 @@ def test_slurm_validators_are_called_with_correct_argument(test_datadir, mocker) | |
], | ||
any_order=True, | ||
) | ||
shared_storage_efs_settings_validator.assert_has_calls( | ||
[call(shared_storage_type="Ebs", shared_storage_efs_settings=None)], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Test] This unit test would make mnore sense if we use a configuration that inject both shared_storage_type and shared_storage_efs_settings into the validators, otherwise you could have the doubt that there exists cases where shared_storage_efs_settings is not injected. My suggestion is to cover two scenarios:
|
||
any_order=True, | ||
) | ||
# capacity reservation validators | ||
capacity_reservation_validator.assert_has_calls( | ||
[ | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -31,7 +31,7 @@ | |||||||||||||||||||
SlurmSettings, | ||||||||||||||||||||
Tag, | ||||||||||||||||||||
) | ||||||||||||||||||||
from pcluster.config.common import CapacityType | ||||||||||||||||||||
from pcluster.config.common import CapacityType, SharedStorageType | ||||||||||||||||||||
from pcluster.constants import PCLUSTER_NAME_MAX_LENGTH, PCLUSTER_NAME_MAX_LENGTH_SLURM_ACCOUNTING | ||||||||||||||||||||
from pcluster.validators.cluster_validators import ( | ||||||||||||||||||||
FSX_MESSAGES, | ||||||||||||||||||||
|
@@ -71,6 +71,7 @@ | |||||||||||||||||||
SchedulerDisableSudoAccessForDefaultUserValidator, | ||||||||||||||||||||
SchedulerOsValidator, | ||||||||||||||||||||
SharedFileCacheNotHomeValidator, | ||||||||||||||||||||
SharedStorageEfsSettingsValidator, | ||||||||||||||||||||
SharedStorageMountDirValidator, | ||||||||||||||||||||
SharedStorageNameValidator, | ||||||||||||||||||||
UnmanagedFsxMultiAzValidator, | ||||||||||||||||||||
|
@@ -2075,6 +2076,50 @@ def test_mixed_security_group_overwrite_validator(head_node_security_groups, que | |||||||||||||||||||
assert_failure_messages(actual_failures, expected_message) | ||||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
@pytest.mark.parametrize( | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Test] It's a best practice to define test cases with a self-explanatory id. This improves readability and troubleshooting. aws-parallelcluster/cli/tests/pcluster/api/controllers/test_cluster_operations_controller.py Lines 138 to 146 in 4f4c670
|
||||||||||||||||||||
"shared_storage_type, shared_storage_efs_settings, expected_message", | ||||||||||||||||||||
[ | ||||||||||||||||||||
( | ||||||||||||||||||||
"Efs", | ||||||||||||||||||||
{"encrypted": True}, | ||||||||||||||||||||
None, | ||||||||||||||||||||
), | ||||||||||||||||||||
( | ||||||||||||||||||||
"Efs", | ||||||||||||||||||||
{"encrypted": False}, | ||||||||||||||||||||
None, | ||||||||||||||||||||
), | ||||||||||||||||||||
( | ||||||||||||||||||||
"Efs", | ||||||||||||||||||||
None, | ||||||||||||||||||||
None, | ||||||||||||||||||||
), | ||||||||||||||||||||
( | ||||||||||||||||||||
"Ebs", | ||||||||||||||||||||
{"encrypted": True}, | ||||||||||||||||||||
f"SharedStorageEfsSettings is specified but the SharedStorageType is set to Ebs. " | ||||||||||||||||||||
"SharedStorageEfsSettings can only be used when SharedStorageType " | ||||||||||||||||||||
f"is specified as {SharedStorageType.EFS.value}.", | ||||||||||||||||||||
), | ||||||||||||||||||||
( | ||||||||||||||||||||
"Ebs", | ||||||||||||||||||||
{"encrypted": False}, | ||||||||||||||||||||
f"SharedStorageEfsSettings is specified but the SharedStorageType is set to Ebs. " | ||||||||||||||||||||
"SharedStorageEfsSettings can only be used when SharedStorageType " | ||||||||||||||||||||
f"is specified as {SharedStorageType.EFS.value}.", | ||||||||||||||||||||
), | ||||||||||||||||||||
( | ||||||||||||||||||||
"Ebs", | ||||||||||||||||||||
None, | ||||||||||||||||||||
None, | ||||||||||||||||||||
), | ||||||||||||||||||||
], | ||||||||||||||||||||
) | ||||||||||||||||||||
def test_shared_storage_efs_settings_validator(shared_storage_type, shared_storage_efs_settings, expected_message): | ||||||||||||||||||||
actual_failures = SharedStorageEfsSettingsValidator().execute(shared_storage_type, shared_storage_efs_settings) | ||||||||||||||||||||
assert_failure_messages(actual_failures, expected_message) | ||||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
@pytest.mark.parametrize( | ||||||||||||||||||||
"root_volume_size, ami_size, expected_message", | ||||||||||||||||||||
[ | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in other part of the PR.
This change is motivated by a refactoring, unrelated to the main goal of this PR.
A best practice is to keep the PR minimal and avoid refactoring on components that are out of scope for the main goal. The refactoring you're suggesting here is correct, but I suggest to do it in a follow up PR.