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.

Sign up for GitHub

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] Add validation for pod_config #4206 #4466

Merged
merged 13 commits into from
Jan 3, 2025
Merged

[k8s] Add validation for pod_config #4206 #4466

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
12f1208
[k8s] Add validation for pod_config #4206
chesterli29 Dec 12, 2024
64bb66a
update: check pod_config when launch
chesterli29 Dec 13, 2024
a0f29e5
fix test
chesterli29 Dec 13, 2024
47e724d
ignore check failed when test with dryrun
chesterli29 Dec 13, 2024
699961d
use deserialize api to check pod_config schema
chesterli29 Dec 25, 2024
164f436
test
chesterli29 Dec 26, 2024
0471b4c
create another api_client with no kubeconfig
chesterli29 Dec 26, 2024
98a1d84
test
chesterli29 Dec 26, 2024
7e501f3
update error message
chesterli29 Dec 26, 2024
ef562da
update test
chesterli29 Dec 31, 2024
2b5dba1
test
chesterli29 Dec 31, 2024
81029d7
test
chesterli29 Dec 31, 2024
d349c37
Update sky/backends/backend_utils.py
romilbhardwaj Jan 3, 2025
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
43 changes: 43 additions & 0 deletions sky/provision/kubernetes/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,15 @@ def check_credentials(context: Optional[str],

_, exec_msg = is_kubeconfig_exec_auth(context)

# Check whether pod_config is valid
pod_config = skypilot_config.get_nested(('kubernetes', 'pod_config'),
default_value={},
override_configs={})
if pod_config:
_, pod_msg = _check_pod_config(context, pod_config)
if pod_msg:
return False, pod_msg
chesterli29 marked this conversation as resolved.
Show resolved Hide resolved

# We now check if GPUs are available and labels are set correctly on the
# cluster, and if not we return hints that may help debug any issues.
# This early check avoids later surprises for user when they try to run
Expand All @@ -891,6 +900,40 @@ def check_credentials(context: Optional[str],
else:
return True, None

def _check_pod_config(
context: Optional[str] = None, pod_config: Optional[Any] = None) \
-> Tuple[bool, Optional[str]]:
"""Check if the pod_config is a valid pod config

Using create_namespaced_pod api with dry_run 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.
"""
try:
namespace = get_kube_config_context_namespace(context)
kubernetes.core_api(context).create_namespaced_pod(
namespace,
body=pod_config,
dry_run='All',
field_validation='Strict',
_request_timeout=kubernetes.API_TIMEOUT)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

@chesterli29 chesterli29 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@chesterli29 chesterli29 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
image
and if we use this config to create sky cluster, it will fail later because the invalid name.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

except kubernetes.api_exception() as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

error_msg = ''
if e.body:
# get detail error message from api_exception
exception_body = json.loads(e.body)
error_msg = exception_body.get('message')
else:
error_msg = str(e)
return False, f'Invalid pod_config: {error_msg}'
except Exception as e: # pylint: disable=broad-except
return False, ('An error occurred: '
f'{common_utils.format_exception(e, use_bracket=True)}')
return True, None


def is_kubeconfig_exec_auth(
context: Optional[str] = None) -> Tuple[bool, Optional[str]]:
Expand Down
Loading