Skip to content

client: introduce ConnectionManager to fix reconnect/disconnect panics#472

Open
dave-tucker wants to merge 3 commits into
ovn-kubernetes:mainfrom
dave-tucker:conmgr
Open

client: introduce ConnectionManager to fix reconnect/disconnect panics#472
dave-tucker wants to merge 3 commits into
ovn-kubernetes:mainfrom
dave-tucker:conmgr

Conversation

@dave-tucker

@dave-tucker dave-tucker commented May 13, 2026

Copy link
Copy Markdown
Collaborator

Add a ConnectionManager (CM) — a finite-state machine goroutine that
owns all OVSDB connection lifecycle and proxies RPC calls, eliminating
the data races and panics that occurred when concurrent callers triggered
reconnect and disconnect simultaneously.

ConnectionManager (client/connection.go):

  • New FSM with three states: Disconnected, Connecting, Connected.
  • Single run-loop goroutine that serialises all state transitions;
    commands arrive on a buffered cmdCh and responses on per-command reply
    channels.
  • Server-push notifications (update/update2/update3) are forwarded to the
    client event loop via a 64-slot eventCh so the rpc2 callback returns
    immediately and never blocks on cache work.
  • Inactivity probing, leader-only mode, and multi-endpoint failover are
    all handled inside the CM run loop, removing the goroutine-per-concern
    races present in the previous implementation.
  • Close() keeps the CM alive in drain mode so in-flight commands receive
    ErrNotConnected rather than blocking; Connect() after Close() restarts
    the event loop cleanly.

client.go / event loop:

  • runEventLoop() reads from eventCh and dispatches:
    EventDisconnected — purges cache, sets deferUpdates=true
    EventReconnected — re-applies schema and reestablishes monitors
    EventInboundRPC — applies update/update2/update3 to the cache
  • waitForCacheConsistent() polls deferUpdates under cacheMutex.RLock so
    Get/List/etc. block until the post-reconnect monitor replay finishes.

Bug fixes:

  • Close() deadlock when reestablishMonitors was in-flight.
  • Close() hang when cache event handlers blocked on shutdown.
  • Connect() after Close() failing to restart the event loop.
  • Data race in DatabaseModel/Mapper accessed without modelMutex.

Integration test fixes:

  • Reset model struct on each Eventually iteration: model.CloneInto uses
    json.Unmarshal which merges into existing maps rather than replacing
    them; a fresh struct on each poll ensures deleted map keys do not
    survive across attempts (TestMultipleOpsTransactIntegration,
    TestMultipleOpsSameRow).
  • Increase TestMultipleOpsSameRow Eventually timeout from 2s to 5s; the
    two-transact setup needs more headroom on slower OVS 3.x runners.
  • Add explicit cache-consistent waits after Transact calls in tests that
    assert immediate cache state.

@dave-tucker
dave-tucker force-pushed the conmgr branch 2 times, most recently from d093e3a to addf503 Compare May 14, 2026 09:54
@dave-tucker
dave-tucker marked this pull request as draft May 14, 2026 13:20
@dave-tucker
dave-tucker marked this pull request as ready for review May 15, 2026 10:23
dave-tucker and others added 3 commits July 10, 2026 07:55
Add a ConnectionManager (CM) — a finite-state machine goroutine that
owns all OVSDB connection lifecycle and proxies RPC calls, eliminating
the data races and panics that occurred when concurrent callers triggered
reconnect and disconnect simultaneously.

ConnectionManager (client/connection.go):
- New FSM with three states: Disconnected, Connecting, Connected.
- Single run-loop goroutine that serialises all state transitions;
  commands arrive on a buffered cmdCh and responses on per-command reply
  channels.
- Server-push notifications (update/update2/update3) are forwarded to the
  client event loop via a 64-slot eventCh so the rpc2 callback returns
  immediately and never blocks on cache work.
- Inactivity probing, leader-only mode, and multi-endpoint failover are
  all handled inside the CM run loop, removing the goroutine-per-concern
  races present in the previous implementation.
- Close() keeps the CM alive in drain mode so in-flight commands receive
  ErrNotConnected rather than blocking; Connect() after Close() restarts
  the event loop cleanly.

client.go / event loop:
- runEventLoop() reads from eventCh and dispatches:
    EventDisconnected  — purges cache, sets deferUpdates=true
    EventReconnected   — re-applies schema and reestablishes monitors
    EventInboundRPC    — applies update/update2/update3 to the cache
- waitForCacheConsistent() polls deferUpdates under cacheMutex.RLock so
  Get/List/etc. block until the post-reconnect monitor replay finishes.

Bug fixes:
- Close() deadlock when reestablishMonitors was in-flight.
- Close() hang when cache event handlers blocked on shutdown.
- Connect() after Close() failing to restart the event loop.
- Data race in DatabaseModel/Mapper accessed without modelMutex.

Integration test fixes:
- Reset model struct on each Eventually iteration: model.CloneInto uses
  json.Unmarshal which merges into existing maps rather than replacing
  them; a fresh struct on each poll ensures deleted map keys do not
  survive across attempts (TestMultipleOpsTransactIntegration,
  TestMultipleOpsSameRow).
- Increase TestMultipleOpsSameRow Eventually timeout from 2s to 5s; the
  two-transact setup needs more headroom on slower OVS 3.x runners.
- Add explicit cache-consistent waits after Transact calls in tests that
  assert immediate cache state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
…ppression

Two correctness issues identified in code review:

1. EventDisconnected/Reconnected/SchemaData competed with EventInboundRPC on
   a single 64-slot non-blocking channel. Under an inbound-update burst,
   lifecycle events could be silently dropped, leaving the client with stale
   state and unrepaired monitors.

   Fix: introduce a dedicated lifecycleCh (cap 8, blocking send) for lifecycle
   events. EventInboundRPC stays on eventCh (cap 64, non-blocking) where
   occasional drops under load are acceptable. runEventLoop uses a priority
   double-select that drains lifecycleCh before blocking on eventCh.

2. After Connect(), setReconnectTime() armed a 3-second suppression window.
   When the OVS container restarted within that window, withinReconnectGrace()
   swallowed the disconnect signal from rpc2, leaving the client in Connected
   state with a dead TCP socket. TestWithReconnect was permanently skipped
   because of this.

   Fix: use rpcClient.DisconnectNotify() directly as the disconnect signal in
   the run-loop select. rpc2 closes this channel when that specific client
   closes; each new connection produces a new channel, so signals from a
   previous connection are naturally abandoned — no grace timer needed.
   Remove setReconnectTime/withinReconnectGrace/startDisconnectWatcher and the
   drain-sleep-drain loops in doCmdConnect and runReconnectLoop.

Unskip TestWithReconnect now that the underlying bug is fixed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
Add two unit tests for the CmdNotLeader code path, which was previously
untested in connection_test.go:

- TestConnectionManagerNotLeaderFailsOver: connect to endpoint A, send
  CmdNotLeader, verify the CM rotates the endpoint list to [B, A],
  reconnects to B, and emits EventReconnected on lifecycleCh.

- TestConnectionManagerNotLeaderNoReconnectDisconnects: send CmdNotLeader
  with no backoff configured; verify the CM transitions to
  StateDisconnected, emits no lifecycle event, and returns
  ErrNotConnected for subsequent RPCs.

Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
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.

1 participant