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] Use images.csv catalog to get container image tags #2556

Merged
merged 6 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions sky/clouds/kubernetes.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from sky import sky_logging
from sky import status_lib
from sky.adaptors import kubernetes
from sky.clouds import service_catalog
from sky.utils import common_utils
from sky.utils import kubernetes_utils
from sky.utils import ux_utils
Expand Down Expand Up @@ -66,10 +67,8 @@ class Kubernetes(clouds.Cloud):
('Docker image is not supported in Kubernetes. ')
}

IMAGE_CPU = ('us-central1-docker.pkg.dev/'
'skypilot-375900/skypilotk8s/skypilot:latest')
IMAGE_GPU = ('us-central1-docker.pkg.dev/skypilot-375900/'
'skypilotk8s/skypilot-gpu:latest')
IMAGE_CPU = 'skypilot:cpu-ubuntu-2004'
IMAGE_GPU = 'skypilot:gpu-ubuntu-2004'

@classmethod
def _cloud_unsupported_features(
Expand Down Expand Up @@ -202,7 +201,14 @@ def make_deploy_resources_variables(
acc_type = k.accelerator_type if k.accelerator_type else None

# Select image based on whether we are using GPUs or not.
image = self.IMAGE_GPU if acc_count > 0 else self.IMAGE_CPU
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:'):
romilbhardwaj marked this conversation as resolved.
Show resolved Hide resolved
image_id = service_catalog.get_image_id_from_tag(
image_id, clouds='kubernetes')
Copy link
Collaborator

@landscapepainter landscapepainter Sep 15, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?


k8s_acc_label_key = None
k8s_acc_label_value = None
Expand All @@ -224,7 +230,7 @@ def make_deploy_resources_variables(
'k8s_acc_label_key': k8s_acc_label_key,
'k8s_acc_label_value': k8s_acc_label_value,
# TODO(romilb): Allow user to specify custom images
'image_id': image,
'image_id': image_id,
}
return deploy_vars

Expand Down
4 changes: 1 addition & 3 deletions sky/clouds/service_catalog/README.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# Service Catalog

We support 3 major clouds: AWS, Azure and GCP.

This module provides information including the instance type offerings, their pricing and data transfer costs. It also provides functions to query these information, and to select the most suitable instance types based on resource requirements. Primarily used by the Clouds module.
This module provides information for clouds supported by SkyPilot, including the instance type offerings, their pricing and data transfer costs. It also provides functions to query these information, and to select the most suitable instance types based on resource requirements. Primarily used by the Clouds module.

- `data_fetchers/fetch_{aws,azure}.py`: each file is a standalone script that queries the cloud APIs to produce the pricing list files.
- `data_fetchers/fetch_gcp.py`: A script that generates the GCP catalog based by crawling GCP websites.
Expand Down
2 changes: 1 addition & 1 deletion sky/clouds/service_catalog/constants.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Constants used for service catalog."""
import os

HOSTED_CATALOG_DIR_URL = 'https://raw.githubusercontent.com/skypilot-org/skypilot-catalog/master/catalogs' # pylint: disable=line-too-long
HOSTED_CATALOG_DIR_URL = 'https://raw.githubusercontent.com/romilbhardwaj/skypilot-catalog/k8s_images/catalogs' # TODO - REVERT AFTER CATALOG PR IS MERGED. pylint: disable=line-too-long
romilbhardwaj marked this conversation as resolved.
Show resolved Hide resolved
CATALOG_SCHEMA_VERSION = 'v5'
LOCAL_CATALOG_DIR = os.path.expanduser('~/.sky/catalogs/')
31 changes: 31 additions & 0 deletions sky/clouds/service_catalog/kubernetes_catalog.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
"""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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mapping SkyPilot image tags to corresponding container image tags.
mapping SkyPilot image tags to corresponding container image ids.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

"""

from typing import Optional

from sky import sky_logging
from sky.clouds.service_catalog import common

logger = sky_logging.init_logger(__name__)
romilbhardwaj marked this conversation as resolved.
Show resolved Hide resolved

_PULL_FREQUENCY_HOURS = 7

# We keep pull_frequency_hours so we can remotely update the default image paths
_image_df = common.read_catalog('kubernetes/images.csv',
pull_frequency_hours=_PULL_FREQUENCY_HOURS)

# TODO(romilb): Refactor implementation of common service catalog functions from
# clouds/kubernetes.py to kubernetes_catalog.py


def get_image_id_from_tag(tag: str, region: Optional[str]) -> Optional[str]:
"""Returns the image id from the tag."""
return common.get_image_id_from_tag_impl(_image_df, tag, region)


def is_image_tag_valid(tag: str, region: Optional[str]) -> bool:
"""Returns whether the image tag is valid."""
return common.is_image_tag_valid_impl(_image_df, tag, region)