[k8s] Add validation for pod_config #4206 #4466

chesterli29 · 2024-12-12T03:36:25Z

Check pod_config when run 'sky check k8s' by using k8s #4206

This commit extends the functionality of sky check k8s by adding a check for pod_config in this step. The method used to check pod_config is by calling the K8s API. This approach has some advantages and disadvantages:

Advantages:

Minimal code changes and 100% reliable check results.
Future K8s extensions to the pod format will not require any adjustments to skypilot's code.

Disadvantages:

Requires interaction with the K8s cluster and cannot be validated locally offline.

Of course, any other suggestions are welcome for discussion.

The test config.yaml

kubernetes:
  pod_config:
    metadata:
      name: local-test
      labels:
        my-label: my-value    # Custom labels to SkyPilot pods
    spec:
      #runtimeClassName: nvidia    # Custom runtimeClassName for GPU pods.
      imagePullSecrets:
        - name: my-secret     # Pull images from a private registry using a secret
      containers:
        - name: local_test
          image: test
          env:                # Custom environment variables for the pod, e.g., for proxy
          - name: HTTP_PROXY
            value: http://proxy-host:3128
          volumeMounts:       # Custom volume mounts for the pod
            - mountPath: /foo
              name: example-volume
              readOnly: true
      volumes:
        - name: example-volume
          hostPath:
            path: /tmp
            type: Directory
        - name: dshm          # Use this to modify the /dev/shm volume mounted by SkyPilot
          emptyDir:
            medium: Memory
            sizeLimit: 3Gi    # Set a size limit for the /dev/shm volume

And the Check Result:

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

Check pod_config when run 'sky check k8s' by using k8s api

romilbhardwaj

Thanks @chesterli29! Left some questions. We may need to use an alternate approach since pod validation from k8s API server may be too strict.

sky/provision/kubernetes/utils.py

romilbhardwaj · 2024-12-12T05:31:38Z

sky/provision/kubernetes/utils.py

+        kubernetes.core_api(context).create_namespaced_pod(
+            namespace,
+            body=pod_config,
+            dry_run='All',
+            field_validation='Strict',
+            _request_timeout=kubernetes.API_TIMEOUT)


Does this approach work even if the pod_config is partially specified? E.g.,

kubernetes: pod_config: spec: containers: - env: - name: MY_ENV_VAR value: "my_value"

My hunch is k8s will reject this pod spec since it's not a complete pod spec, but it's a valid pod_config in our case.

yes, the k8s will reject this pod spec.
if this pod_config is valid in this project. is there any definition about this config? for example: some filed is required or optional? or all the filed is optional here, but it must follow the k8s pod require only if it has been set ?

here is my solution about this, we can check the pod config by using k8s api after combine_pod_config_fields and combine_metadata_fields during launch (that is the early stage of launching.).
it's really hard and complex to follow and maintain the k8s pod json/yaml schema in this project.

all the filed is optional here, but it must follow the k8s pod require only if it has been set ?

Yes, this is the definition of a valid pod_spec.

can check the pod config by using k8s api after combine_pod_config_fields and combine_metadata_fields during launch (that is the early stage of launching.)

Yes, that sounds reasonable as long as we can surface to the user where the error comes in the user's pod config.

Have we considered having a simple local schema check, with the json schema fetched and flattened from something like https://github.com/instrumenta/kubernetes-json-schema/tree/master?

Have we considered having a simple local schema check, with the json schema fetched and flattened from something like https://github.com/instrumenta/kubernetes-json-schema/tree/master?

Yeah, I took a look at this before. The main problem with this setup is that it needs to grab JSON schema files from other repo eg: https://github.com/yannh/kubernetes-json-schema, depending on which version of k8s user using. I'm not sure if it's a good idea for sky to download dependencies to the local machine while it's running. Plus, if we want to check pod_config locally using JSON schema, we might need to let users choose their k8s version so we can get the right schema file.

Let's try the approach you proposed above (check the pod config by using k8s api after combine_pod_config_fields and combine_metadata_fields) if it can surface the exact errors to the users.

