[API server] use loadbalancer to expose ingress by default #4926

aylei · 2025-03-11T04:49:28Z

Motivation of changing the defaults: LoadBalancer (if available) is more stable and secure compared to NodePort, correspondingly better UX and lower support costs.

Note: LoadBalancer might be unavailable on some k8s infra, we still make it default because it is the de-facto way to expose L4 endpoint. As an example, nginx-ingress also use LoadBalancer as default service type https://artifacthub.io/packages/helm/ingress-nginx/ingress-nginx

Edit

New behavior for users upgraded from earlier 0.8.0-nightly versions, an error will be raised:

$ helm upgrade --install skypilot-alpha ./charts/skypilot -f values.yaml
Error: UPGRADE FAILED: execution error at (skypilot/templates/ingress-nodeport.yaml:8:8): Service skypilot-alpha-ingress-controller-np is deprecated and will be removed in SkyPilot v0.9.0. Refer to https://docs.skypilot.co/en/latest/reference/api-server/api-server-admin-deploy.html#migrate-from-legacy-nodeport-service for migration steps.

This ensures the warning is acknowledged by users.

Tested (run the relevant ones):

Code formatting: install pre-commit (auto-check on commit) or bash format.sh
Manually Tested on GKE
Manually Tested on EKS
Manually Tested on AKS
All smoke tests: /smoke-test (CI) or pytest tests/test_smoke.py (local)
Relevant individual tests: /smoke-test -k test_name (CI) or pytest tests/test_smoke.py::test_name (local)
Backward compatibility: /quicktest-core (CI) or conda deactivate; bash -i tests/backward_compatibility_tests.sh (local)

Signed-off-by: Aylei <[email protected]>

romilbhardwaj

Awesome, thanks @aylei!

docs/source/reference/api-server/api-server-admin-deploy.rst

romilbhardwaj · 2025-03-12T00:26:10Z

docs/source/reference/api-server/api-server-admin-deploy.rst

+        helm upgrade --install -n $NAMESPACE $RELEASE_NAME skypilot/skypilot-nightly --devel \
+            --set ingress.nodePortEnabled=false


Will this also delete the old NodePort service?

Yes, this command is solely for deleting the old NodePort service

romilbhardwaj · 2025-03-12T00:31:27Z

charts/skypilot/values.yaml

-      # Use ClusterIP here to disable the LoadBalancer created by nginx. 
-      # If not using NodePort, set this to LoadBalancer.
-      type: ClusterIP  
+      type: LoadBalancer


IIUC, in the future this can directly be set to NodePort for users who want to use nodeport? If so, we should probably remove the ingress.nodePortEnabled right before 0.9.0 release.

Same for charts/skypilot/templates/ingress-nodeport.yaml - that file should be removed before 0.9.0. Good to have a TODO there.

Yes, I think setting ingress-nginx.controller.service.* is recommended after this PR for new deployments. But it requires the similar migration steps for legacy deployments to abandon the additional node port service:

Enable the nginx NodePort service, since 30050 and 30051 have been used, the new service should pick up new ports;

Ask clients to migrate to the new server URL (with new ports)

Set ingress.nodePortEnabled=false to delete the old NodePort service

Not sure whether we should keep the additional node port service to simplify upgrades for nightly users, but we can document the above steps and leave the choice to users. It is reasonable to take no action if they are happy with current setup.

A new idea is that we can just error out to user if:

There is an existing legacy node port service

and ingress.nodePortEnabled is not set

Suggested message: "Service ${serviceName} is deprecated and will be removed in 0.9.0, follow the guide to migrate: ${URL}", in the guide we suggest user to set ingress.nodePortEnabled=true explicitly first and gradually migrate their users to service managed by ingress-nginx.controller.service.* and finally set ingress.nodePortEnabled=false.

Also the message should be updated in 0.9.0 release: "Service ${serviceName} is deprecated and was removed in 0.9.0, migrate the service using the previous helm chart version before upgrade."

This will add some friction to our nightly users that have an API sever chart deployed, but should be okay since the adoption is not that wide.

@romilbhardwaj wdyt?

This proposal sounds good to me - essentially we require current users to be explicit about their intent to use NodePort after this PR.

charts/skypilot/values.yaml

Co-authored-by: Romil Bhardwaj <[email protected]>

Signed-off-by: Aylei <[email protected]>

aylei · 2025-03-21T01:41:15Z

@romilbhardwaj this PR is ready for another look

romilbhardwaj

Thanks @aylei!

romilbhardwaj · 2025-03-21T06:49:36Z

docs/source/reference/api-server/api-server-admin-deploy.rst

