[autoscaler][aws] Update _configure_key_pair to available_node_types #15108
Conversation
There was a problem hiding this comment.
Can you modify log_to_cli to also check available node types?
| def test_missing_key(iam_client_stub): | ||
| """ | ||
| Check that an error is raised when ssh_private_key is given in the | ||
| auth_config but a node type is missing KeyName. | ||
| """ | ||
| config = helpers.load_aws_example_config_file("example-full.yaml") | ||
| config["auth"]["ssh_private_key"] = "~/fake/path" | ||
| config["available_node_types"]["ray.head.default"]["node_config"][ | ||
| "KeyName"] = "fakeKey" | ||
|
|
||
| # bootstrap_config configures iam role and then raises an assertion error | ||
| stubs.configure_iam_role_default(iam_client_stub) | ||
| with pytest.raises(AssertionError): | ||
| helpers.bootstrap_aws_config(config) | ||
|
|
||
| # expect no pending responses left in IAM client stub queue | ||
| iam_client_stub.assert_no_pending_responses() | ||
|
|
There was a problem hiding this comment.
Could you add one more test to ensure the UserData configuration works correctly? i.e. having the ssh_private_key passed in and having ["node_config"]["UserData"] set to a non-empty value
Eh, yeah, thanks -- need to consider this carefully, as that's way it currently has to be done... In an initial version, had a check for that, then got rid of it.. |
|
@DmitriGekhtman I think it might be good to support that workflow for a bit longer? What are your thoughts @AmeerHajAli |
Yes, for sure. No need to change things suddenly. |
DmitriGekhtman
added
the
@author-action-required
| if "UserData" not in node_config: | ||
| cli_logger.doassert( # todo: verify schema beforehand? | ||
| "KeyName" in node_config, | ||
| _key_assert_msg(node_type)) # todo: err msg |
| for node_type in node_types: | ||
| node_config = node_types[node_type]["node_config"] | ||
| if "UserData" not in node_config: | ||
| cli_logger.doassert( # todo: verify schema beforehand? |
| node_config["KeyName"] == DEFAULT_KEY_PAIR["KeyName"] | ||
| node_config.pop("KeyName") |
There was a problem hiding this comment.
What are you trying to do?
I don't understand
| config["auth"]["ssh_private_key"] = "~/fake/path" | ||
| config["available_node_types"]["ray.head.default"]["node_config"][ | ||
| "KeyName"] = "fakeKey" | ||
|
|
There was a problem hiding this comment.
Lets explicitly unset the "ray.worker_default" to make it easier to read this code and understand that there is another key that does not have a KeyName.
| config["head_node"]["KeyName"] = key_name | ||
| config["worker_nodes"]["KeyName"] = key_name | ||
| for node_type in node_types: | ||
| node_config = node_types[node_type]["node_config"] | ||
| node_config["KeyName"] = key_name |
There was a problem hiding this comment.
When doing something like this, we should explicitly raise a warning saying that KeyName in head_node/worker_nodes are deprecated and they must be passed in the node config.
|
@DmitriGekhtman , closing this as it is no longer necessary, right? |
Right, superseded by #15584! |
Why are these changes needed?
Closes #15096
Addresses key_pair part of #14856
Related issue number
Checks
scripts/format.shto lint the changes in this PR.Added tests to check key-pair filling logic and assertion error on missing KeyName.
Did manual test: Can launch AWS cluster by specifying KeyName under available node types.