-
Notifications
You must be signed in to change notification settings - Fork 555
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
[Core][AWS] Allow specification of Security Groups for resources. #3501
[Core][AWS] Allow specification of Security Groups for resources. #3501
Conversation
ae9633d
to
e644f58
Compare
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.
Feedback for resolving the following would be appreciated:
- ports / security_group_name for serve
- cloud resources only receive
cluster_name_on_cloud
formake_deploy_variables
instead ofcluster_name
sky/utils/schemas.py
Outdated
@@ -567,7 +569,7 @@ def get_config_schema(): | |||
# Validation may fail if $schema is included. | |||
if k != '$schema' | |||
} | |||
resources_schema['properties'].pop('ports') |
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.
I'm not sure how best to resolve this. For Serve, ports are automatically specified for the load balancer which conflicts with being able to set SGs for it.
#3473
@@ -102,11 +102,27 @@ Available fields and semantics: | |||
|
|||
# Security group (optional). | |||
# | |||
# The name of the security group to use for all instances. If not specified, |
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.
Updated to match the format of remote identity.
sky/clouds/aws.py
Outdated
if user_security_group is not None and not isinstance( | ||
user_security_group, str): | ||
for profile in user_security_group: | ||
if fnmatch.fnmatchcase(cluster_name_on_cloud, |
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.
This function does not currently have access to cluster_name
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.
This is a good catch! A proposal would be we refactor the make_deploy_resources_variable
to take ClusterName
dataclass in all cloud classes, instead of a single string for the cluster name:
skypilot/sky/provision/provisioner.py
Lines 41 to 50 in 05aafb8
@dataclasses.dataclass | |
class ClusterName: | |
display_name: str | |
name_on_cloud: str | |
def __repr__(self) -> str: | |
return repr(self.display_name) | |
def __str__(self) -> str: | |
return self.display_name |
To do so, we can move the ClusterName
class out of provision
module to utils.resources_utils
, and let the cloud class and the functions under provision
module to use ClusterName
object as input.
@Michaelvll @concretevitamin do you have any suggestions for the concerns I raised? Thanks! |
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.
Thanks for adding the support for specifying security group names for different cluster @JGSweets! This is awesome. I left some comments, mainly concerning that specifying security group name will cause open ports not reliable if the user does not have the security group set up correctly. For now, a good warning or hint message can be enough.
sky/clouds/aws.py
Outdated
if user_security_group is not None and not isinstance( | ||
user_security_group, str): | ||
for profile in user_security_group: | ||
if fnmatch.fnmatchcase(cluster_name_on_cloud, |
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.
This is a good catch! A proposal would be we refactor the make_deploy_resources_variable
to take ClusterName
dataclass in all cloud classes, instead of a single string for the cluster name:
skypilot/sky/provision/provisioner.py
Lines 41 to 50 in 05aafb8
@dataclasses.dataclass | |
class ClusterName: | |
display_name: str | |
name_on_cloud: str | |
def __repr__(self) -> str: | |
return repr(self.display_name) | |
def __str__(self) -> str: | |
return self.display_name |
To do so, we can move the ClusterName
class out of provision
module to utils.resources_utils
, and let the cloud class and the functions under provision
module to use ClusterName
object as input.
sky/utils/schemas.py
Outdated
@@ -567,7 +569,7 @@ def get_config_schema(): | |||
# Validation may fail if $schema is included. | |||
if k != '$schema' | |||
} | |||
resources_schema['properties'].pop('ports') | |||
resources_schema['properties'].pop('port', None) |
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.
ports
should be the correct field?
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.
Yes, ports is correct as it pops this from
_get_single_resources_schema`
Line 659 in 8a0a34d
'ports': { |
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.
Oh, I mean, it seems we changed it to port
now, which seems not a valid property to pop? It seems the issue is fixed in the latest commit : )
sky/utils/schemas.py
Outdated
{ | ||
# A list of single-element dict to pretain the | ||
# order. | ||
# Example: | ||
# security_group_name: | ||
# - my-cluster1-*: my-security-group-1 | ||
# - my-cluster2-*: my-security-group-2 | ||
# - "*"": my-security-group-3 | ||
'type': 'array', | ||
'items': { | ||
'type': 'object', | ||
'additionalProperties': { | ||
'type': 'string' | ||
}, | ||
'maxProperties': 1, | ||
'minProperties': 1, | ||
}, | ||
} |
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.
minor nit: Since this pattern has been used by the remote_identity
and security_group_name
, we can probably have a constant or a function to generate this dict.
4be366f
to
9c71b97
Compare
@@ -154,7 +154,11 @@ | |||
# we need to take this field from the new yaml. | |||
('provider', 'tpu_node'), | |||
('provider', 'security_group', 'GroupName'), | |||
('available_node_types', 'ray.head.default', 'node_config', |
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.
Fixes an error with IAM roles if the cluster previously existed.
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.
Does this mean we want to always update the IamInstanceProfile
for an old cluster when a user update the ~/.sky/config.yaml
? Does this work with launching an existing cluster with a different remote_identity
? It would be nice to include some tests in the PR description : )
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.
I guess my thoughts are if you have a specified cluster name and you change the IAM or SG, I would expect the cluster to update. However, maybe that isn't the desired behavior? Otherwise, one would have to tear down the entire cluster to change it.
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.
@Michaelvll I did test launching with different names to ensure it changes with the name in serve between controller and worker. In this case the controller and the worker are getting different cluster names.
Workers being replicas.
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.
The behavior sounds fine to me, except that it may cause a risk that the controller may get a different IamInstanceProfile that no longer has permission to access the previous service replicas or managed job clusters.
sky/resources.py
Outdated
if security_group_name is not None: | ||
with ux_utils.print_exception_no_traceback(): | ||
logger.warning( | ||
f'Ports {self.ports} and security group name are ' | ||
f'specified: {security_group_name}. It is not ' | ||
'guaranteed that the ports will be opened if the ' | ||
'specified security group is not correctly set up. ' | ||
'Please try to specify `ports` only and leave out ' | ||
'`aws.security_group_name` in `~/.sky/config.yaml` to ' | ||
'allow SkyPilot to automatically create and configure ' | ||
'the security group.') |
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.
I'm not sure this is the ideal place as it seems to print the warning 10+
times per launch.
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.
This is a good catch! Let's remove the warning here and move it to the aws.make_deploy_resources_variables
so that the warning will only shown when launching an exact launchable resource. (check out the comment in aws.py above)
@Michaelvll @cblmemo I've updated the PR with the Do we want to further refactor |
9c71b97
to
0b2d5b2
Compare
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.
Thanks for the update @JGSweets and sorry for the delay! This PR and the ClusterName
refactoring looks mostly good to me. Left several additional comments. I don't think we need further refactoring for provisioners at the moment.
sky/utils/schemas.py
Outdated
@@ -567,7 +569,7 @@ def get_config_schema(): | |||
# Validation may fail if $schema is included. | |||
if k != '$schema' | |||
} | |||
resources_schema['properties'].pop('ports') | |||
resources_schema['properties'].pop('port', None) |
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.
Oh, I mean, it seems we changed it to port
now, which seems not a valid property to pop? It seems the issue is fixed in the latest commit : )
@@ -154,7 +154,11 @@ | |||
# we need to take this field from the new yaml. | |||
('provider', 'tpu_node'), | |||
('provider', 'security_group', 'GroupName'), | |||
('available_node_types', 'ray.head.default', 'node_config', |
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.
Does this mean we want to always update the IamInstanceProfile
for an old cluster when a user update the ~/.sky/config.yaml
? Does this work with launching an existing cluster with a different remote_identity
? It would be nice to include some tests in the PR description : )
sky/clouds/aws.py
Outdated
if user_security_group is None and resources.ports is not None: | ||
# Already checked in Resources._try_validate_ports | ||
assert user_security_group is None | ||
security_group = USER_PORTS_SECURITY_GROUP_NAME.format( | ||
cluster_name_on_cloud) | ||
elif user_security_group is not None: | ||
assert resources.ports is None | ||
security_group = user_security_group | ||
else: | ||
cluster_name.name_on_cloud) | ||
elif user_security_group is None: | ||
security_group = DEFAULT_SECURITY_GROUP_NAME |
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.
Let's move the warning from aws.py to here so that it will only be printed once whenever it is trying to launch an AWS cluster with the security group specified.
security_group = user_security_group
if security_group is None:
if resources.ports is not None:
# Already checked in Resources._try_validate_ports
security_group = USER_PORTS_SECURITY_GROUP_NAME.format(
cluster_name.name_on_cloud)
elif user_security_group is None:
security_group = DEFAULT_SECURITY_GROUP_NAME
elif resources.ports is not None:
# resources.ports will only take effect when the security group is not specified
# by user.
logger.warning(
f'{colorama.Fore.YELLOW}AWS security group name is specified '
f'({security_group}) with aws.security_group_name in ~/.sky/config.yaml, '
f'while ports ({resources.ports}) is also specified in task resources. The '
'ports are not guaranteed to be opened in the specified security group, '
'due to the complex rules setup. Please manually make sure it.'
f'{colorama.Style.RESET_ALL}')
Unfortunately, since we call make_deploy_resources_variables
in the following place, the logging will still twice for each failover. The resources_vars
retrieved is just for getting the custom_resources
later, so we can add a custom_resources
field in the provision_record
returned by the bulk_provision
.
skypilot/sky/backends/cloud_vm_ray_backend.py
Lines 1557 to 1562 in 0b2d5b2
resources_vars = ( | |
to_provision.cloud.make_deploy_resources_variables( | |
to_provision, | |
resources_utils.ClusterName( | |
cluster_name, handle.cluster_name_on_cloud), | |
region, zones)) |
skypilot/sky/backends/cloud_vm_ray_backend.py
Lines 1544 to 1553 in 0b2d5b2
provision_record = provisioner.bulk_provision( | |
to_provision.cloud, | |
region, | |
zones, | |
resources_utils.ClusterName( | |
cluster_name, handle.cluster_name_on_cloud), | |
num_nodes=num_nodes, | |
cluster_yaml=handle.cluster_yaml, | |
prev_cluster_ever_up=prev_cluster_ever_up, | |
log_dir=self.log_dir) |
I am ok to leave the change for bulk_provision
and provision_record
to a future PR if we think the current PR involves too many things.
sky/resources.py
Outdated
raise ValueError( | ||
'Cannot specify ports when AWS security group name is ' | ||
'specified.') | ||
if self.cloud is None or isinstance(self.cloud, clouds.AWS): |
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.
This can be simplified to:
if self.cloud is None or isinstance(self.cloud, clouds.AWS): | |
if isinstance(self.cloud, clouds.AWS): |
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.
this is removed anyways since the warning is moved.
sky/resources.py
Outdated
with ux_utils.print_exception_no_traceback(): | ||
logger.warning( | ||
f'Ports {self.ports} and security group name are ' | ||
f'specified: {security_group_name}. It is not ' | ||
'guaranteed that the ports will be opened if the ' | ||
'specified security group is not correctly set up. ' | ||
'Please try to specify `ports` only and leave out ' | ||
'`aws.security_group_name` in `~/.sky/config.yaml` to ' | ||
'allow SkyPilot to automatically create and configure ' | ||
'the security group.') |
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.
with ux_utils.print_exception_no_traceback(): | |
logger.warning( | |
f'Ports {self.ports} and security group name are ' | |
f'specified: {security_group_name}. It is not ' | |
'guaranteed that the ports will be opened if the ' | |
'specified security group is not correctly set up. ' | |
'Please try to specify `ports` only and leave out ' | |
'`aws.security_group_name` in `~/.sky/config.yaml` to ' | |
'allow SkyPilot to automatically create and configure ' | |
'the security group.') | |
logger.warning( | |
f'Ports {self.ports} and security group name are ' | |
f'specified: {security_group_name}. It is not ' | |
'guaranteed that the ports will be opened if the ' | |
'specified security group is not correctly set up. ' | |
'Please try to specify `ports` only and leave out ' | |
'`aws.security_group_name` in `~/.sky/config.yaml` to ' | |
'allow SkyPilot to automatically create and configure ' | |
'the security group.') |
sky/utils/schemas.py
Outdated
def _get_cloud_name_property_mapping(field: str): | ||
return { | ||
field: { | ||
'oneOf': [ | ||
{ | ||
'type': 'string' | ||
}, | ||
{ | ||
# A list of single-element dict to pretain the | ||
# order. | ||
# Example: | ||
# property_name: | ||
# - my-cluster1-*: my-property-1 | ||
# - my-cluster2-*: my-property-2 | ||
# - "*"": my-property-3 | ||
'type': 'array', | ||
'items': { | ||
'type': 'object', | ||
'additionalProperties': { | ||
'type': 'string' | ||
}, | ||
'maxProperties': 1, | ||
'minProperties': 1, | ||
}, | ||
} | ||
] | ||
} | ||
} |
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.
nit: we can use a constant instead and let the callers use the constant.
_PRORPERTY_NAME_OR_CLUSTER_NAME_TO_PROPERTY = {
'oneOf': [
{
'type': 'string'
},
{
# A list of single-element dict to pretain the
# order.
# Example:
# property_name:
# - my-cluster1-*: my-property-1
# - my-cluster2-*: my-property-2
# - "*"": my-property-3
'type': 'array',
'items': {
'type': 'object',
'additionalProperties': {
'type': 'string'
},
'maxProperties': 1,
'minProperties': 1,
},
}
]
}
In the callers, we just use:
'remote_identity': _PRORPERTY_NAME_OR_CLUSTER_NAME_TO_PROPERTY
'security_group_name': _PRORPERTY_NAME_OR_CLUSTER_NAME_TO_PROPERTY
sky/resources.py
Outdated
if security_group_name is not None: | ||
with ux_utils.print_exception_no_traceback(): | ||
logger.warning( | ||
f'Ports {self.ports} and security group name are ' | ||
f'specified: {security_group_name}. It is not ' | ||
'guaranteed that the ports will be opened if the ' | ||
'specified security group is not correctly set up. ' | ||
'Please try to specify `ports` only and leave out ' | ||
'`aws.security_group_name` in `~/.sky/config.yaml` to ' | ||
'allow SkyPilot to automatically create and configure ' | ||
'the security group.') |
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.
This is a good catch! Let's remove the warning here and move it to the aws.make_deploy_resources_variables
so that the warning will only shown when launching an exact launchable resource. (check out the comment in aws.py above)
0b2d5b2
to
ed48c42
Compare
@Michaelvll Could you please re-review this PR? Thanks! |
9709440
to
0af7e9a
Compare
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.
Thanks for the update @JGSweets! Sorry for the delay. The PR looks mostly good to me, except some minor fixes. : )
sky/backends/backend_utils.py
Outdated
('available_node_types', 'ray.worker.default', 'node_config', | ||
'IamInstanceProfile'), |
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.
I don't think we need this, as our new provisioner no longer needs to look into the worker config. Similar for the line below, let's remove the line for ray.worker.default
, 'node_config', 'UserData'`
@@ -154,7 +154,11 @@ | |||
# we need to take this field from the new yaml. | |||
('provider', 'tpu_node'), | |||
('provider', 'security_group', 'GroupName'), | |||
('available_node_types', 'ray.head.default', 'node_config', |
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.
The behavior sounds fine to me, except that it may cause a risk that the controller may get a different IamInstanceProfile that no longer has permission to access the previous service replicas or managed job clusters.
sky/clouds/aws.py
Outdated
if user_security_group is not None and not isinstance( | ||
user_security_group, str): |
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.
nit: this seems more direct to me
if user_security_group is not None and not isinstance( | |
user_security_group, str): | |
if user_security_group is not None and isinstance(user_security_group, list): |
sky/clouds/aws.py
Outdated
# Already checked in Resources._try_validate_ports | ||
security_group = USER_PORTS_SECURITY_GROUP_NAME.format( | ||
cluster_name.name_on_cloud) | ||
elif security_group is not None and resources.ports is not None: |
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.
Seems this is ensured by the previous if statement.
elif security_group is not None and resources.ports is not None: | |
elif resources.ports is not None: |
sky/clouds/aws.py
Outdated
f'Ports {resources.ports} and security group name are ' | ||
f'specified: {security_group}. It is not ' | ||
'guaranteed that the ports will be opened if the ' | ||
'specified security group is not correctly set up. ' | ||
'Please try to specify `ports` only and leave out ' | ||
'`aws.security_group_name` in `~/.sky/config.yaml` to ' | ||
'allow SkyPilot to automatically create and configure ' | ||
'the security group.') |
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.
nit: simplified and clearified the logging a little bit.
f'Ports {resources.ports} and security group name are ' | |
f'specified: {security_group}. It is not ' | |
'guaranteed that the ports will be opened if the ' | |
'specified security group is not correctly set up. ' | |
'Please try to specify `ports` only and leave out ' | |
'`aws.security_group_name` in `~/.sky/config.yaml` to ' | |
'allow SkyPilot to automatically create and configure ' | |
'the security group.') | |
f'Skip opening ports {resources.ports} for cluster {cluster_name!r}, ' | |
'as `aws.security_group_name` in `~/.sky/config.yaml` is specified as ' | |
f' {security_group!r}. Please make sure the specified security group ' | |
'has requested ports setup; or, leave out `aws.security_group_name` ' | |
'in `~/.sky/config.yaml`.') |
'security_group_name': | ||
(_PRORPERTY_NAME_OR_CLUSTER_NAME_TO_PROPERTY), |
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.
Do we need this parenthesis?
'security_group_name': | |
(_PRORPERTY_NAME_OR_CLUSTER_NAME_TO_PROPERTY), | |
'security_group_name': _PRORPERTY_NAME_OR_CLUSTER_NAME_TO_PROPERTY, |
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.
Unfortunately, black would fix to what you suggest, but then the linter would say the line is too long. I can add a commented exception if we want instead.
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.
Ahh, no worries then. I was thinking it might be within the length limit.
sky/clouds/aws.py
Outdated
if user_security_group is not None and not isinstance( | ||
user_security_group, str): | ||
for profile in user_security_group: | ||
if fnmatch.fnmatchcase(cluster_name.name_on_cloud, |
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.
Should use display name for matching, as it is user facing:
if fnmatch.fnmatchcase(cluster_name.name_on_cloud, | |
if fnmatch.fnmatchcase(cluster_name.display_name, |
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.
If we change this for security groups, we should also change this for IAM. I'll add that to this PR if that's okay. Nevermind, IAM already uses cluster_name
which equates to display_name
!
tests/unit_tests/test_resources.py
Outdated
expected_config.update({ | ||
'security_group': "fake-skypilot-default", | ||
'security_group_managed_by_skypilot': 'false' | ||
}) |
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.
How about checking the remote_identity
as well?
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.
remote_identity
is not constructed here, however, new tests were added to fix that.
security_group_name: | ||
- sky-serve-fake1-*: fake-1-sg | ||
- sky-serve-fake2-*: fake-2-sg | ||
- "*": fake-skypilot-default |
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.
Let's get rid of this "*"
item here to test the default remote_identity set by SkyPilot?
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.
Good catch!
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.
Discovered and fixed a bug related to this.
tests/unit_tests/test_resources.py
Outdated
|
||
# test using culuster matches regex, top | ||
cluster_name = resources_utils.ClusterName( | ||
display_name='display', name_on_cloud='sky-serve-fake1-1234') |
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.
Let's match the cluster name by the display name instead of the name_on_cloud
as the former is the one a user actually specifies?
@@ -803,11 +808,13 @@ def write_cluster_config( | |||
|
|||
assert cluster_name is not None | |||
excluded_clouds = [] | |||
remote_identity = skypilot_config.get_nested( |
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.
@Michaelvll fixed a bug here when there was no match.
@@ -397,18 +399,33 @@ def make_deploy_resources_variables(self, | |||
image_id = self._get_image_id(image_id_to_use, region_name, | |||
r.instance_type) | |||
|
|||
user_security_group = skypilot_config.get_nested( | |||
|
|||
user_security_group_config = skypilot_config.get_nested( |
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.
this fixes a bug if no match is found when a list is provided
d336bc4
to
ba68d5e
Compare
@patch('sky.backends.backend_utils._get_yaml_path_from_cluster_name', | ||
return_value='/tmp/fake/path') | ||
@patch('sky.utils.common_utils.fill_template') | ||
def test_write_cluster_config_w_remote_identity(mock_fill_template, |
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.
new test for remote identity, this is somewhat redundant with the resource_utils test since it is encapsulated in write_cluster_config
.
I can remove that test if we desire.
097da3b
to
22221f7
Compare
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.
Thanks for the updates @JGSweets and sorry for the delay! The latest PR looks great to me and the newly added tests look awesome! I am running a smoke test on aws right now and once it passes, we should be good to go.
-
pytest tests/test_smoke.py --aws
-
tests/backward_compatibility_tests.sh
'security_group_name': | ||
(_PRORPERTY_NAME_OR_CLUSTER_NAME_TO_PROPERTY), |
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.
Ahh, no worries then. I was thinking it might be within the length limit.
Seems we have missed one place below: It should be the following instead?
|
22221f7
to
7796022
Compare
@Michaelvll Good find! Not sure how I missed that. I've fixed it. |
Updated tests passed as well. |
clone_disk_from, | ||
handle.cluster_name_on_cloud, | ||
cluster_name=resources_utils.ClusterName( | ||
display_name=clone_disk_from, |
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.
@Michaelvll is this update correct?
Sorry for the delay @JGSweets! The tests are all passed. Merging it now. Thank you for your contribution! |
) * feat: allow security group specification * feat: refactor format of sg names * refactor: update readme for security group name update * fix: format * refactor: use ClusterName data class * fix: move warning * fix: clean code * fix: clean code * fix: schema constant * refactor: add sg test * fix: pylint * refactor: updates to use display name * fix: bug in remote identity and update tests * fix: formatting * fix: remove * fix: format * fix: missing resources_utils ClusterName * fix: tests * fix: bug * fix: clone_disk_from reference
Similar to #3488 but for SGs
This PR:
Addresses: #3473
In
~/.sky/config.yaml
:security_group_name
via stringTested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_comaptibility_tests.sh
EDITED: