Repository: skypilot-org/skypilot
Pull request #4526 (2025-01-03 16:07:03)

[docs] Add image_id doc in task YAML for OCI
<!-- Describe the changes in this PR -->

doc for image_id for OCI.

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [ ] Code formatting: `bash format.sh`
- [ ] Any manual or new tests for this PR (please specify below)
- [ ] 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: `conda deactivate; bash -i tests/backward_compatibility_tests.sh`


HysunHe: @cblmemo This is the doc for image_id description for OCI of the task YAML.

Patch:
diff --git a/docs/source/reference/yaml-spec.rst b/docs/source/reference/yaml-spec.rst
index 8a490b7e8..d2f050699 100644
--- a/docs/source/reference/yaml-spec.rst
+++ b/docs/source/reference/yaml-spec.rst
@@ -176,9 +176,9 @@ Available fields:
       # tpu_vm: True  # True to use TPU VM (the default); False to use TPU node.
 
       # Custom image id (optional, advanced). The image id used to boot the
-      # instances. Only supported for AWS and GCP (for non-docker image). If not
-      # specified, SkyPilot will use the default debian-based image suitable for
-      # machine learning tasks.
+      # instances. Only supported for AWS, GCP, OCI and IBM (for non-docker image).
+      # If not specified, SkyPilot will use the default debian-based image
+      # suitable for machine learning tasks.
       #
       # Docker support
       # You can specify docker image to use by setting the image_id to
@@ -204,7 +204,7 @@ Available fields:
       #   image_id:
       #     us-east-1: ami-0729d913a335efca7
       #     us-west-2: ami-050814f384259894c
-      image_id: ami-0868a20f5a3bf9702
+      #
       # GCP
       # To find GCP images: https://cloud.google.com/compute/docs/images
       # image_id: projects/deeplearning-platform-release/global/images/common-cpu-v20230615-debian-11-py310
@@ -215,6 +215,24 @@ Available fields:
       # To find Azure images: https://docs.microsoft.com/en-us/azure/virtual-machines/linux/cli-ps-findimage
       # image_id: microsoft-dsvm:ubuntu-2004:2004:21.11.04
       #
+      # OCI
+      # To find OCI images: https://docs.oracle.com/en-us/iaas/images
+      # You can choose the image with OS version from the following image tags 
+      # provided by SkyPilot:
+      #   image_id: skypilot:gpu-ubuntu-2204
+      #   image_id: skypilot:gpu-ubuntu-2004
+      #   image_id: skypilot:gpu-oraclelinux9
+      #   image_id: skypilot:gpu-oraclelinux8
+      #   image_id: skypilot:cpu-ubuntu-2204
+      #   image_id: skypilot:cpu-ubuntu-2004
+      #   image_id: skypilot:cpu-oraclelinux9
+      #   image_id: skypilot:cpu-oraclelinux8
+      #
+      # It is also possible to specify your custom image's OCID with OS type,
+      # for example:
+      #   image_id: ocid1.image.oc1.us-sanjose-1.aaaaaaaaywwfvy67wwe7f24juvjwhyjn3u7g7s3wzkhduxcbewzaeki2nt5q:oraclelinux
+      #   image_id: ocid1.image.oc1.us-sanjose-1.aaaaaaaa5tnuiqevhoyfnaa5pqeiwjv6w5vf6w4q2hpj3atyvu3yd6rhlhyq:ubuntu
+      #
       # IBM
       # Create a private VPC image and paste its ID in the following format:
       # image_id: <unique_image_id>
@@ -224,6 +242,7 @@ Available fields:
       # https://www.ibm.com/cloud/blog/use-ibm-packer-plugin-to-create-custom-images-on-ibm-cloud-vpc-infrastructure
       # To use a more limited but easier to manage tool:
       # https://github.com/IBM/vpc-img-inst
+      image_id: ami-0868a20f5a3bf9702
 
       # Labels to apply to the instances (optional).
       #
diff --git a/sky/backends/backend_utils.py b/sky/backends/backend_utils.py
index 1de799e7c..6e79469a8 100644
--- a/sky/backends/backend_utils.py
+++ b/sky/backends/backend_utils.py
@@ -926,6 +926,13 @@ def write_cluster_config(
             tmp_yaml_path,
             cluster_config_overrides=to_provision.cluster_config_overrides)
         kubernetes_utils.combine_metadata_fields(tmp_yaml_path)
+        yaml_obj = common_utils.read_yaml(tmp_yaml_path)
+        pod_config = yaml_obj['available_node_types']['ray_head_default'][
+            'node_config']
+        valid, message = kubernetes_utils.check_pod_config(pod_config)
+        if not valid:
+            raise exceptions.InvalidCloudConfigs(
+                f'Invalid pod_config. Details: {message}')
 
     if dryrun:
         # If dryrun, return the unfinished tmp yaml path.
diff --git a/sky/backends/wheel_utils.py b/sky/backends/wheel_utils.py
index ed580569e..805117ee2 100644
--- a/sky/backends/wheel_utils.py
+++ b/sky/backends/wheel_utils.py
@@ -153,7 +153,10 @@ def build_sky_wheel() -> Tuple[pathlib.Path, str]:
         if not path.exists():
             return -1.
         try:
-            return max(os.path.getmtime(root) for root, _, _ in os.walk(path))
+            return max(
+                os.path.getmtime(os.path.join(root, f))
+                for root, dirs, files in os.walk(path)
+                for f in (*dirs, *files))
         except ValueError:
             return -1.
 
diff --git a/sky/clouds/aws.py b/sky/clouds/aws.py
index cafc789c5..c665263e2 100644
--- a/sky/clouds/aws.py
+++ b/sky/clouds/aws.py
@@ -2,6 +2,8 @@
 import enum
 import fnmatch
 import functools
+import hashlib
+import json
 import os
 import re
 import subprocess
@@ -16,6 +18,7 @@ from sky import sky_logging
 from sky import skypilot_config
 from sky.adaptors import aws
 from sky.clouds import service_catalog
+from sky.clouds.service_catalog import common as catalog_common
 from sky.clouds.utils import aws_utils
 from sky.skylet import constants
 from sky.utils import common_utils
@@ -624,14 +627,10 @@ class AWS(clouds.Cloud):
 
     @classmethod
     def _current_identity_type(cls) -> Optional[AWSIdentityType]:
-        proc = subprocess.run('aws configure list',
-                              shell=True,
-                              check=False,
-                              stdout=subprocess.PIPE,
-                              stderr=subprocess.PIPE)
-        if proc.returncode != 0:
+        stdout = cls._aws_configure_list()
+        if stdout is None:
             return None
-        stdout = proc.stdout.decode()
+        output = stdout.decode()
 
         # We determine the identity type by looking at the output of
         # `aws configure list`. The output looks like:
@@ -646,10 +645,10 @@ class AWS(clouds.Cloud):
 
         def _is_access_key_of_type(type_str: str) -> bool:
             # The dot (.) does not match line separators.
-            results = re.findall(fr'access_key.*{type_str}', stdout)
+            results = re.findall(fr'access_key.*{type_str}', output)
             if len(results) > 1:
                 raise RuntimeError(
-                    f'Unexpected `aws configure list` output:\n{stdout}')
+                    f'Unexpected `aws configure list` output:\n{output}')
             return len(results) == 1
 
         if _is_access_key_of_type(AWSIdentityType.SSO.value):
@@ -664,37 +663,20 @@ class AWS(clouds.Cloud):
             return AWSIdentityType.SHARED_CREDENTIALS_FILE
 
     @classmethod
-    @functools.lru_cache(maxsize=1)  # Cache since getting identity is slow.
-    def get_user_identities(cls) -> Optional[List[List[str]]]:
-        """Returns a [UserId, Account] list that uniquely identifies the user.
-
-        These fields come from `aws sts get-caller-identity`. We permit the same
-        actual user to:
-
-          - switch between different root accounts (after which both elements
-            of the list will be different) and have their clusters owned by
-            each account be protected; or
-
-          - within the same root account, switch between different IAM
-            users, and treat [user_id=1234, account=A] and
-            [user_id=4567, account=A] to be the *same*. Namely, switching
-            between these IAM roles within the same root account will cause
-            the first element of the returned list to differ, and will allow
-            the same actual user to continue to interact with their clusters.
-            Note: this is not 100% safe, since the IAM users can have very
-            specific permissions, that disallow them to access the clusters
-            but it is a reasonable compromise as that could be rare.
-
-        Returns:
-            A list of strings that uniquely identifies the user on this cloud.
-            For identity check, we will fallback through the list of strings
-            until we find a match, and print a warning if we fail for the
-            first string.
+    @functools.lru_cache(maxsize=1)
+    def _aws_configure_list(cls) -> Optional[bytes]:
+        proc = subprocess.run('aws configure list',
+                              shell=True,
+                              check=False,
+                              stdout=subprocess.PIPE,
+                              stderr=subprocess.PIPE)
+        if proc.returncode != 0:
+            return None
+        return proc.stdout
 
-        Raises:
-            exceptions.CloudUserIdentityError: if the user identity cannot be
-                retrieved.
-        """
+    @classmethod
+    @functools.lru_cache(maxsize=1)  # Cache since getting identity is slow.
+    def _sts_get_caller_identity(cls) -> Optional[List[List[str]]]:
         try:
             sts = aws.client('sts')
             # The caller identity contains 3 fields: UserId, Account, Arn.
@@ -773,6 +755,72 @@ class AWS(clouds.Cloud):
         #   automatic switching for AWS. Currently we only support one identity.
         return [user_ids]
 
+    @classmethod
+    @functools.lru_cache(maxsize=1)  # Cache since getting identity is slow.
+    def get_user_identities(cls) -> Optional[List[List[str]]]:
+        """Returns a [UserId, Account] list that uniquely identifies the user.
+
+        These fields come from `aws sts get-caller-identity` and are cached
+        locally by `aws configure list` output. The identities are assumed to
+        be stable for the duration of the `sky` process. Modifying the
+        credentials while the `sky` process is running will not affect the
+        identity returned by this function.
+
+        We permit the same actual user to:
+
+          - switch between different root accounts (after which both elements
+            of the list will be different) and have their clusters owned by
+            each account be protected; or
+
+          - within the same root account, switch between different IAM
+            users, and treat [user_id=1234, account=A] and
+            [user_id=4567, account=A] to be the *same*. Namely, switching
+            between these IAM roles within the same root account will cause
+            the first element of the returned list to differ, and will allow
+            the same actual user to continue to interact with their clusters.
+            Note: this is not 100% safe, since the IAM users can have very
+            specific permissions, that disallow them to access the clusters
+            but it is a reasonable compromise as that could be rare.
+
+        Returns:
+            A list of strings that uniquely identifies the user on this cloud.
+            For identity check, we will fallback through the list of strings
+            until we find a match, and print a warning if we fail for the
+            first string.
+
+        Raises:
+            exceptions.CloudUserIdentityError: if the user identity cannot be
+                retrieved.
+        """
+        stdout = cls._aws_configure_list()
+        if stdout is None:
+            # `aws configure list` is not available, possible reasons:
+            # - awscli is not installed but credentials are valid, e.g. run from
+            #   an EC2 instance with IAM role
+            # - aws credentials are not set, proceed anyway to get unified error
+            #   message for users
+            return cls._sts_get_caller_identity()
+        config_hash = hashlib.md5(stdout).hexdigest()[:8]
+        # Getting aws identity cost ~1s, so we cache the result with the output of
+        # `aws configure list` as cache key. Different `aws configure list` output
+        # can have same aws identity, our assumption is the output would be stable
+        # in real world, so the number of cache files would be limited.
+        # TODO(aylei): consider using a more stable cache key and evalute eviction.
+        cache_path = catalog_common.get_catalog_path(
+            f'aws/.cache/user-identity-{config_hash}.txt')
+        if os.path.exists(cache_path):
+            try:
+                with open(cache_path, 'r', encoding='utf-8') as f:
+                    return json.loads(f.read())
+            except json.JSONDecodeError:
+                # cache is invalid, ignore it and fetch identity again
+                pass
+
+        result = cls._sts_get_caller_identity()
+        with open(cache_path, 'w', encoding='utf-8') as f:
+            f.write(json.dumps(result))
+        return result
+
     @classmethod
     def get_active_user_identity_str(cls) -> Optional[str]:
         user_identity = cls.get_active_user_identity()
