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

AWS: support custom instance tags in ~/.sky/config.yaml. #1992

Merged
merged 4 commits into from
May 28, 2023

Conversation

concretevitamin
Copy link
Collaborator

@concretevitamin concretevitamin commented May 27, 2023

To use this, write to ~/.sky/config.yaml:

aws:
  # Tags (key-value pairs) to assign to all instances launched by SkyPilot for
  # this cloud.
  #
  # Users should guarantee that these key-values are valid AWS tags, otherwise
  # errors from the cloud provider will be surfaced.
  instance_tags:
    my-tag-1: my-value-1
    my-tag-2: my-value-2
    tag with spaces: my-value
    value-with-spaces: my value
Screen Shot 2023-05-27 at 15 48 07

Tested (run the relevant ones):

  • Any manual or new tests for this PR (please specify below)
    • Good case above
    • Bad case: a key with 129 characters (1 above limit)
      • This triggers failover with errors in each region create_instances: Attempt failed with An error occurred (InvalidParameterValue) when calling the RunInstances operation: Tag key exceeds the maximum length of 128 characters, retrying.
      • TODO: We can optimize this in the future
    • Back compat: sky launch on a previously autostopped cluster with new tags specified
      • Observed that new tags are not applied to old cluster (expected)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@Michaelvll
Copy link
Collaborator

Just a quick question, do we want to have it in the global config or the task specification instead? I am wondering if the user would like to have different tag for different tag.

@concretevitamin
Copy link
Collaborator Author

concretevitamin commented May 27, 2023 via email

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature in such a short time @concretevitamin! LGTM. It should be good to go in, if the tests are passed.

@concretevitamin
Copy link
Collaborator Author

Tested:

  • Good case above

  • Bad case: a key with 129 characters (1 above limit)

    • This triggers failover with errors in each region create_instances: Attempt failed with An error occurred (InvalidParameterValue) when calling the RunInstances operation: Tag key exceeds the maximum length of 128 characters, retrying.
    • TODO: We can optimize this in the future
  • Back compat: sky launch on a previously autostopped cluster with new tags specified

    • Observed that new tags are not applied to old cluster (expected)

@concretevitamin concretevitamin merged commit 156572b into master May 28, 2023
15 checks passed
@concretevitamin concretevitamin deleted the aws-custom-tags branch May 28, 2023 16:07
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.

None yet

2 participants