Skip to content

grpcconf: add opt-in round_robin LB policy and client keepalive#7

Merged
samlown merged 2 commits into
mainfrom
grpc-round-robin-keepalive
Jul 14, 2026
Merged

grpcconf: add opt-in round_robin LB policy and client keepalive#7
samlown merged 2 commits into
mainfrom
grpc-round-robin-keepalive

Conversation

@samlown

@samlown samlown commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

gRPC clients built via grpcconf.Service used gRPC's default pick_first balancer: each client opens one connection to a single backend pod and pins to it for the connection's lifetime. When that pod is rolled the client keeps dialing the dead endpoint, and with no keepalive a stalled request hangs until the kernel TCP timeout (~15 min). This is the root cause of the 2026-07-13 API 504 storm.

Changes

  • New Policy field on Servicepick_first (default / zero value) or round_robin. round_robin is opt-in per service via config; callers targeting a headless Service set policy: round_robin to spread calls across all backend pods and drop failed sub-channels on re-resolution.
  • Unconditional client keepalive (Time: 5m, Timeout: 20s, PermitWithoutStream: false) so a black-holed connection is detected in minutes rather than at the TCP timeout. Applies regardless of policy (safe against the servers' default EnforcementPolicy).
  • Explicit blank import _ "google.golang.org/grpc/balancer/roundrobin" so an upstream grpc change can't silently drop the transitively-registered balancer and revert us to pick_first.

Caller-supplied dial opts are still applied last and win.

Behaviour / rollout note

Because round_robin is now opt-in and defaults to pick_first, merging this alone does not change runtime behaviour — each consuming service (api → silo/transform/access/sequence) must set policy: round_robin in its config to get the fix. Tracked as a follow-up.

Keepalive 5m floor

The silo/transform/access/sequence servers run gRPC's default EnforcementPolicy (MinTime 5m); a faster client ping would be too_many_pings-GOAWAY'd. 5m is the safe floor for a client-only change. Server-side follow-ups (invopop/silo#231, invopop/transform#83, invopop/access#238, invopop/sequence#25) relax the policy; a companion PR (#8) then shortens the interval.

Testing

go build ./..., go vet, gofmt clean. Confirmed round_robin resolves at runtime (balancer.Get("round_robin") != nil).

Copilot AI review requested due to automatic review settings July 13, 2026 22:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the shared grpcconf.Service client defaults to improve resiliency when dialing internal gRPC backends (particularly across Kubernetes pod rolls and dead/black-holed connections).

Changes:

  • Default gRPC client load balancing to round_robin via grpc.WithDefaultServiceConfig.
  • Add client keepalive parameters (Time: 5m, Timeout: 20s, PermitWithoutStream: false) to bound stalls on dead connections.
  • Clarify that caller-provided dial options are appended last (so they can override defaults).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/grpcconf/grpcconf.go
Comment thread pkg/grpcconf/grpcconf.go
@samlown

samlown commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — checked this against the pinned gRPC version (google.golang.org/grpc v1.81.1), and the round_robin balancer is already registered, so no side-effect import is needed here:

  • The core google.golang.org/grpc package blank-imports the balancer itself — clientconn.go:55: _ "google.golang.org/grpc/balancer/roundrobin" // To register roundrobin.
  • balancer/roundrobin registers itself in init() via balancer.Register(builder{}) (Name = "round_robin").
  • go list -deps ./pkg/grpcconf shows google.golang.org/grpc/balancer/roundrobin in the transitive graph, and a minimal program importing only google.golang.org/grpc reports balancer.Get("round_robin") != niltrue.

Since grpcconf imports google.golang.org/grpc, the balancer is registered transitively and WithDefaultServiceConfig applies the policy at runtime. The suggestion is correct in general for balancers that core doesn't pull in (and for some older grpc-go versions), but it's redundant here. Happy to add the explicit _ "google.golang.org/grpc/balancer/roundrobin" as belt-and-suspenders if we'd rather not depend on the transitive import staying put across upgrades.

Add a Policy field to Service selecting the client load-balancing
policy: pick_first (the default, matching gRPC) or round_robin.
round_robin spreads calls across all backend pods returned by a headless
Service and drops failed sub-channels on re-resolution, instead of
pick_first pinning to a single pod — the root cause of the 2026-07-13
API 504 storm. Callers opt in per service via config.

Also add an unconditional client keepalive so a black-holed connection
is detected in minutes rather than at the kernel TCP timeout (~15m), and
an explicit blank import of the round_robin balancer so an upstream
change can't silently drop it and revert us to pick_first.

keepaliveTime is 5m to stay within the servers' default
EnforcementPolicy.MinTime; a follow-up relaxes that and shortens it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@samlown samlown changed the title grpcconf: default to round_robin LB and client keepalive grpcconf: add opt-in round_robin LB policy and client keepalive Jul 14, 2026
Copilot AI review requested due to automatic review settings July 14, 2026 08:16
@samlown samlown force-pushed the grpc-round-robin-keepalive branch from d9e2c91 to c1f82c2 Compare July 14, 2026 08:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread pkg/grpcconf/grpcconf.go
Bundled with the gRPC reliability changes in this release. v1.82.0 has no
changes to the round_robin/pick_first balancers or DNS resolver; note the
balancer registry is now case-sensitive by default, which our exact
"round_robin" service-config name satisfies.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 14, 2026 08:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pkg/grpcconf/grpcconf.go:107

  • When PolicyRoundRobin is enabled, the effectiveness of round_robin depends on the name resolver returning multiple backend addresses (e.g., DNS A records for a headless Service) and re-resolving on failures. This code always dials s.URL() (Host+":"+Port) without normalizing/validating the target scheme, so a misconfigured Host value could silently result in no effective load balancing.
func (s *Service) Connection(opts ...grpc.DialOption) (*grpc.ClientConn, error) {
	do := s.DialOptions()
	do = append(do, opts...)
	return grpc.NewClient(s.URL(), do...)
}

Comment thread pkg/grpcconf/grpcconf.go
@samlown samlown merged commit 191009f into main Jul 14, 2026
3 checks passed
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