diff --git a/sky/provision/kubernetes/utils.py b/sky/provision/kubernetes/utils.py
index 487868d1d..14b6b42aa 100644
--- a/sky/provision/kubernetes/utils.py
+++ b/sky/provision/kubernetes/utils.py
@@ -893,6 +893,52 @@ def check_credentials(context: Optional[str],
         return True, None
 
 
+def check_pod_config(pod_config: dict) \
+    -> Tuple[bool, Optional[str]]:
+    """Check if the pod_config is a valid pod config
+
+    Using deserialize api to check the pod_config is valid or not.
+
+    Returns:
+        bool: True if pod_config is valid.
+        str: Error message about why the pod_config is invalid, None otherwise.
+    """
+    errors = []
+    # This api_client won't be used to send any requests, so there is no need to
+    # load kubeconfig
+    api_client = kubernetes.kubernetes.client.ApiClient()
+
+    # Used for kubernetes api_client deserialize function, the function will use
+    # data attr, the detail ref:
+    # https://github.com/kubernetes-client/python/blob/master/kubernetes/client/api_client.py#L244
+    class InnerResponse():
+
+        def __init__(self, data: dict):
+            self.data = json.dumps(data)
+
+    try:
+        # Validate metadata if present
+        if 'metadata' in pod_config:
+            try:
+                value = InnerResponse(pod_config['metadata'])
+                api_client.deserialize(
+                    value, kubernetes.kubernetes.client.V1ObjectMeta)
+            except ValueError as e:
+                errors.append(f'Invalid metadata: {str(e)}')
+        # Validate spec if present
+        if 'spec' in pod_config:
+            try:
+                value = InnerResponse(pod_config['spec'])
+                api_client.deserialize(value,
+                                       kubernetes.kubernetes.client.V1PodSpec)
+            except ValueError as e:
+                errors.append(f'Invalid spec: {str(e)}')
+        return len(errors) == 0, '.'.join(errors)
+    except Exception as e:  # pylint: disable=broad-except
+        errors.append(f'Validation error: {str(e)}')
+        return False, '.'.join(errors)
+
+
 def is_kubeconfig_exec_auth(
         context: Optional[str] = None) -> Tuple[bool, Optional[str]]:
     """Checks if the kubeconfig file uses exec-based authentication


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 5789214dc..d3eaeb261 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -7,6 +7,7 @@ import pytest
 
 import sky
 from sky import skypilot_config
+import sky.exceptions
 from sky.skylet import constants
 from sky.utils import common_utils
 from sky.utils import kubernetes_enums
@@ -99,6 +100,29 @@ def _create_task_yaml_file(task_file_path: pathlib.Path) -> None:
         """))
 
 
+def _create_invalid_config_yaml_file(task_file_path: pathlib.Path) -> None:
+    task_file_path.write_text(
+        textwrap.dedent("""\
+        experimental:
+            config_overrides:
+                kubernetes:
+                    pod_config:
+                        metadata:
+                            labels:
+                                test-key: test-value
+                            annotations:
+                                abc: def
+                        spec:
+                            containers:
+                                - name:
+                            imagePullSecrets:
+                                - name: my-secret-2
+
+        setup: echo 'Setting up...'
+        run: echo 'Running...'
+        """))
+
+
 def test_nested_config(monkeypatch) -> None:
     """Test that the nested config works."""
     config = skypilot_config.Config()
@@ -335,6 +359,28 @@ def test_k8s_config_with_override(monkeypatch, tmp_path,
     assert cluster_pod_config['spec']['runtimeClassName'] == 'nvidia'
 
 
+def test_k8s_config_with_invalid_config(monkeypatch, tmp_path,
+                                        enable_all_clouds) -> None:
+    config_path = tmp_path / 'config.yaml'
+    _create_config_file(config_path)
+    monkeypatch.setattr(skypilot_config, 'CONFIG_PATH', config_path)
+
+    _reload_config()
+    task_path = tmp_path / 'task.yaml'
+    _create_invalid_config_yaml_file(task_path)
+    task = sky.Task.from_yaml(task_path)
+
+    # Test Kubernetes pod_config invalid
+    cluster_name = 'test_k8s_config_with_invalid_config'
+    task.set_resources_override({'cloud': sky.Kubernetes()})
+    exception_occurred = False
+    try:
+        sky.launch(task, cluster_name=cluster_name, dryrun=True)
+    except sky.exceptions.ResourcesUnavailableError:
+        exception_occurred = True
+    assert exception_occurred
+
+
 def test_gcp_config_with_override(monkeypatch, tmp_path,
                                   enable_all_clouds) -> None:
     config_path = tmp_path / 'config.yaml'