Repository navigation
feat(worker): reconcile direct local routing EndpointSlices - #798
Conversation
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
📄
|
Signed-off-by: thxCode <thxcode0824@gmail.com>
…uards Signed-off-by: thxCode <thxcode0824@gmail.com>
The chart and site workflows installed helm v3.21.0. The tooling moves to v3.21.4 in the same series, so the workflows move with it: a workflow that renders or packages a chart with another helm than the repository develops against can produce an artifact the local gates never saw. The Go version needs no change: every workflow already reads the version from a go.mod (the root one, source/go.mod in the site checkout and gpustack.ai/gpustack/go.mod in the api checkout), so the 1.27.3 of the root go.mod is what they resolve. Signed-off-by: thxCode <thxcode0824@gmail.com>
Signed-off-by: thxCode <thxcode0824@gmail.com>
Review feedback summary
|
acb2c4d to
82f6566
Compare
…leanup Signed-off-by: thxCode <thxcode0824@gmail.com>
Review feedback summary (
|
| Source | Location | Verdict | Outcome |
|---|---|---|---|
@gpustack-code-review (#discussion_r4374446221) |
pkg/systemkuberess/service.go:160 |
Valid | Fixed (b7d1b748): newRoutingService now sets Spec.IPFamilies to [IPv4] or [IPv6] matching addressType (via routingIPFamily), and svcCompareFn checks Spec.IPFamilies as well as ClusterIP family so the recreated Service receives a ClusterIP of the matching family and converges (Test_installFakeSystemRoutingServiceKeepsTheRoutingServiceOfTheCluster). |
@gpustack-code-review (#discussion_r4374446230) |
pkg/systemkuberess/service.go:163 |
Valid | Fixed (b7d1b748): newRoutingService sets Protocol: core.ProtocolTCP on the https port and svcCompareFn checks ap.Name == expectedPort.Name, ap.Port == expectedPort.Port, and protocol compatibility (Test_installFakeSystemRoutingServiceKeepsTheRoutingServiceOfTheCluster/recreates_the_routing_service_when_the_port_name_differs). |
@gpustack-code-review (#discussion_r4374446201) |
pkg/systemkuberess/endpoint_slice.go:363 |
Valid | Fixed (b7d1b748): cleanupObsoleteRoutingSlices now scopes sliceCli.List with meta.ListOptions{LabelSelector: discovery.LabelServiceName + "=" + svc.GetName()}. |
@gpustack-code-review (#issuecomment-6409086430) |
docs/contribute/internals.md:130-131 |
Valid | Fixed (b7d1b748): Updated wording to state that the routing Service is recreated whenever its spec changes and the legacy Endpoints are recreated whenever they are deleted. |
@gpustack-code-review (#issuecomment-6409086430) |
pkg/systemkuberess/service.go:47, pkg/systemkuberess/endpoint_slice.go:410-412 |
Valid (reminder) | Covered (08f1078c, c59136d7, b7d1b748): pkg/systemkuberess/service_test.go ships unit tests covering all routing Service, Endpoints, and EndpointSlice reconcile, ownership, replacement, and cleanup paths. |
@gpustack-code-review (#issuecomment-6409086430) |
pkg/systemkuberess/endpoint_slice.go:144-148 |
Not applicable | Declined: An address-family change only happens when the worker restarts on a new primary host IP family, at which point the previous IP family address is no longer reachable and installRoutingService has already recreated the Service for the new family. Keeping the deterministic <svc>-direct slice name avoids leaving stale unreachable endpoints. |
@gpustack-code-review (#issuecomment-6409086430) |
.github/workflows/site.yml:102-104 |
Out of scope | Declined: All GitHub Actions across .github/workflows/*.yml in this repository follow major-version tag pinning (@v4), and line 102 (uses: azure/setup-helm@v4) was not modified in this PR. |
Review feedback summary (
|
| Source | Location | Verdict | Outcome |
|---|---|---|---|
@gpustack-code-review (#discussion_r4374538579) |
pkg/systemkuberess/service.go:197 |
Valid (reminder) | Covered (b7d1b748): Regression tests in pkg/systemkuberess/service_test.go (Test_installFakeSystemRoutingServiceKeepsTheRoutingServiceOfTheCluster and Test_installFakeSystemRoutingService) cover Spec.IPFamilies, port-name comparison, and label-scoped EndpointSlice cleanup. |
@gpustack-code-review (#issuecomment-6409086430) |
pkg/systemkuberess/endpoint_slice.go:360-361 |
Confirmed | No change needed: Prior to this PR, pkg/systemkuberess only created Service and Endpoints (never a direct EndpointSlice), and both endpointslice-mirroring-controller and newRoutingEndpointSlice always set kubernetes.io/service-name: <svcName> (discovery.LabelServiceName), as Required by the Kubernetes EndpointSlice contract for service association. |
What type of PR is this?
/kind enhancement
/area worker
/area documentation
What this PR does / why we need it:
Reconciles directly managed
discovery.k8s.io/v1EndpointSliceresources alongside the legacyEndpointsobject when the worker runs outside a nearby cluster, aligns workflow Helm pins tov3.21.4, and renames the local cluster setup guide:EndpointSlicerouting and legacy compatibility (pkg/systemkuberess/{endpoint_slice.go,service.go},deploy/gpustack-operator/chart/templates/worker/serviceaccount.yaml,docs/contribute/internals.md):ClusterIProutingServicewithout recreating it on restart whenSpec.ClusterIPhas already been assigned by the API server in the matching IP family.Endpointsobject withendpointslice.kubernetes.io/skip-mirror: "true"and directly reconciles<service>-directEndpointSlicewithendpointslice.kubernetes.io/managed-by: gpustack-operator-worker,kubernetes.io/service-name: <service>, matching address family (IPv4/IPv6), aready: true, serving: trueendpoint at the reachable worker IP, porthttps, and a controllerOwnerReferencepointing to the routingService.managed-bylabel isendpointslice-controller.k8s.ioand their controllerOwnerReferencematches the legacyEndpointsUID, preserving unrelated slices.SelfSubjectAccessReviewchecks (services,services/finalizers,endpoints,endpointslices) and worker RBAC (discovery.k8s.ioendpointslicesget/list/create/update/delete), and documents the three startup routing environment variables (KUBERNETES_POD_NAMESPACE,KUBERNETES_SERVICE_NAME,KUBERNETES_POD_IP) indocs/contribute/internals.md..github/workflows/chart.yml,.github/workflows/site.yml):azure/setup-helm@v4fromv3.21.0tov3.21.4to match the repository's Helm toolchain pin.docs/contribute/local-cluster-setup.md,docs/README.md,docs/contribute/development.md,site/data/navigation.yaml):docs/contribute/kind.mdtodocs/contribute/local-cluster-setup.md(# Local Cluster Setup).Which issue(s) this PR links to:
Relates #794
Relates #795
Relates #797
Special notes for your reviewer:
CGO_ENABLED=1 go test -race -count=1 ./pkg/systemkuberess/...,make lint,make lint chart,make lint docs, andmake test chart.Does this PR introduce a user-facing change?