feat: native gRPC multi-backend failover via pick_first - #415
Conversation
One channel for the process lifetime with C-core pick_first over comma-separated collector addresses (Node native failover analogue). Single address stays plain host:port; multi uses ipv4:/ipv6: (mixed families via IPv4-mapped). Multi hostnames expand once at channel build. pick_first shuffleAddressList is on; target / default_authority stay in config order. Channel options: HTTP proxy off, no keepalive, reconnect backoff capped at 30s, service_config retries only reportInstanceProperties. Skip reports until READY. Unary and sync streaming RPCs use a deadline (10s floor, always > queue window); aio client-streaming collect omits timeout because generators await empty queues. Failed send batches are counted as drops. Instance properties errors do not block keepAlive. Replacing a protocol closes the previous channel (sync close; aio await aclose on the agent loop). Timed shutdown flush so atexit cannot hang. Includes unit coverage and a multi-OAP gRPC failover E2E case.
|
Thanks for the comprehensive failover work. I found four issues that should be addressed before merging: 1. [P1] Keep multi-backend collector channels excluded from gRPC instrumentation
The existing gRPC plugin excludes the agent's collector channel by comparing I reproduced an agent-channel RPC producing a SkyWalking span. In the sync reporter, completing a reporting RPC then enqueues its own span, which can sustain a continuous self-reporting loop. Please adapt the multi-address sync and aio paths so agent-owned channels remain excluded from tracing. Prefer an explicit agent-channel marker over resolving/comparing the target again, and add regression tests with the gRPC plugin installed. 2. [P1] Set gRPC fork support before importing grpc
This top-level import loads grpcio reads this setting during module initialization. I reproduced Please keep the generic logging helpers in a gRPC-neutral module or otherwise defer importing 3. [P2] Do not cancel the task owned by
|
d9105a4 to
d6d11de
Compare
…own, RPC margin) Skip sw_grpc instrumentation for agent collector channels via thread-local scope; move throttled reporter logs off grpc_channel so GRPC_ENABLE_FORK_SUPPORT is set before import grpc; cancel only background tasks on async shutdown; widen sync RPC deadline vs queue batch window.
|
Thanks for the thorough review, All four points are addressed in the latest commit 1. [P1] Keep multi-backend collector channels excluded from gRPC instrumentationAgent-owned collector channels are now marked explicitly instead of relying on Regression tests with 2. [P1] Set gRPC fork support before importing grpcThrottled reporter logging helpers were moved out of Subprocess-based import-order tests confirm that 3. [P2] Do not cancel the task owned by
|
|
Thanks for the update. I rechecked head The collector-channel exclusion now works for both sync and aio, the fork-support import ordering is fixed, and positive queue timeouts now have sufficient RPC headroom. I still see two P2 issues:
All CI checks are green, including the failover jobs, and the focused unit suite passes; these two lifecycle/configuration cases are not covered by the current tests. |
Run async cleanup on the asyncio.run root after _finished is set so protocol aclose() completes before Runner teardown. Preserve the first Queue.get attempt when SW_AGENT_QUEUE_TIMEOUT=0. Add regression tests.
|
Thanks for the recheck. Both follow-up items from the latest review are addressed in 1. Keep the
|
|
Thanks for the update. The two follow-up issues are fixed, but the root-lifecycle change introduces one new P2 regression. [P2] Observe unexpected background-task completion while waiting for shutdown At The concrete path is the unwrapped profile command dispatcher ( I reproduced this on The underlying possibility of Please keep cleanup inside the root, while also observing both the shutdown event and unexpected background-task completion. Any completed task should have its result/exception retrieved and logged (and then be restarted or trigger orderly root cleanup). A regression test should verify that a failing background task cannot disappear silently. |
Wait on both the shutdown event and background tasks so failures like __command_dispatch() are retrieved and logged instead of being ignored. Retrieve outcomes from already-done tasks during shutdown cancellation.
|
Thanks for the recheck. The remaining P2 from the latest review is addressed in Observe unexpected background-task completion while waiting for shutdownThe
A regression test verifies that a failing background task is logged and cannot disappear silently while the root continues waiting. CIFork CI is green at |
|
Thanks for the update. The original silent-exception path is now observed, root-owned cleanup runs, and a yielding 1. [P2] Clean shutdown logs normal reporter completion as an error
Reproduction scenario using the actual
Result, reproduced 30/30 runs: The same cleanup path also logs a real task exception twice: once when During shutdown, a task that returned normally after 2. [P2] Unexpected cancellation before shutdown remains silent
Reproduction scenario using the actual root lifecycle:
Result, reproduced 30/30 runs: Therefore a cancelled reporter, command dispatcher, or connectivity watcher can disappear while the agent appears healthy. Cancellation observed by the supervisor before shutdown should be treated as unexpected and should trigger logging plus orderly root cleanup. Cancellation deliberately issued later by Suggested regression coverage:
The focused local suite passes ( |
… failures Supervisor treats early success/cancel as errors; cleanup only logs real exceptions and skips already-handled outcomes so clean shutdown stays silent. Assign the outcome marker directly to satisfy flake8 B010.
|
Thanks for the recheck. Both P2 follow-ups from the latest review are addressed in 1. Clean shutdown no longer logs normal reporter completion as an error
2. Unexpected cancellation before shutdown is observedThe shutdown supervisor now treats pre-shutdown Regression coverage now exercises the wait helper and the subsequent cleanup /
CIFork CI is green at |
Summary
SW_AGENT_COLLECTOR_BACKEND_SERVICES, aligned with skywalking-nodejs #144: one channel for the process lifetime with C-corepick_firsthost:port; multi usesipv4:/ipv6:(mixed families via IPv4-mapped).shuffleAddressListis on;:authority/ TLS SNI stay on the first configured endpointgrpc.enable_http_proxy=0, no gRPC keepalive, reconnect backoff capped at 30s,service_configretries onlyManagementService.reportInstanceProperties. READY gate before dequeue; failed send batches discarded with throttled drop logs. Unary / sync streaming RPCs use a deadline; aio client-streaming collect omitstimeout=because generators await empty queues. Bounded shutdown flush soatexitcannot hangDocs:
docs/en/setup/Intrusive.mdandConfiguration.md.Test plan
tests/e2e/case/grpc/failover/(two OAPs; stop a backend that has traffic; sorted service list)e2e-failoveron 3.10 / 3.12 / 3.14)