If that does not work, we may need to do schema validation locally. Pod API has been relatively stable, so might not be too bad to have a fixed version schema for validation.

LGTM,
BTW i found a error case when i test the approach with json schema in kubernetes-json-schema.
here is my part of test yaml

containers: - name: local_test image: test

note, the name here local_test with _ inside, it's invalid when we creating a pod, but will pass the check by json schema.

and if we use this config to create sky cluster, it will fail later because the invalid name.

FYI. Here is the output after pod_config check failed during launch

check merged pod_config during launch using k8s api

if there is no kube config in env, ignore ValueError when launch with dryrun. For now, we don't support check schema offline.

chesterli29 · 2024-12-17T08:34:36Z

The approach has been adjusted. it will check the pod_config using the k8s API at each launch after combine_metadata_fields. Using create_namespace_pod with dryrun and non-static, so it allows users to input fields that are not part of the pod definition.

romilbhardwaj · 2024-12-19T05:52:22Z

sky/backends/backend_utils.py

+            tmp_yaml_path, dryrun)
+        if not valid:
+            raise exceptions.InvalidCloudConfigs(
+                f'There are invalid config in pod_config, deatil: {message}')


Suggested change

f'There are invalid config in pod_config, deatil: {message}')

f'Invalid pod_config. Details: {message}')

romilbhardwaj · 2024-12-19T05:57:49Z

sky/provision/kubernetes/utils.py

+            body=pod_config,
+            dry_run='All',
+            _request_timeout=kubernetes.API_TIMEOUT)
+    except kubernetes.api_exception() as e:


Can kubernetes.api_exception() be caused by reasons other than those not related to invalid config (e.g., insufficient permissions)? In that case, the error message is misleading. For example, I ran into this:

W 12-18 21:53:35 cloud_vm_ray_backend.py:2065] sky.exceptions.ResourcesUnavailableError: Failed to provision on cloud Kubernetes due to invalid cloud config: sky.exceptions.InvalidCloudConfigs: There are invalid config in pod_config, deatil: pods "Unknown" is forbidden: error looking up service account default/skypilot-service-account: serviceaccount "skypilot-service-account" not found

Can we filter the exception further and return valid = False only if the failure is due to invalid pod schema?

If it's hard to filter, here is an alternative implementation (not sure if it works, needs testing):

from typing import Any, Dict, List, Optional from kubernetes import client from kubernetes.client.api_client import ApiClient def validate_pod_config(pod_config: Dict[str, Any]) -> List[str]: """Validates a pod_config dictionary against Kubernetes schema. Args: pod_config: Dictionary containing pod configuration Returns: List of validation error messages. Empty list if validation passes. """ errors = [] # Create API client for schema validation api_client = ApiClient() try: # The pod_config can contain metadata and spec sections allowed_top_level = {'metadata', 'spec'} unknown_fields = set(pod_config.keys()) - allowed_top_level if unknown_fields: errors.append(f'Unknown top-level fields in pod_config: {unknown_fields}') # Validate metadata if present if 'metadata' in pod_config: try: api_client.sanitize_for_serialization( client.V1ObjectMeta(**pod_config['metadata']) ) except (ValueError, TypeError) as e: errors.append(f'Invalid metadata: {str(e)}') # Validate spec if present if 'spec' in pod_config: try: api_client.sanitize_for_serialization( client.V1PodSpec(**pod_config['spec']) ) except (ValueError, TypeError) as e: errors.append(f'Invalid spec: {str(e)}') except Exception as e: errors.append(f'Validation error: {str(e)}') return errors

Thanks ! If sanitize_for_serialization works, i think this approach is much better than create with dryrun.

romilbhardwaj · 2024-12-19T06:05:02Z

sky/backends/backend_utils.py

+        valid, message = kubernetes_utils.check_pod_config(
+            tmp_yaml_path, dryrun)


Running this on a fresh kubernetes cluster which does not already contain skypilot-service-account fails:

