Skip to content
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

[autoscaler] Translate to/from AWS 'Name' tag #2219

Merged
merged 2 commits into from
Jun 11, 2018

Conversation

ericl
Copy link
Contributor

@ericl ericl commented Jun 8, 2018

What do these changes do?

GCP support moved tags to have cross-cloud names (#2061), however we should still use "Name" for the node name. GCP doesn't support upper-case tags, so implement this translation in the AWS node provider.

fyi @hartikainen

Related issue number

#2209

@hartikainen
Copy link
Contributor

Looks good to me. This makes me wonder if it might be, after all, better to use provider specific names for the tags. Having one condition here is not a problem, but who knows, maybe in future we end up having multiple similar cases where we have to do something provider specific configurations.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/5946/
Test FAILed.

@ericl
Copy link
Contributor Author

ericl commented Jun 9, 2018

It's a fair point, if I knew about this edge case that would definitely have been the preferable option before...

@@ -59,7 +78,7 @@ def node_tags(self, node_id):
tags = {}
for tag in node.tags:
tags[tag["Key"]] = tag["Value"]
return tags
return from_aws_format(tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this for this method in particular? ie, why not have this for all methods where you call to_aws_format?

tags is a dict and is passed by ref; so every time you call to_aws_format you end up modifying the original...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only one that returns tags?

And yes, the mutation is fine.

Copy link
Contributor

@richardliaw richardliaw left a comment

Choose a reason for hiding this comment

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

lgtm

@ericl
Copy link
Contributor Author

ericl commented Jun 11, 2018

Tests look unrelated.

@ericl ericl merged commit 7fcaad2 into ray-project:master Jun 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants