[dualtor]: Add IPv6 only active-active control plane design - #2403
[dualtor]: Add IPv6 only active-active control plane design#2403zjswhhh wants to merge 2 commits into
Conversation
Signed-off-by: Jing Zhang <zhangjing@microsoft.com>
|
/azp run |
|
No pipelines are associated with this pull request. |
|
/azp run |
|
No pipelines are associated with this pull request. |
lolyu
left a comment
lolyu
left a comment
There was a problem hiding this comment.
Review: #2403
Clear, conservative, well-structured HLD. I read the full document and verified its load-bearing claims against source rather than taking them on faith:
- ✅ The
MUX_CABLEschema already carriesserver_ipv6/soc_ipv6(confirmed in sonic-swssmuxorch.cpp), so "no new Config DB fields" holds. - ✅ Loopback2 = prober source / Loopback3 = GUID source matches the actual linkmgrd source (
MuxManager.h,LinkProberBase.cpp,DbInterface.cpp) and the companion PR #344. - ✅ Companion linkmgrd #344 exists, same author, and its IPv4-first/IPv6-fallback logic matches this doc's Component Requirements.
The design accurately describes the feature its companion PRs implement. Nothing blocks. Two substantive suggestions inline (independent server-vs-soc family selection; missing-loopback error handling); the rest is polish.
One structural point that doesn't anchor to a single line — the server-vs-soc family cross-product. server_* (linkmgrd probing) and soc_* (ycabled gRPC) are selected independently per-field, so a port with server_ipv4 present but only soc_ipv6 would probe over IPv4 while doing gRPC over IPv6. Is that mixed-family case intended and supported? The Compatibility matrix only varies the soc_* axis and doesn't show the server×soc cross-product. Recommend either stating explicitly that family is selected independently per endpoint (and confirming mixed is valid), or adding a constraint that a port's server and soc endpoints must share a family. That's the one genuine spec gap — see inline #1.
Minor (polish): the <ul><li> HTML inside the Component Requirements table renders on GitHub but may not on the SONiC doc site's static generator or other markdown processors — worth checking whether the repo's other HLDs use raw HTML in tables or prefer sub-sections.
|
|
||
| ## Compatibility | ||
|
|
||
| | Config shape | linkmgrd selected endpoint | ycabled selected endpoint | |
There was a problem hiding this comment.
🟡 The selection granularity should be stated explicitly here. The rule is "use IPv6 when the matching IPv4 field is absent" — but server_* and soc_* are independent, so this matrix only covers the soc_* axis. It doesn't show the server×soc cross-product: e.g. server_ipv4 present + soc_ipv4 absent (only soc_ipv6) would have linkmgrd probing over IPv4 while ycabled connects gRPC over IPv6 on the same port. Is that mixed-family case supported? Recommend either (a) adding a sentence that family is selected independently per endpoint (server vs soc) and the mixed case is valid, or (b) adding a constraint that a port's server and soc endpoints must use the same family. Right now it's an unspecified behavior a reader can't resolve from the doc.
| | Component | Required behavior | | ||
| | --- | --- | | ||
| | Config generation | <ul><li>Generate `MUX_CABLE` entries when `server_ipv6` exists even if `server_ipv4` is absent.</li><li>Include `server_ipv6` and `soc_ipv6` fields when available.</li><li>Preserve IPv4 fields when they are present.</li></ul> | | ||
| | linkmgrd | <ul><li>Select IPv4 endpoints first when present.</li><li>Use `server_ipv6` and `soc_ipv6` when the matching IPv4 field is absent.</li><li>Use Loopback2 IPv6 as the link-prober source address when the selected probe endpoint is IPv6.</li><li>Use Loopback3 IPv6 as the GUID source component when the selected probe endpoint is IPv6.</li><li>Use ICMPv6 link probing for IPv6-selected endpoints.</li><li>Create IPv6 hardware ICMP echo session fields when hardware probing is enabled for IPv6-selected endpoints.</li></ul> | |
There was a problem hiding this comment.
🟡 These two loopback rules (Loopback2 IPv6 = prober source, Loopback3 IPv6 = GUID source) are correct — I verified them against the linkmgrd source and PR #344 — but the doc states them as bare facts. Two additions would help: (a) a brief why for these specific loopbacks so the convention isn't magic to a future reader/operator; and (b) the failure behavior when an IPv6-only deployment is missing Loopback2/Loopback3 IPv6 addresses. The Error Handling section covers missing endpoint fields but not missing source loopbacks, yet the Validation section (item 2) implies they're required config — so a "IPv6 selected but Loopback2/3 IPv6 source unavailable" row in Error Handling would close that gap.
|
|
||
| 1. If both IPv4 and IPv6 endpoint fields are missing, leave the endpoint unavailable for that port and log the condition. | ||
| 2. If the selected endpoint is malformed, reject that endpoint for the affected port and log the condition. | ||
| 3. If IPv4 is present but malformed, do NOT silently fall back to IPv6. |
There was a problem hiding this comment.
🟢 This rule — IPv4 present-but-malformed must NOT silently fall back to IPv6 — is the right call and well stated. It correctly distinguishes "IPv4 absent" (fall back to IPv6) from "IPv4 broken" (fail loudly, don't mask a misconfiguration), which is exactly the distinction that prevents a malformed-config split-brain from being papered over. Good that it's explicit.
3 participants
[dualtor]: Add IPv6 only active-active control plane design
Add a SONiC-level design document for IPv6-only active-active DualToR control-plane support. The document describes the intended behavior for config generation, linkmgrd, ycabled, compatibility, error handling, and sonic-mgmt validation without implementation-specific details.
The design is intentionally conservative: existing IPv4-only and dual-stack deployments continue to use IPv4, and IPv6 is selected only when the corresponding IPv4 field is missing.
sign-off: Jing Zhang zhangjing@microsoft.com