Skip to content

Commit 34ad211

Browse files
committed
refactor(sdk): address Go replica routing review
Share the header check between the routed attempt and its retry, keep the first-frame EOF rule in one bidi helper, reuse Get's request for the owner lookup, and document the header contract next to the replica routing docs. Part of #4223 Signed-off-by: divesh <dgude@nvidia.com>
1 parent 86df4ec commit 34ad211

6 files changed

Lines changed: 41 additions & 43 deletions

File tree

‎docs/kubernetes/ingress.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,7 @@ helm upgrade <release-name> oci://ghcr.io/nvidia/openshell/helm-chart \
257257
--set grpcRoute.replicaRouting.enabled=true
258258
```
259259

260-
The chart renders one Service per replica and a GRPCRoute rule that matches the `x-openshell-replica` request header. The CLI and the Rust SDK set that header from the owner the gateway reports and retry without it if the named replica is unavailable. Replica routing requires `workload.kind=statefulset`, `workload.allowMultiReplicaStatefulSet=true`, and at most 15 replicas.
260+
The chart renders one Service per replica and a GRPCRoute rule that matches the `x-openshell-replica` request header. The CLI and the Rust and Go SDKs send `x-openshell-want-owner: 1` on `GetSandbox`, read the owner from the `x-openshell-owner` response header, and set it as `x-openshell-replica`. If the proxy answers `UNAVAILABLE` with an empty message, `no healthy upstream`, or `connection failure`, they retry once without it. Replica routing requires `workload.kind=statefulset`, `workload.allowMultiReplicaStatefulSet=true`, and at most 15 replicas.
261261

262262
If the release uses `workload.kind=deployment`, this upgrade replaces the Deployment with a StatefulSet, so every gateway pod restarts at once. The StatefulSet also requests a 1Gi PersistentVolumeClaim per replica, which stays `Pending` on clusters without a default StorageClass.
263263

‎sdk/go/openshell/v1/exec_client.go‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -104,17 +104,8 @@ func (e *execClient) Interactive(ctx context.Context, workspace, sandboxName str
104104
startReq.WorkspaceScope = namedWorkspaceScope(workspace)
105105

106106
streamCtx, cancel := context.WithCancel(ctx)
107-
stream, err := openRouted(streamCtx, owner, func(ctx context.Context) (grpc.BidiStreamingClient[pb.ExecSandboxInput, pb.ExecSandboxEvent], error) {
108-
s, err := e.client.ExecSandboxInteractive(ctx)
109-
if err != nil {
110-
return nil, err
111-
}
112-
// io.EOF means the stream already ended; its status comes from Recv.
113-
if err := s.Send(&pb.ExecSandboxInput{Payload: &pb.ExecSandboxInput_Start{Start: startReq}}); err != nil && err != io.EOF {
114-
return s, err
115-
}
116-
return s, nil
117-
})
107+
start := &pb.ExecSandboxInput{Payload: &pb.ExecSandboxInput_Start{Start: startReq}}
108+
stream, err := openBidiRouted(streamCtx, owner, start, e.client.ExecSandboxInteractive)
118109
if err != nil {
119110
cancel()
120111
return nil, converter.FromGRPCError(err)

‎sdk/go/openshell/v1/routing.go‎

Lines changed: 21 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,15 @@ import (
88
"io"
99
"strings"
1010

11-
pb "github.com/NVIDIA/OpenShell/sdk/go/proto/openshellv1"
1211
"google.golang.org/grpc"
1312
"google.golang.org/grpc/codes"
1413
"google.golang.org/grpc/metadata"
1514
"google.golang.org/grpc/status"
1615
)
1716

1817
// Metadata that lets an edge proxy send a sandbox's long-lived calls straight
19-
// to the gateway replica holding its supervisor session.
18+
// to the gateway replica holding its supervisor session. The headers and the
19+
// retry rule are documented in docs/kubernetes/ingress.mdx.
2020
const (
2121
ownerReplicaHeader = "x-openshell-owner"
2222
ownerRequestHeader = "x-openshell-want-owner"
@@ -42,34 +42,41 @@ func sandboxOwner(ctx context.Context, sandboxes SandboxInterface, workspace, na
4242
// openRouted opens a stream tagged with the owner replica. It opens once more
4343
// untagged when the proxy reports that the owner's route has no ready endpoints.
4444
// A stale owner is safe: any replica relays the call to the real owner.
45+
// With an owner, it returns only after the gateway answers, as the CLI does.
4546
func openRouted[S grpc.ClientStream](ctx context.Context, owner string, open func(context.Context) (S, error)) (S, error) {
4647
if owner == "" {
4748
return open(ctx)
4849
}
49-
stream, err := open(metadata.AppendToOutgoingContext(ctx, routeReplicaHeader, owner))
50+
stream, err := answered(open(metadata.AppendToOutgoingContext(ctx, routeReplicaHeader, owner)))
51+
if retryUnrouted(err) {
52+
return answered(open(ctx))
53+
}
54+
return stream, err
55+
}
56+
57+
// answered waits for the response headers. Nil metadata means the stream ended
58+
// without headers, and RecvMsg then returns its status.
59+
func answered[S grpc.ClientStream](stream S, err error) (S, error) {
5060
if err == nil {
51-
// Header blocks until the proxy answers; nil metadata means the stream
52-
// ended without headers, and RecvMsg then returns its status.
5361
var md metadata.MD
5462
if md, err = stream.Header(); err == nil && md == nil {
5563
err = stream.RecvMsg(nil)
5664
}
5765
}
58-
if retryUnrouted(err) {
59-
return open(ctx)
60-
}
6166
return stream, err
6267
}
6368

64-
// openForward opens a ForwardTcp stream routed to owner and sends its init frame.
65-
func openForward(ctx context.Context, client pb.OpenShellClient, owner string, init *pb.TcpForwardFrame) (grpc.BidiStreamingClient[pb.TcpForwardFrame, pb.TcpForwardFrame], error) {
66-
return openRouted(ctx, owner, func(ctx context.Context) (grpc.BidiStreamingClient[pb.TcpForwardFrame, pb.TcpForwardFrame], error) {
67-
s, err := client.ForwardTcp(ctx)
69+
// openBidiRouted is openRouted for a bidi stream whose first frame is sent on open.
70+
func openBidiRouted[I, O any](ctx context.Context, owner string, first *I, open func(context.Context, ...grpc.CallOption) (grpc.BidiStreamingClient[I, O], error)) (grpc.BidiStreamingClient[I, O], error) {
71+
return openRouted(ctx, owner, func(ctx context.Context) (grpc.BidiStreamingClient[I, O], error) {
72+
s, err := open(ctx)
6873
if err != nil {
6974
return nil, err
7075
}
71-
// io.EOF means the stream already ended; its status comes from Recv.
72-
if err := s.Send(init); err != nil && err != io.EOF {
76+
// io.EOF means the stream already ended; answered reports its status.
77+
if err := s.Send(first); err == io.EOF {
78+
return answered(s, nil)
79+
} else if err != nil {
7380
return s, err
7481
}
7582
return s, nil

‎sdk/go/openshell/v1/sandbox_client.go‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -117,32 +117,32 @@ func validateTemplateCreateSpec(spec *SandboxSpec) error {
117117
}
118118

119119
func (s *sandboxClient) Get(ctx context.Context, workspace, name string) (*Sandbox, error) {
120-
resp, err := s.client.GetSandbox(ctx, &pb.GetSandboxRequest{
121-
Name: name,
122-
WorkspaceScope: namedWorkspaceScope(workspace),
123-
})
124-
if err != nil {
125-
return nil, converter.FromGRPCError(err)
126-
}
127-
return converter.SandboxFromProto(resp.GetSandbox()), nil
120+
return s.get(ctx, workspace, name)
128121
}
129122

130123
// getWithOwner is Get that also asks the gateway for the owner replica.
131124
func (s *sandboxClient) getWithOwner(ctx context.Context, workspace, name string) (*Sandbox, string, error) {
132125
var md metadata.MD
133-
resp, err := s.client.GetSandbox(
134-
metadata.AppendToOutgoingContext(ctx, ownerRequestHeader, "1"),
135-
&pb.GetSandboxRequest{Name: name, WorkspaceScope: namedWorkspaceScope(workspace)},
136-
grpc.Header(&md),
137-
)
126+
sb, err := s.get(metadata.AppendToOutgoingContext(ctx, ownerRequestHeader, "1"), workspace, name, grpc.Header(&md))
138127
if err != nil {
139-
return nil, "", converter.FromGRPCError(err)
128+
return nil, "", err
140129
}
141130
var owner string
142131
if v := md.Get(ownerReplicaHeader); len(v) > 0 {
143132
owner = v[0]
144133
}
145-
return converter.SandboxFromProto(resp.GetSandbox()), owner, nil
134+
return sb, owner, nil
135+
}
136+
137+
func (s *sandboxClient) get(ctx context.Context, workspace, name string, opts ...grpc.CallOption) (*Sandbox, error) {
138+
resp, err := s.client.GetSandbox(ctx, &pb.GetSandboxRequest{
139+
Name: name,
140+
WorkspaceScope: namedWorkspaceScope(workspace),
141+
}, opts...)
142+
if err != nil {
143+
return nil, converter.FromGRPCError(err)
144+
}
145+
return converter.SandboxFromProto(resp.GetSandbox()), nil
146146
}
147147

148148
func (s *sandboxClient) List(workspace string, opts ...ListOptions) (*Pager[*Sandbox], error) {

‎sdk/go/openshell/v1/ssh_client.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ func (s *sshClient) Tunnel(ctx context.Context, workspace, sandboxName string, p
100100
}
101101

102102
streamCtx, cancel := context.WithCancel(ctx)
103-
stream, err := openForward(streamCtx, s.client, owner, initFrame)
103+
stream, err := openBidiRouted(streamCtx, owner, initFrame, s.client.ForwardTcp) //nolint:revive // proto-generated method name
104104
if err != nil {
105105
cancel()
106106
return nil, converter.FromGRPCError(err)

‎sdk/go/openshell/v1/tcp_client.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func (t *tcpClient) Forward(ctx context.Context, workspace, sandboxName string,
6262
}
6363

6464
streamCtx, cancel := context.WithCancel(ctx)
65-
stream, err := openForward(streamCtx, t.client, owner, initFrame)
65+
stream, err := openBidiRouted(streamCtx, owner, initFrame, t.client.ForwardTcp) //nolint:revive // proto-generated method name
6666
if err != nil {
6767
cancel()
6868
return nil, converter.FromGRPCError(err)

0 commit comments

Comments
 (0)