[orchagent] HLD: Improve Resiliency of Orchagent Route Programming

[venkit-nexthop]

Summary

• Adds a new HLD document under doc/orchagent/ describing a redesign of the orchagent route programming hot path.
• The redesign updates m_toSync directly from mqPollThread, splits drain() into three independently schedulable stages (toBulkTask, flushTask, responseHandlingTask) with yield/resume semantics and bounded time quanta, and makes the route state publish asynchronous.
• Documents the new show orchagent tasks / sonic-clear orchagent tasks instrumentation.
• Diagrams are checked in as external PNGs under doc/orchagent/images/orchagent_route_redesign/ (no inline base64).

Why

Tracked under sonic-net/sonic-buildimage#27098: the current serial pops() / addToSync() / drain() flow in the orchagent main thread lets ZMQ buffers and intermediate non-coalescing queues back up under churn and starves other orch agents of CPU.

Test plan

• HLD reviewed against the SONiC HLD template.
• Diagrams render correctly on GitHub via the relative images/orchagent_route_redesign/*.png links.
• Doc-only change; no build/test impact.

[mssonicbld]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[venkit-nexthop]

@prabhataravind @qiluo-msft @prsunny @vivekrnv @deepak-singhal0408 for viz

[mssonicbld]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[mssonicbld]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[mssonicbld]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[securely1g]

HLD Review

TL;DR: Solid HLD that addresses a real pain point — orchagent's route programming path starving other orch agents under churn. The design is well-structured and the incremental rollout approach is pragmatic. A few concerns below.

---

Strengths

• Problem statement is crystal clear — serial processing + non-coalescing queues = starvation under load
• Incremental implementation plan (initial PRs hold lock for full drain() duration, same as today, then relax later)
• No SAI/syncd/DB schema changes — purely internal to orchagent
• Good observability story with the per-Executor stats (median/q1/q3/max/outliers)
• Warmboot/fastboot impact properly addressed (neutral to positive)
• Already has companion PRs in flight: swss-common#1187, swss#4564, swss#4437, swss#4563, utilities#4536

Concerns / Questions

1. Lock contention details are thin — §7.2 says "locks protect concurrent access to m_toSync" but doesn't specify the lock granularity. With mqPollThread writing and toBulkTask reading/removing concurrently, what's the locking strategy? Per-bucket? A single mutex with short critical sections? This is the crux of the performance story and deserves more detail.

2. Time quantum values not specified — §7.5 says "fixed time quantum" but never states what the default is (or if it's configurable). Is it 10ms? 50ms? 500ms? The right value depends heavily on the platform and scale. Should be at least mentioned as a tunable with a sensible default.

3. Self-notification for reschedule — §7.4 says tasks reschedule by "posting a notification event to self." What's the priority/ordering of these self-notifications vs. other Selectables? If route tasks keep self-notifying, could they still starve lower-priority orch agents through round-robin effects?

4. unordered_map iteration order — §7.5 says toBulkTask walks "some bounded number of entries" from m_toSync. With unordered_map, iteration order is non-deterministic. Under sustained churn, could some prefixes get consistently deprioritized (starvation at the entry level)? Maybe a cursor/epoch approach would be safer.

5. Failure retry semantics — §7.5 says responseHandlingTask writes failed entries back to m_toSync "if the key does not exist there." But what if the failure is persistent (e.g., SAI resource exhaustion)? The entry will bounce between m_toSync → toBulk → flush → fail → m_toSync indefinitely. Is there a retry limit or backoff?

6. Testing gaps — No mention of testing the lock correctness under concurrent access (e.g., TSAN/thread sanitizer). Given this introduces real multi-threaded mutation of m_toSync, that seems important. Also no chaos/fault-injection test for the yield/resume checkpointing.

7. Memory bound on toBulk — If toBulkTask accumulates entries across multiple quanta before flushTask runs, toBulk could grow large. Is there a cap on toBulk size that triggers a flush, or is it purely time-driven?

Minor nits

• §9.2 table formatting is slightly broken in the markdown (missing closing | on the header row) — cosmetic
• The open items (§14) about fpmsyncd back-pressure feel important enough to be tracked as a follow-up issue rather than just a bullet

---

Overall this is a well-thought-out HLD for a meaningful improvement. The main gap is more specifics on the concurrency model (lock strategy, quantum values, starvation prevention). Would approve with those addressed.

[deepak-singhal0408]

**On Qi's SyncMap direction — **

+1 to Qi's suggestion of a coalescing intermediate rather than locking m_toSync. To be clear, this isn't "lock-free" — it relocates the lock: instead of holding a lock on the hot m_toSync across doTask, the ingest side fills a small intermediate under a brief O(1) handoff, and the main thread swaps it out and programs the snapshot unlocked. Two design reasons:

Result — two things the goal hinges on:

• Keep the socket drained so the producer never hits HWM. A lock on m_toSync held during doTask makes ingest and programming contend, so the socket isn't drained while a burst is being programmed; a separate intermediate lets drain and programming run concurrently.
• Bound the buffer. Because the intermediate is a SyncMap (coalesced by key), a flapping prefix collapses to a single entry — so a sustained burst stays bounded by the number of distinct keys rather than growing per message toward OOM (as a queue<KCO> would). Coalescing is the memory-safety win, not just a throughput one.

Architecture: it keeps m_toSync single-threaded, preserving orchagent's existing "main loop isn't thread-safe" contract — a sharing property a shorter lock hold wouldn't restore.

On "won't the later yield/resume split shorten the lock anyway?" — it reduces the duration, but the intermediate is what makes that split correct: you can't hold a lock across a yield, so a resumed drain() needs a stable snapshot to iterate while new arrivals accumulate elsewhere — otherwise you're programming a map ingest is mutating mid-pass. The intermediate isn't an alternative to the split; it's the boundary the split depends on.

[deepak-singhal0408]

@venkit-nexthop, could you check and address comments..
This design change is critical to Scaled systems stability under stress/churn.

JFYI, I will be tracking this PR set actively and will try to help coordinate reviews/discussions/merges as needed.

[venkit-nexthop]

@venkit-nexthop, could you check and address comments.. This design change is critical to Scaled systems stability under stress/churn.

JFYI, I will be tracking this PR set actively and will try to help coordinate reviews/discussions/merges as needed.

I am working on this.

[mssonicbld]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[deepak-singhal0408]

Thanks for addressing the feedbacks. Overall Design LGTM.