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] Fix resources.image_id backward compatibility #4425

Merged
merged 4 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
5 changes: 3 additions & 2 deletions sky/clouds/kubernetes.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ class Kubernetes(clouds.Cloud):
SKY_SSH_KEY_SECRET_NAME = 'sky-ssh-keys'
SKY_SSH_JUMP_NAME = 'sky-ssh-jump-pod'

LEGACY_SINGLETON_REGION = 'kubernetes'

# Limit the length of the cluster name to avoid exceeding the limit of 63
# characters for Kubernetes resources. We limit to 42 characters (63-21) to
# allow additional characters for creating ingress services to expose ports.
Expand All @@ -54,7 +56,6 @@ class Kubernetes(clouds.Cloud):
_DEFAULT_MEMORY_CPU_RATIO = 1
_DEFAULT_MEMORY_CPU_RATIO_WITH_GPU = 4 # Allocate more memory for GPU tasks
_REPR = 'Kubernetes'
_LEGACY_SINGLETON_REGION = 'kubernetes'
_CLOUD_UNSUPPORTED_FEATURES = {
# TODO(romilb): Stopping might be possible to implement with
# container checkpointing introduced in Kubernetes v1.25. See:
Expand Down Expand Up @@ -630,7 +631,7 @@ def instance_type_exists(self, instance_type: str) -> bool:
instance_type)

def validate_region_zone(self, region: Optional[str], zone: Optional[str]):
if region == self._LEGACY_SINGLETON_REGION:
if region == self.LEGACY_SINGLETON_REGION:
# For backward compatibility, we allow the region to be set to the
# legacy singleton region.
# TODO: Remove this after 0.9.0.
Expand Down
23 changes: 22 additions & 1 deletion sky/resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class Resources:
"""
# If any fields changed, increment the version. For backward compatibility,
# modify the __setstate__ method to handle the old version.
_VERSION = 19
_VERSION = 20

def __init__(
self,
Expand Down Expand Up @@ -1607,4 +1607,25 @@ def __setstate__(self, state):
self._cluster_config_overrides = state.pop(
'_cluster_config_overrides', None)

if version < 20:
# Pre-0.7.0, we used 'kubernetes' as the default region for
# Kubernetes clusters. With the introduction of support for
# multiple contexts, we now set the region to the context name.
# Since we do not have information on which context the cluster
# was run in, we default it to the current active context.
legacy_region = clouds.Kubernetes().LEGACY_SINGLETON_REGION
original_cloud = state.get('_cloud', None)
original_region = state.get('_region', None)
if (clouds.Kubernetes().is_same_cloud(original_cloud) and
romilbhardwaj marked this conversation as resolved.
Show resolved Hide resolved
original_region == legacy_region):
current_context = (
kubernetes_utils.get_current_kube_config_context_name())
Comment on lines +1621 to +1622
Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious, do we have k8s_namspace or k8s_context noted in the cluster yaml in previous version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have namespace, but unfortunately context was added to our j2 after 0.6.1 in #3913.

state['_region'] = current_context
# Also update the image_id dict if it contains the old region
if isinstance(state['_image_id'], dict):
if legacy_region in state['_image_id']:
state['_image_id'][current_context] = (
state['_image_id'][legacy_region])
del state['_image_id'][legacy_region]

self.__dict__.update(state)