W 12-18 22:00:56 cloud_vm_ray_backend.py:2065] sky.exceptions.ResourcesUnavailableError: Failed to provision on cloud Kubernetes due to invalid cloud config: sky.exceptions.InvalidCloudConfigs: There are invalid config in pod_config, deatil: pods "Unknown" is forbidden: error looking up service account default/skypilot-service-account: serviceaccount "skypilot-service-account" not found

Note that this service account is created in our downstream provisioning logic in config.py before the pod is provisioned. We may want to move this check there.

romilbhardwaj · 2024-12-19T06:07:32Z

sky/provision/kubernetes/utils.py

+        if dryrun:
+            logger.debug('ignore ValueError as there is no kube config '
+                         'in the enviroment with dry_run. '
+                         'For now we don\'t support check pod_config offline.')
+            return True, None
+        return False, common_utils.format_exception(e)


This dry run case might not be required if we move the call to check_pod_config to our downstream logic in provision/kubernetes/instance.py or provision/kubernetes/config.py

romilbhardwaj · 2024-12-19T06:10:33Z

sky/provision/kubernetes/utils.py

@@ -892,6 +892,53 @@ def check_credentials(context: Optional[str],
        return True, None


+def check_pod_config(cluster_yaml_path: str, dryrun: bool) \


For reusability as a general method, we may want to use pod_config dict or V1Pod object as the arg

Suggested change

def check_pod_config(cluster_yaml_path: str, dryrun: bool) \

def check_pod_config(pod_config: Dict)

romilbhardwaj · 2024-12-19T06:13:37Z

sky/provision/kubernetes/utils.py

+        kubernetes.core_api(context).create_namespaced_pod(
+            namespace,
+            body=pod_config,
+            dry_run='All',
+            _request_timeout=kubernetes.API_TIMEOUT)


BTW, if we are going for this approach, is there any advantage to doing it here vs directly catching and raising errors when we do the actual create_namespaced_pod call here:

skypilot/sky/provision/kubernetes/instance.py

Lines 585 to 586 in 745cf59

pod = kubernetes.core_api(context).create_namespaced_pod(

namespace, pod_spec)

From the results, they should be the same. However, just like we do validation for other configs, we want to expose potential configuration issues at an earlier stage.

romilbhardwaj

This is great, thanks @chesterli29! Some minor comments, otherwise good to go.

romilbhardwaj · 2024-12-26T23:30:44Z

sky/backends/backend_utils.py

+        valid, message = kubernetes_utils.check_pod_config(pod_config)
+        if not valid:
+            raise exceptions.InvalidCloudConfigs(
+                f'Invalid pod_config. Deatil: {message}')


Suggested change

f'Invalid pod_config. Deatil: {message}')

f'Invalid pod_config. Details: {message}')

romilbhardwaj · 2024-12-26T23:40:28Z

tests/test_config.py

+                kubernetes:
+                    pod_config:
+                        metadata:
+                            labels:
+                                test-key: test-value
+                            annotations:
+                                abc: def
+                        spec:
+                            containers:
+                                - name:
+                            imagePullSecrets:
+                                - name: my-secret-2


Should we retain just the relevant kubernetes field and remove the docker, gcp, nvidia_gpus and other fields?

Also can we put a quick comment on what's the invalid field here?

Well, the deserialize api will ignore other invalid field here, as we can see the implement in the https://github.com/kubernetes-client/python/blob/e10470291526c82f12a0a3405910ccc3f3cdeb26/kubernetes/client/api_client.py#L620

for attr, attr_type in six.iteritems(klass.openapi_types): if klass.attribute_map[attr] in data: value = data[klass.attribute_map[attr]] kwargs[attr] = self.__deserialize(value, attr_type)

Ah I meant for readability in tests, let's have only this:

experimental: config_overrides: kubernetes: pod_config: metadata: labels: test-key: test-value annotations: abc: def spec: containers: - name: imagePullSecrets: - name: my-secret-2

romilbhardwaj · 2024-12-27T00:00:05Z

tests/test_config.py

