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

[k8s] Attach custom metadata to objects created by SkyPilot #3333

Merged
merged 7 commits into from
Mar 22, 2024

Conversation

romilbhardwaj
Copy link
Collaborator

@romilbhardwaj romilbhardwaj commented Mar 19, 2024

Closes #3317. Adds the ability to attach custom metadata to all objects created by SkyPilot. Useful for accounting and management of SkyPilot specific resources.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Manual testing by inspecting resources with kubectl describe ... with both ingress and loadbalancer port modes.
  • Kubernetes smoke tests in progress pytest tests/test_smoke.py --kubernetes

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 the quick fix @romilbhardwaj! Left a question : )

Comment on lines +285 to +289
custom_metadata:
labels:
mylabel: myvalue
annotations:
myannotation: myvalue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are both of the labels and annotations required? We are using instance_tags: for the other clouds to allow user specify custom tags for all the VMs created. Would it be possible to have a similar argument for k8s pods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both are not required, they can be different or even just one can be present. Labels and annotations serve different purposes - labels are used for filtering, selecting and scheduling, while annotations are metadata that can be consumed by third party tools (e.g., prometheus.io/scrape annotations tells prometheus which objects to scrape).

Given this, users may want to set labels and/or annotations, this config gives them the flexibility to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense! Maybe in the future, we can provide also provide instance_tags as an alias for specifying both labels and annotations with the same set of key, values.

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 support @romilbhardwaj! LGTM.

Comment on lines +816 to +817
# Add custom metadata from config
merge_custom_metadata(content['service_spec']['metadata'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: If we have multiple users sharing the same jump pod, and they have different metadata field setup in their ~/.sky/config.yaml. What will happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great question - the metadata specified during the first creation of the ssh jump pod will be used throughout it's life. Other users sharing the jump pod will not have their metadata overwrite the existing pod and service's metadata.

@@ -1059,7 +1068,48 @@ def get_endpoint_debug_message() -> str:
debug_cmd=debug_cmd)


def combine_pod_config_fields(config_yaml_path: str) -> None:
def merge_dicts(source: Dict[Any, Any], destination: Dict[Any, Any]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

make this function private? Seems only used within the same file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's also used in authentication.py, where we use merge_dicts when setting up the secrets..

Comment on lines +285 to +289
custom_metadata:
labels:
mylabel: myvalue
annotations:
myannotation: myvalue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense! Maybe in the future, we can provide also provide instance_tags as an alias for specifying both labels and annotations with the same set of key, values.

@romilbhardwaj romilbhardwaj merged commit 8f4f6f8 into master Mar 22, 2024
19 checks passed
@romilbhardwaj romilbhardwaj deleted the k8s_custom_metadata branch March 22, 2024 07:41
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.

[k8s] Add ability to add custom annotations/metadata to k8s objects created by SkyPilot.
2 participants