feat(k8s): support multi-container (sidecar) SkyPilot pods#8444
Conversation
Summary of ChangesHello @php-workx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances SkyPilot's compatibility with Kubernetes environments that utilize multi-container pods, particularly those where sidecars are automatically injected. By introducing explicit targeting of the primary workload container, SkyPilot can now reliably execute commands and gather resource information, preventing operational disruptions and ensuring correct behavior in complex Kubernetes deployments. This change improves the robustness and flexibility of SkyPilot's Kubernetes provisioning capabilities without affecting existing single-container setups. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces explicit targeting of the primary workload container, named 'ray-node', within Kubernetes pods to better support multi-container deployments. Changes include adding a RAY_NODE_CONTAINER_NAME constant, modifying KubernetesCommandRunner to accept a container argument for kubectl exec and rsync operations, and implementing a get_pod_primary_container utility function. This utility is used across the codebase to ensure pre-initialization, AppArmor annotation removal, resource parsing (CPU, memory, GPU), and command execution correctly interact with the intended container. The previous restriction of single-container deployments has been removed. A review comment suggests a more concise Pythonic approach for retrieving CPU requests using dictionary get with or operators.
|
/smoke-test |
aylei
left a comment
There was a problem hiding this comment.
Thanks for adding this @php-workx ! Could you include a smoke test with sidecar injected to make this PR self-contained?
Signed-off-by: Aylei <rayingecho@gmail.com>
|
/smoke-test -k test_kubernetes_pod_config_sidecar |
1 similar comment
|
/smoke-test -k test_kubernetes_pod_config_sidecar |
|
/smoke-test -k test_kubernetes_pod_config_sidecar --kubernetes |
1 similar comment
|
/smoke-test -k test_kubernetes_pod_config_sidecar --kubernetes |
33844cb to
441f613
Compare
Signed-off-by: Aylei <rayingecho@gmail.com>
441f613 to
781ef64
Compare
Signed-off-by: Aylei <rayingecho@gmail.com>
|
/smoke-test -k test_kubernetes_pod_config_sidecar --kubernetes |
|
Oh, seems like there was quite a bit of work left?! Thanks so much for pushing this forward, @aylei ! 🙏🏻 |
|
/smoke-test --kubernetes |
|
/quicktest-core |
SkyPilot’s Kubernetes provisioning and control-plane interactions currently assume SkyPilot pods contain a single container and implicitly target containers[0]. In real clusters, mutating webhooks often inject sidecars (log shippers, mesh proxies, security agents), which breaks this assumption and can cause provisioning failures or commands executing in the wrong container.
This PR adds multi-container awareness by consistently targeting the primary workload container (default:
ray-node) when executing commands or inspecting resources.Key changes
kubectl exectargets the intended container when sidecars are present.ray-node) as a constant:sky/provision/kubernetes/constants.py:RAY_NODE_CONTAINER_NAME.Behavior
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)