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

Add docs for opening ports on Kubernetes #2997

Merged
merged 5 commits into from
Feb 7, 2024

Conversation

hemildesai
Copy link
Contributor

Add docs for #2713

hemildesai and others added 4 commits January 18, 2024 00:01
…o hemil/k8s-ports-docs

# Conflicts:
#	docs/source/reference/config.rst
#	docs/source/reference/kubernetes/index.rst
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 @hemildesai! I've reworked the docs to focus on how to get configured quickly with direct instructions - let me know what you think!

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for adding the doc @hemildesai @romilbhardwaj! Left several minor comments.

docs/source/reference/config.rst Show resolved Hide resolved
docs/source/reference/config.rst Outdated Show resolved Hide resolved
# deploying the NGINX ingress controller. In this mode, the port can be
# accessed externally using the Ingress URL plus a path prefix of the form
# /skypilot/{cluster_pod_name}/{port}
# Refer to kubernetes-ingress.yml.j2 for the exact template.
Copy link
Collaborator

Choose a reason for hiding this comment

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

At the first glance, I thought I should fill out the template myself. Maybe we can say: "SkyPilot uses the config in sky/templates/kubernetes-ingress.yml.j2 to setup the ingress"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I think this was a little too much detail, and the user should likely not look/edit at this j2 unless they really know what they are doing. I have removed this for now.

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 for the review @Michaelvll! PTAL : )

# deploying the NGINX ingress controller. In this mode, the port can be
# accessed externally using the Ingress URL plus a path prefix of the form
# /skypilot/{cluster_pod_name}/{port}
# Refer to kubernetes-ingress.yml.j2 for the exact template.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! I think this was a little too much detail, and the user should likely not look/edit at this j2 unless they really know what they are doing. I have removed this for now.

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for the update! LGTM.

docs/source/reference/config.rst Show resolved Hide resolved
To work around this issue, make sure all your ports have services running behind them.

.. note::
LoadBalancer services are not supported on kind clusters created using :code:`sky local up`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a quick thought, maybe we can do it in the future, but is it possible that we can have sky local up with the nginx ingress setup by default, so the user can directly try the open ports on the cluster that is local up'ed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea! tracking here - #3112 : )

@romilbhardwaj romilbhardwaj merged commit d795a68 into skypilot-org:master Feb 7, 2024
19 checks passed
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.

None yet

3 participants