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

[Docs] Multiple k8s support #4586

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

[Docs] Multiple k8s support #4586

wants to merge 19 commits into from

Conversation

Michaelvll
Copy link
Collaborator

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

@Michaelvll
Copy link
Collaborator Author

Added the figure. Ready for review @romilbhardwaj.

@Michaelvll
Copy link
Collaborator Author

Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

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

Thanks @Michaelvll! This is great, left some comments.

docs/source/docs/index.rst Outdated Show resolved Hide resolved
docs/source/images/multi-kubernetes.svg Outdated Show resolved Hide resolved
docs/source/reference/kubernetes/multi-kubernetes.rst Outdated Show resolved Hide resolved

In this example, we have two Kubernetes clusters: ``my-h100-cluster`` and ``my-tpu-cluster``, and each Kubernetes cluster has a context for it.

Point to a Kubernetes Cluster and Launch
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to have a section on adding contexts to allowed_contexts". If allowed_contexts is not set, I believe --region will not work, we will only use the active context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to clearly mark section titles:

Step 1 - Set Up Credentials for Multiple Kubernetes Clusters

Step 2 - Configure SkyPilot to access multiple clusters

Then have sections on launching, show-gpus, failover etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As the final check, we have the users run sky check kubernetes as a way to verify which contexts are available to SkyPilot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Updated the doc. PTAL.

docs/source/reference/kubernetes/multi-kubernetes.rst Outdated Show resolved Hide resolved
docs/source/reference/kubernetes/multi-kubernetes.rst Outdated Show resolved Hide resolved
docs/source/reference/kubernetes/multi-kubernetes.rst Outdated Show resolved Hide resolved
docs/source/reference/kubernetes/multi-kubernetes.rst Outdated Show resolved Hide resolved
@@ -245,10 +245,10 @@ def _format_enabled_cloud(cloud_name: str) -> str:
# here we are using rich. We should migrate this file to
# use colorama as we do in the rest of the codebase.
symbol = ('└── ' if i == len(existing_contexts) - 1 else '├── ')
contexts_formatted.append(f'\n {symbol}{context}')
contexts_formatted.append(f'\n {symbol}{context}')
Copy link
Collaborator Author

@Michaelvll Michaelvll Jan 31, 2025

Choose a reason for hiding this comment

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

Reduced the indent for cleaner view, cc'ing @romilbhardwaj : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants