Skip to content

[k8s] Add podip mode support for exposing ports #3445

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

Merged
merged 4 commits into from
May 1, 2024
Merged

Conversation

romilbhardwaj
Copy link
Collaborator

Adds a podip mode for exposing ports on Kubernetes clusters. This is useful when the ports need to only be internally accessible from within the Kubernetes cluster and user wants to reduce the number of svcs running.

Since ports exposed using podip are not accessible from outside the cluster, we are not adding this mode to our main docs for now. Only covered in the config.yaml documentation, can add to main docs based on demand.

Added after feedback from user.

Tested manually with sky launch -c test2 --cloud kubernetes --ports 8888 -- 'python -m http.server 8888' -> sky status --endpoints test2 -> Access endpoint from within cluster.

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 this @romilbhardwaj! LGTM.

Comment on lines 1279 to 1291
def get_pod_name(cluster_name_on_cloud: str):
"""Returns the pod name of the head pod for the given cluster name on cloud

Args:
cluster_name_on_cloud: Name of the cluster on cloud

Returns:
str: Pod name of the head pod
"""
# We could have iterated over all pods in the namespace and checked for the
# label, but since we know the naming convention, we can directly return the
# head pod name.
return f'{cluster_name_on_cloud}-head'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a very shallow function and is only used once. Should we just have '{cluster_name_on_cloud}-head' in the place we call this function?

If we really want to keep this function, it might be better to rename it to get_head_pod_name instead, otherwise, it is a bit confusing for the multi-node case

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 - I've refactored to get_head_pod_name.

@romilbhardwaj romilbhardwaj merged commit 49dcc33 into master May 1, 2024
20 checks passed
@romilbhardwaj romilbhardwaj deleted the k8s_podip branch May 1, 2024 21:15
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