docs(): Add ADR for Hub-and-Spoke Partial Mesh MVP topology (#300)#406
docs(): Add ADR for Hub-and-Spoke Partial Mesh MVP topology (#300)#406Shreesha001 wants to merge 2 commits into
Conversation
Signed-off-by: Shreesha001 <shettyshreesha552@gmail.com>
d565d89 to
fb6dbee
Compare
There was a problem hiding this comment.
Pull request overview
Adds an Architecture Decision Record (ADR) documenting the proposed Hub-and-Spoke topology for the Partial Mesh MVP in KubeSlice, intended as a design gate for subsequent controller/worker-operator implementation work.
Changes:
- Introduces a new ADR covering proposed SliceConfig topology schema, edge computation, rewire behavior, status aggregation, webhook validation rules, and convergence sequences.
- Documents operational trade-offs and non-goals for the MVP scope.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### 6. Status aggregation | ||
|
|
||
| Per-link connectivity is reported on `WorkerSliceGateway.status`, since `WorkerSliceGateway` represents an individual gateway link between two clusters. | ||
|
|
||
| ```yaml | ||
| status: | ||
| connectionState: Connected # Connected | NotConnected | Pending | ||
| lastTransitionTime: "2026-06-24T10:05:00Z" | ||
| reason: DialTimeout | ||
| message: "gateway dial failed after 30s" | ||
| ``` | ||
|
|
||
| `lastTransitionTime` is updated only when the connection state changes, not on every reconcile. | ||
|
|
||
| The controller aggregates the status of all `WorkerSliceGateway` objects belonging to the Slice and reports overall topology health on `SliceConfig.status.conditions`. | ||
|
|
||
| This proposal adds a standard `Conditions []metav1.Condition` field to `SliceConfigStatus`, which currently only contains `KubesliceEvents []KubesliceEvent`. | ||
|
|
||
| ```yaml | ||
| status: | ||
| conditions: | ||
| - type: TopologyConverged | ||
| status: "True" | ||
| reason: AllEdgesReady | ||
| message: All desired topology links are established. | ||
| lastTransitionTime: "2026-06-24T10:05:00Z" | ||
| ``` | ||
|
|
||
| `TopologyConverged` is: | ||
|
|
||
| * `True` when `failingEdges == 0` and `readyEdges == desiredEdges`. | ||
| * `False` when one or more desired gateway links are not connected. The condition message identifies the first failing gateway link. | ||
|
|
||
| When gateway links are removed (for example during hub rewiring), their corresponding `WorkerSliceGateway` objects are deleted through the existing cleanup flow and no stale status entries remain. | ||
|
|
||
| ### 7. Spoke-to-spoke reachability | ||
|
|
There was a problem hiding this comment.
@Shreesha001 - please review and resolve this one.
pnavali
left a comment
There was a problem hiding this comment.
This will be a living document with multiple reviews.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: pnavali <75800004+pnavali@users.noreply.github.com>
gourishkb
left a comment
There was a problem hiding this comment.
Strongest of the two ADRs this week. Decision 3's explicit Client/Server role pinning (hub = Server, spoke = Client) is the right call and something the current code doesn't guarantee — good catch. Decision 7's spoke-to-spoke routing via a slice-CIDR-scoped default route is clever and self-maintaining. A few things need resolution before this gates #470/#471.
Must fix
-
Decision 2 contradicts Decision 7 on worker-side changes. Decision 2 says "the worker-operator needs little or no change for the core feature to work" because gating happens controller-side. Decision 7 then proposes adding a default-route injection to the
sliceRouterInjectRoutepath inrouter-sidecar(worker-side). These two claims conflict. Please clarify: is spoke-to-spoke routing via hub in scope for this MVP, and if so, acknowledge explicitly that it requires worker-side changes. If it is out of scope, move it to Non-Goals and remove Decision 7. -
Decision 4's concurrent edit guard requires stateful webhook validation — the webhook must read the existing
SliceConfig'sstatus.conditions(to checkTopologyConverged) during an UPDATE call. This is more complex than the stateless validations already in the file. Please address: (a) does the webhook have a client to read the existing object? (it receivesoldobject in the admission request, so yes — useold.Status.Conditions, no extra fetch needed); (b) what is the behavior whenTopologyConvergedcondition is absent (first topology update on a new slice)? Document the answer so the implementer doesn't have to guess. -
SliceConfigStatus.Conditionsfield addition is an implicit CRD schema change. Decision 6 addsConditions []metav1.ConditiontoSliceConfigStatus. This requires a Go type change,make generate && make manifests, and a separate PR for the CRD update. The ADR should list this as an explicit prerequisite for #303 and reference the PR that will carry it. Right now it reads like a free addition but it isn't — it must be done before any status-aggregation code can ship. -
Missing: FullMesh → HubAndSpoke transition in sequence diagrams. The hub-change sequence is covered, but the initial transition from an existing full-mesh slice to HubAndSpoke is not. Specifically: when an operator applies
topology.mode: HubAndSpoketo an existing full-mesh slice,cleanupObsoleteGatewayswill delete a large number ofWorkerSliceGatewayobjects simultaneously. What is the expected connectivity impact? Is there a maximum safe rate of deletion? Please add a note in Decision 4 and a short sequence for this case.
Minor
-
Issues #301, #302, #303, #304 appear in the header's
Relatedfield but are not in the original downstream issue list from #300 (which only lists worker-operator #470 and #471). Please confirm these are newly created issues, link them from #300, and update the issue body to reference them so the tracking is complete. -
Missing trailing newline at end of file.
| - **Hub↔Spoke:** every hub connects to every spoke. | ||
| - **Hub↔Hub:** every hub connects to every other hub (only when `|H| > 1`). | ||
| - **Spoke↔Spoke:** never created. | ||
|
|
There was a problem hiding this comment.
This paragraph says "the worker-operator needs little or no change for the core feature to work". Decision 7 later proposes adding a default-route injection to sliceRouterInjectRoute in router-sidecar, which IS a worker-side change. These two claims conflict — please reconcile.
| Hubs: `[hub-1, hub-2]` | ||
| Spokes: `[a, b, c]` | ||
|
|
||
| | Edge | Type | |
There was a problem hiding this comment.
The Client/Server role pinning (hub=Server, spoke=Client) is the right call for NAT-traversal. Suggest adding it as a webhook validation rule: reject HubAndSpoke if the hub cluster has no NodeIP or no external endpoint configured (i.e. ClusterStatus.NodeIPs is empty). Otherwise an operator can designate a NAT'd spoke as hub and not realize it won't work until tunnels fail to come up.
| ``` | ||
|
|
||
| **MVP scope note:** the algorithm above is written for a general `|H|`, but the MVP caps `hubs` at one entry via webhook rule 10. Hub↔hub edges, Example 2, and the multi-hub terms of the formula document the design's generality and are not reachable this term; enabling multi-hub later means removing that webhook rule, with no schema or algorithm change. | ||
|
|
There was a problem hiding this comment.
The concurrent-edit guard is correct in principle, but the webhook receives the old object in the admission request — use oldObj.Status.Conditions to read TopologyConverged without an extra API call. Please also specify the nil case: if the condition is absent (slice was just created or never transitioned), treat it as True (no in-progress rewire) so the first topology update is always allowed.
| ```yaml | ||
| status: | ||
| conditions: | ||
| - type: TopologyConverged |
There was a problem hiding this comment.
Adding Conditions []metav1.Condition to SliceConfigStatus requires a Go type change and CRD regeneration (make generate && make manifests). This is not free — it needs its own PR (can be part of the #301 CRD PR). Please call this out as an explicit dependency so whoever implements #303 knows they need the schema change landed first.
| - Hub health-based failover (accepted single-hub SPOF risk for this term) | ||
| - Any topology mode other than `HubAndSpoke` and `FullMesh` | ||
|
|
||
| --- No newline at end of file |
There was a problem hiding this comment.
Missing trailing newline.
Adds the ADR for the Partial Mesh MVP (Hub-and-Spoke topology) at
docs/adr/hub-and-spoke-adr.md, as required by #300. It covers all seven decisions from the issue (CRD schema, resolver placement, edge computation, rewire safety, backward compatibility, desiredPeers delivery, status aggregation), plus validation webhook rules, both sequence diagrams, non-goals, and open issues for rewiring.This is the prerequisite gate for worker-operator #470 and #471.
Fixes #300
How Has This Been Tested?
Documentation-only change; no code modified. Code references in the ADR were verified against the current source.
Checklist:
Does this PR introduce a breaking change for other components like worker-operator?
No.