[RayService] Support attaching incremental upgrade to an existing Gateway - #5018
[RayService] Support attaching incremental upgrade to an existing Gateway#5018ScraperNerd18 wants to merge 5 commits into
Conversation
…eway
NewClusterWithIncrementalUpgrade always creates its own per-RayService Gateway
(<name>-gateway) and waits for the controller to program it. That fails on
clusters whose Gateway controller only reconciles one specific shared Gateway --
notably Contour in static gateway.gatewayRef mode (Contour v1.30+ removed
controllerName), where KubeRay-created Gateways stay Programmed=Unknown forever
and the traffic shift never starts.
Add ClusterUpgradeOptions.existingGatewayRef {name, namespace}. When set:
- createGateway returns nil so reconcileGateway skips Gateway creation
(KubeRay does not own the shared Gateway; the existing desiredGateway==nil
guard in reconcileGateway already handles this),
- RayServiceGatewayNamespacedName resolves to the referenced Gateway, so the
HTTPRoute's ParentRef and the readiness check target it,
- the HTTPRoute and its backendRefs use the RayService namespace (owner-ref GC
keeps working; equals the Gateway ns in the default case, differs for a
cross-namespace shared Gateway -- which must allow routes from that ns).
gatewayClassName becomes optional and is mutually exclusive with
existingGatewayRef; validation updated accordingly. CRDs + deepcopy updated.
Fully backward compatible: unset existingGatewayRef => previous behavior
(KubeRay creates the per-RayService Gateway from gatewayClassName).
Signed-off-by: ScraperNerd18 <ScraperNerd18@users.noreply.github.com>
…tewayRef Generated by `make generate` after adding the GatewayRef type and ClusterUpgradeOptions.existingGatewayRef field: applyconfiguration builders for GatewayRef/ClusterUpgradeOptions, the ForKind switch entry, and the CRD reference docs. Signed-off-by: ScraperNerd18 <ScraperNerd18@users.noreply.github.com>
Cover the three behaviors of attaching to a pre-existing Gateway:
- ValidateClusterUpgradeOptions: existingGatewayRef valid on its own, mutually
exclusive with gatewayClassName, and requires both name and namespace,
- createGateway returns nil (no Gateway) when existingGatewayRef is set,
- createHTTPRoute targets the referenced Gateway via ParentRef (in the
Gateway's namespace) while the HTTPRoute and its backendRefs stay in the
RayService namespace,
- RayServiceGatewayNamespacedName resolves to the referenced Gateway.
Signed-off-by: ScraperNerd18 <ScraperNerd18@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…GatewayRef is set deleteRayServiceOwnedResources loads the Gateway via RayServiceGatewayNamespacedName and deletes it during suspend. When existingGatewayRef is set that name resolves to the shared Gateway KubeRay does not own, so suspending one RayService would tear down cluster-wide ingress for every other RayService attached to that Gateway. Skip Gateway deletion when existingGatewayRef is set (KubeRay never created it; the reconcileGateway desiredGateway==nil guard already means it was never managed). The KubeRay-owned HTTPRoute in the RayService namespace is still deleted. Add a regression test asserting the shared Gateway is preserved while the HTTPRoute is removed on suspend. Signed-off-by: ScraperNerd18 <ScraperNerd18@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 2b29e6b. Configure here.
The previous fix skipped all Gateway deletion when existingGatewayRef is set. That
left an orphan: a RayService switched from gatewayClassName to existingGatewayRef
still owns its "{name}-gateway" in the RayService namespace, which was then never
removed on suspend (and can back a real cloud load balancer).
Target the KubeRay-owned Gateway name ("{name}-gateway" in the RayService namespace)
explicitly for deletion instead of RayServiceGatewayNamespacedName. This:
- always deletes the per-RayService Gateway KubeRay may own (including the
switch-over orphan),
- never deletes the shared referenced Gateway (a different name/namespace),
- is a no-op (NotFound) for a RayService that only ever used existingGatewayRef.
Extend the regression test to assert the orphaned per-RayService Gateway is deleted
while the shared Gateway is preserved.
Signed-off-by: ScraperNerd18 <ScraperNerd18@users.noreply.github.com>
|
I think it's related to #4956? Do you think the proposal in that issue can also solve the problem on your side? Will find time to look into it |
There was a problem hiding this comment.
Could you also add a example in https://github.com/ray-project/kuberay/blob/master/ray-operator/config/samples/ ?
Also resolve the conflict, thnaks!
|
cc @ryanaoleary to take a look. |
| ObjectMeta: metav1.ObjectMeta{Name: httpRouteName, Namespace: rayServiceInstance.Namespace}, | ||
| Spec: gwv1.HTTPRouteSpec{ | ||
| CommonRouteSpec: gwv1.CommonRouteSpec{ | ||
| ParentRefs: []gwv1.ParentReference{ |
There was a problem hiding this comment.
I think this is not an issue for the per-RayService Gateway case, since the Gateway is dedicated to the RayService. However, if users want to use a shared Gateway to manage traffic for the entire cluster, we may need to consider how to scope the HTTPRoute to the correct listener and hostname.
For now, I think we can leave this as a follow-up and address it when we get more user feedback.
There was a problem hiding this comment.
The reason existingGatewayRef isn't optional for us (not just a nice-to-have): some Gateway controllers only program a single admin-owned Gateway and never reconcile controller-created ones — Contour's static gateway.gatewayRef mode is the concrete case. There, a per-RayService Gateway is created but never programmed, so traffic simply never flows. Attaching to the shared Gateway is the only working path on those clusters, which is why it's in scope now rather than a follow-up.
There was a problem hiding this comment.
Anyways, it's a capability, so we can use a shared gateway.
HTTPRoute carries hostnames (and pathPrefixes), so each RayService carves out its own host/path on the shared Gateway and routes don't collide.
3 participants
Why are these changes needed?
NewClusterWithIncrementalUpgradealways creates its own per-RayService Gateway (<name>-gateway) and waits for the Gateway controller to program it before shifting traffic. This fails on clusters whose Gateway controller only reconciles one specific shared Gateway — notably Contour in staticgateway.gatewayRefmode (Contour v1.30+ removedcontrollerName), where KubeRay-created Gateways stayProgrammed=Unknownforever and the incremental traffic shift never starts.This PR adds
ClusterUpgradeOptions.existingGatewayRef {name, namespace}. When set:createGatewayreturnsnil, soreconcileGatewayskips Gateway creation (KubeRay does not own the shared Gateway; the existingdesiredGateway == nilguard already handles this).RayServiceGatewayNamespacedNameresolves to the referenced Gateway, so the HTTPRoute'sParentRefand the readiness check target it.backendRefsuse the RayService's namespace (owner-ref GC keeps working; equals the Gateway ns in the default case, differs for a cross-namespace shared Gateway — which must allow HTTPRoutes from that namespace).gatewayClassNamebecomes optional and is mutually exclusive withexistingGatewayRef; validation is updated accordingly. CRDs, deepcopy, and generated client code are regenerated.Fully backward compatible: unset
existingGatewayRef⇒ previous behavior (KubeRay creates the per-RayService Gateway fromgatewayClassName).Related issue number
Checks
ValidateClusterUpgradeOptions:existingGatewayRefvalid alone, mutually exclusive withgatewayClassName, requires both name and namespace.createGatewayreturnsnilwhenexistingGatewayRefis set.createHTTPRoutetargets the referenced Gateway viaParentRef(in the Gateway's namespace) while the HTTPRoute and itsbackendRefsstay in the RayService namespace.RayServiceGatewayNamespacedNameresolves to the referenced Gateway.