+    except sky.exceptions.ResourcesUnavailableError:
+        exception_occurred = True


Should we also verify the error message?

I tried to validate this error message, but it's meaningless for this test because the error message returned by _provision in the end is not directly related to the actual error.

[k8s] Add validation for pod_config skypilot-org#4206

12f1208

Check pod_config when run 'sky check k8s' by using k8s api

chesterli29 force-pushed the issue_4206 branch from e994181 to 12f1208 Compare December 12, 2024 03:41

Michaelvll requested a review from romilbhardwaj December 12, 2024 03:45

romilbhardwaj reviewed Dec 12, 2024

View reviewed changes

chesterli29 added 3 commits December 13, 2024 16:42

update: check pod_config when launch

64bb66a

check merged pod_config during launch using k8s api

fix test

a0f29e5

ignore check failed when test with dryrun

47e724d

if there is no kube config in env, ignore ValueError when launch with dryrun. For now, we don't support check schema offline.

romilbhardwaj reviewed Dec 19, 2024

View reviewed changes

chesterli29 added 5 commits December 25, 2024 12:36

use deserialize api to check pod_config schema

699961d

test

164f436

create another api_client with no kubeconfig

0471b4c

test

98a1d84

update error message

7e501f3

romilbhardwaj reviewed Dec 27, 2024

View reviewed changes

chesterli29 added 3 commits December 31, 2024 10:32

update test

ef562da

test

2b5dba1

test

81029d7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[k8s] Add validation for pod_config #4206 #4466

[k8s] Add validation for pod_config #4206 #4466

chesterli29 commented Dec 12, 2024

romilbhardwaj left a comment

romilbhardwaj Dec 12, 2024

chesterli29 Dec 12, 2024

chesterli29 Dec 12, 2024 •

edited

Loading

romilbhardwaj Dec 12, 2024

romilbhardwaj Dec 12, 2024

chesterli29 Dec 13, 2024 •

edited

Loading

romilbhardwaj Dec 13, 2024

chesterli29 Dec 13, 2024

chesterli29 Dec 13, 2024

chesterli29 commented Dec 17, 2024

romilbhardwaj Dec 19, 2024

romilbhardwaj Dec 19, 2024

romilbhardwaj Dec 19, 2024

chesterli29 Dec 20, 2024

romilbhardwaj Dec 19, 2024

romilbhardwaj Dec 19, 2024

romilbhardwaj Dec 19, 2024

romilbhardwaj Dec 19, 2024

chesterli29 Dec 20, 2024

romilbhardwaj left a comment

romilbhardwaj Dec 26, 2024

romilbhardwaj Dec 26, 2024

romilbhardwaj Dec 26, 2024

chesterli29 Dec 27, 2024

romilbhardwaj Dec 27, 2024

romilbhardwaj Dec 27, 2024

chesterli29 Dec 31, 2024

	f'There are invalid config in pod_config, deatil: {message}')
	f'Invalid pod_config. Details: {message}')

		valid, message = kubernetes_utils.check_pod_config(
		tmp_yaml_path, dryrun)

		@@ -892,6 +892,53 @@ def check_credentials(context: Optional[str],
		return True, None


		def check_pod_config(cluster_yaml_path: str, dryrun: bool) \

	def check_pod_config(cluster_yaml_path: str, dryrun: bool) \
	def check_pod_config(pod_config: Dict)

	pod = kubernetes.core_api(context).create_namespaced_pod(
	namespace, pod_spec)

	f'Invalid pod_config. Deatil: {message}')
	f'Invalid pod_config. Details: {message}')

		except sky.exceptions.ResourcesUnavailableError:
		exception_occurred = True

[k8s] Add validation for pod_config #4206 #4466

Are you sure you want to change the base?

[k8s] Add validation for pod_config #4206 #4466

Conversation

chesterli29 commented Dec 12, 2024

romilbhardwaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesterli29 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesterli29 Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesterli29 commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romilbhardwaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesterli29 Dec 12, 2024 •

edited

Loading

chesterli29 Dec 13, 2024 •

edited

Loading