-            $ NODE_PORT=$(kubectl get svc ${RELEASE_NAME}-ingress-controller-np -n $NAMESPACE -o jsonpath='{.spec.ports[?(@.name=="http")].nodePort}')
-            $ NODE_IP=$(kubectl get nodes -o jsonpath='{ $.items[0].status.addresses[?(@.type=="ExternalIP")].address }')
-            $ ENDPOINT=http://${WEB_USERNAME}:${WEB_PASSWORD}@${NODE_IP}:${NODE_PORT}
+            $ ENDPOINT=$(kubectl get svc ${RELEASE_NAME}-ingress-nginx-controller -n $NAMESPACE -o jsonpath='http://{.status.loadBalancer.ingress[0].ip}')


We should probably retain the ENDPOINT=http://${WEB_USERNAME}:${WEB_PASSWORD}@${NODE_IP} format so that the output is printed as http://skypilot:[email protected] and users can directly copy-paste the endpoint in their config.yaml.

Great idea!

romilbhardwaj · 2025-03-21T07:02:50Z

docs/source/reference/api-server/api-server-admin-deploy.rst

-        :sync: nodeport-tab
-
-        1. Make sure ports 30050 and 30051 are open on your nodes.
+    .. tab-item:: LoadBalancer (Default)


Should we also change the "A Kubernetes cluster with ports 30050 and 30051 available for NodePort services" prerequisite at L15 to "A Kubernetes cluster with LoadBalancer or NodePort service support"?

Good catch!

Signed-off-by: Aylei <[email protected]>

aylei added 3 commits March 10, 2025 20:11

[API server] use loadbalancer to expose ingress by default

cc9f5c6

Signed-off-by: Aylei <[email protected]>

update

3491372

Signed-off-by: Aylei <[email protected]>

compatbility refine

ebaacc9

Signed-off-by: Aylei <[email protected]>

aylei requested review from romilbhardwaj and Michaelvll March 11, 2025 04:49

aylei marked this pull request as ready for review March 11, 2025 04:49

aylei added 2 commits March 11, 2025 12:56

fix doc

fb65b8e

Signed-off-by: Aylei <[email protected]>

refine docs

8c103e1

Signed-off-by: Aylei <[email protected]>

romilbhardwaj reviewed Mar 12, 2025

View reviewed changes

aylei and others added 2 commits March 12, 2025 09:24

Update docs/source/reference/api-server/api-server-admin-deploy.rst

bf98e6e

Co-authored-by: Romil Bhardwaj <[email protected]>

Resolve merge conflicts

c1179c7

Signed-off-by: Aylei <[email protected]>

aylei requested a review from romilbhardwaj March 20, 2025 02:31

aylei force-pushed the aylei/apiserver-lb branch from 0111eef to f06c659 Compare March 20, 2025 08:20

Refine deprecation policy

cd8ee18

Signed-off-by: Aylei <[email protected]>

aylei force-pushed the aylei/apiserver-lb branch from f06c659 to cd8ee18 Compare March 20, 2025 08:30

aylei added 2 commits March 20, 2025 16:43

Merge branch 'master' into aylei/apiserver-lb

36c521a

Address doc lint issue

39d8a8c

Signed-off-by: Aylei <[email protected]>

romilbhardwaj approved these changes Mar 21, 2025

View reviewed changes

Address review comments

172f8cf

Signed-off-by: Aylei <[email protected]>

aylei merged commit 052e0ea into master Mar 25, 2025
18 checks passed

aylei deleted the aylei/apiserver-lb branch March 25, 2025 01:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API server] use loadbalancer to expose ingress by default #4926

[API server] use loadbalancer to expose ingress by default #4926

aylei commented Mar 11, 2025 •

edited

Loading

romilbhardwaj left a comment

romilbhardwaj Mar 12, 2025

aylei Mar 12, 2025

romilbhardwaj Mar 12, 2025

romilbhardwaj Mar 12, 2025

aylei Mar 12, 2025 •

edited

Loading

aylei Mar 20, 2025 •

edited

Loading

romilbhardwaj Mar 21, 2025

aylei commented Mar 21, 2025

romilbhardwaj left a comment

romilbhardwaj Mar 21, 2025

aylei Mar 24, 2025

romilbhardwaj Mar 21, 2025

aylei Mar 24, 2025

		helm upgrade --install -n $NAMESPACE $RELEASE_NAME skypilot/skypilot-nightly --devel \
		--set ingress.nodePortEnabled=false

[API server] use loadbalancer to expose ingress by default #4926

[API server] use loadbalancer to expose ingress by default #4926

Conversation

aylei commented Mar 11, 2025 • edited Loading

Edit

romilbhardwaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

aylei Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei commented Mar 21, 2025

romilbhardwaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aylei commented Mar 11, 2025 •

edited

Loading

aylei Mar 12, 2025 •

edited

Loading

aylei Mar 20, 2025 •

edited

Loading