[k8s] Use images.csv catalog to get container image tags
#2556
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
landscapepainter
approved these changes
Sep 15, 2023
There was a problem hiding this comment.
Awesome @romilbhardwaj. Left some comments. Tested out and it works smoothly. This should be good to go after.
| """Kubernetes Catalog. | ||
|
|
||
| Kubernetes does not require a catalog of instances, but we need an image catalog | ||
| mapping SkyPilot image tags to corresponding container image tags. |
There was a problem hiding this comment.
Suggested change
| mapping SkyPilot image tags to corresponding container image tags. | |
| mapping SkyPilot image tags to corresponding container image ids. |
There was a problem hiding this comment.
Hmm, they are still container image tags (e.g., ubuntu:latest), since container image ids are hex values and are typically not used to specify images to use.
sky/clouds/kubernetes.py
Outdated
Comment on lines
204
to
211
| image_id = self.IMAGE_GPU if acc_count > 0 else self.IMAGE_CPU | ||
| # Get the container image ID from the service catalog. | ||
| # Note that currently we do not support custom images, so this condition | ||
| # will always be triggered. In the future we may want to get image_id | ||
| # from the resources object if it is set. | ||
| if image_id.startswith('skypilot:'): | ||
| image_id = service_catalog.get_image_id_from_tag( | ||
| image_id, clouds='kubernetes') |
There was a problem hiding this comment.
Suggested change
| image_id = self.IMAGE_GPU if acc_count > 0 else self.IMAGE_CPU | |
| # Get the container image ID from the service catalog. | |
| # Note that currently we do not support custom images, so this condition | |
| # will always be triggered. In the future we may want to get image_id | |
| # from the resources object if it is set. | |
| if image_id.startswith('skypilot:'): | |
| image_id = service_catalog.get_image_id_from_tag( | |
| image_id, clouds='kubernetes') | |
| image_id = None | |
| image_tag = self.IMAGE_GPU if acc_count > 0 else self.IMAGE_CPU | |
| # Get the container image ID from the service catalog. | |
| # Note that currently we do not support custom images, so this condition | |
| # will always be triggered. In the future we may want to get image_id | |
| # from the resources object if it is set. | |
| if image_tag.startswith('skypilot:'): | |
| image_id = service_catalog.get_image_id_from_tag( | |
| image_tag, clouds='kubernetes') |
Perhaps we should separate between image_tag and image_id?
There was a problem hiding this comment.
agreed, its a bit ambiguous here. However, since the image_id can also come from resources in the future, perhaps its best to leave image_id here?
Michaelvll
reviewed
Sep 15, 2023
|
Thanks for the feedback @landscapepainter and @Michaelvll! Merging now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2430.
We now use
images.csvto store a mapping of skypilot image tags to actual container registry tags. This will allow us to push updates to the path/tag of the default k8s image used without having to update clients. Previously, these container tags were hardcoded in kubernetes.py.bash format.shsky launch -c mycluster --cloud kubernetesconstants.pyafter [k8s] Addimages.csvfor Kubernetes skypilot-catalog#47 is merged.