Add EFS access point support (aws-parallelcluster#2337) #6359
Conversation
Signed-off-by: Thomas Heinen <[email protected]>
| @@ -1109,6 +1109,7 @@ def _add_efs_storage(self, id: str, shared_efs: SharedEfs): | |||
| shared_efs.encryption_in_transit | |||
| ) | |||
| self.shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"].append(shared_efs.iam_authorization) | |||
| self.shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"].append(shared_efs.accesspoint_id) | |||
There was a problem hiding this comment.
Here and in other files, we are assuming that more than one access point can be specified for a given file system (AccesspointsIds rather than AccesspointId).
It's true that a file system can support multiple access points (https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#efs-access-poiont-create), but the mount action is always done using a single access point (https://docs.aws.amazon.com/efs/latest/ug/mounting-access-points.html).
So please, let's assume to specify only one access point on the cluster config.
There was a problem hiding this comment.
I think this is actually a per filesystem array, similar to the encryption in transit array
There was a problem hiding this comment.
Yes, it's actually an array of all the separate FS in config. I just aligned naming and structure to the other properties which are also named in plural.
| @@ -18,7 +18,7 @@ class EfsMountOptionsValidator(Validator): | |||
| IAM Authorization requires Encryption in Transit. | |||
| """ | |||
|
|
|||
| def _validate(self, encryption_in_transit: bool, iam_authorization: bool, name: str): | |||
| def _validate(self, encryption_in_transit: bool, iam_authorization: bool, accesspoint_id: str, name: str): | |||
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/validators/test_efs_validators.py
| @@ -335,6 +335,10 @@ def _add_compute_resource_launch_template( | |||
| self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], | |||
| use_lower_case=True, | |||
| ), | |||
| "efs_accesspoint_ids": to_comma_separated_string( | |||
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/templates/test_queues_stack.py#L114
| @@ -235,6 +235,10 @@ def _add_login_nodes_pool_launch_template(self): | |||
| self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], | |||
| use_lower_case=True, | |||
| ), | |||
| "efs_accesspoint_ids": to_comma_separated_string( | |||
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/templates/test_login_nodes_stack.py#L23
| @@ -1288,6 +1289,10 @@ def _add_head_node(self): | |||
| "efs_iam_authorizations": to_comma_separated_string( | |||
| self.shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], use_lower_case=True | |||
| ), | |||
| "efs_accesspoint_ids": to_comma_separated_string( | |||
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/templates/test_cluster_stack.py#L901
| @@ -18,7 +18,7 @@ class EfsMountOptionsValidator(Validator): | |||
| IAM Authorization requires Encryption in Transit. | |||
| """ | |||
|
|
|||
| def _validate(self, encryption_in_transit: bool, iam_authorization: bool, name: str): | |||
| def _validate(self, encryption_in_transit: bool, iam_authorization: bool, accesspoint_id: str, name: str): | |||
There was a problem hiding this comment.
The validator should verify that the provided access point exists
|
Hi @thheinen, Made a first round of review and unblocked PR checks. |
Signed-off-by: Thomas Heinen <[email protected]>
Description of changes
Add support to specify EFS access points per issue #2337
Tests
Tested mounting EFS with and without accesspoints from an aws-parallelcluster cluster.
More tests to be done but opening a draft PR for discussion and early feedback
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.