Skip to content

net: scope worker gateway topology watches - #918

Open
Patrick W. Healy (phealy) wants to merge 36 commits into
phealy/net-api-assignmentsfrom
phealy/net-api-worker-watches
Open

Patrick W. Healy (phealy) wants to merge 36 commits into
phealy/net-api-assignmentsfrom
phealy/net-api-worker-watches

Conversation

@phealy

@phealy Patrick W. Healy (phealy) commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Stop worker agents from watching cluster-wide gateway topology they don't use.

  • Gateway informers. The agent starts the GatewayPool, GatewayPoolNode and SiteGatewayPoolAssignment informers only while its role or its Site's assignments need them. It stops them when they no longer do.
  • MTU.
    • MTU is resolved from actual forwarding paths rather than from undirected topology components.
    • SiteGatewayPoolAssignment MTUs no longer apply to ordinary worker mesh links. They still apply on the gateway path to an assigned Site.
  • Watch timeout. The randomized Node informer watch timeout is doubled, so watches are re-established half as often. LIST requests are unchanged.
  • Startup race. The gateway event callback is installed before the reconcile worker starts, and events go through a synchronized lookup. Events that arrive during startup are no longer lost.

Testing

  • Unit tests for:
    • informer start and stop across role and assignment changes
    • registering the callback after events start arriving (add, update and delete for all three resources)
    • MTU along forwarding paths
    • the watch timeout wrapper
  • Tested on an isolated AKS cluster with two gateways and two remote workers:
    • ICMP and HTTP succeeded in both directions, 10 of 10.
    • Removing the remote Site from the assignment and adding it back stopped and restarted exactly the three gateway informers per remote. No agents restarted.
    • Demoting a gateway and promoting it again replaced only that gateway's agent. The other gateway carried traffic with no loss.

Stack

This is one of five stacked PRs that reduce API server load from unbounded-net-controller and unbounded-net-node. Kubernetes remains the source of truth. Review them bottom to top:

  1. net: skip no-op managed kube-proxy reconciliation #914 Controller no-op reconciliation
  2. net: reconcile SiteNodeSlices per Site #915 Targeted SiteNodeSlice reconciliation
  3. net: publish node assignments and replace stale agents #916 Controller-published node assignments
  4. net: adopt and acknowledge node assignments in the agent #917 Node agent assignment adoption
  5. net: scope worker gateway topology watches #918 Scoped worker gateway watches

Motivation. A large cluster running v0.8.0 had the controller continuously writing objects that hadn't changed. Every node agent also watched the whole cluster's gateway topology. On a canary cluster, the full stack cut controller API calls by 54.4% and node agent API calls by 73.8%.

Use informer cache comparisons and normalized server defaults to avoid no-op API writes, while narrowing DaemonSet and Site event requeues.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved startup concurrency, blocked reconciliation, MTU calculation, and status consistency issues prevent approval.

6 open findings
What changed in this PR

Reduces node-agent API server load by running gateway topology watches only when needed, completing the five-PR optimization stack.

Changes:

  • Adds gateway informer lifecycle management, synchronized callbacks, and readiness reporting.
  • Calculates MTU limits from forwarding paths rather than connected topology.
  • Doubles informer watch timeouts and adds tests and documentation.
File Description
docs/​net/​configuration.md Documents forwarding-path MTU limits.
docs/​net/​architecture.md Explains conditional gateway watches.
docs/​content/​reference/​networking/​configuration.md Updates public MTU guidance.
cmd/​unbounded-net-node/​status_server.go Adds lifecycle-aware readiness and status.
cmd/​unbounded-net-node/​status_server_http_test.go Tests gateway readiness responses.
cmd/​unbounded-net-node/​site_watch_reconcile.go Integrates informer lifecycle and transit MTUs.
cmd/​unbounded-net-node/​reconciliation_helpers.go Adds forwarding-path MTU calculations.
cmd/​unbounded-net-node/​reconciliation_helpers_test.go Tests revised MTU behavior.
cmd/​unbounded-net-node/​node_types.go Carries transit MTU limits.
cmd/​unbounded-net-node/​mtu.go Applies role-aware assignment limits.
cmd/​unbounded-net-node/​main.go Separates discovery and gateway informers.
cmd/​unbounded-net-node/​informer_factory.go Doubles watch timeouts.
cmd/​unbounded-net-node/​informer_factory_test.go Tests watch and LIST options.
cmd/​unbounded-net-node/​gateway_informer_lifecycle.go Manages conditional gateway caches.
cmd/​unbounded-net-node/​gateway_informer_lifecycle_test.go Tests lifecycle transitions and callbacks.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cmd/unbounded-net-node/reconciliation_helpers.go Outdated
Comment thread cmd/unbounded-net-node/reconciliation_helpers.go Outdated
Comment thread cmd/unbounded-net-node/site_watch_reconcile.go
Comment thread cmd/unbounded-net-node/site_watch_reconcile.go Outdated
Comment thread cmd/unbounded-net-node/gateway_informer_lifecycle.go Outdated
Comment thread cmd/unbounded-net-node/status_server.go
Generations are only comparable within one UID, so treat a same-name
replacement observed as an update as relevant.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
SiteNodeSlice membership is derived from internal IPs, so Node updates and
deletions dirty both the labeled and IP-matched Sites. Slice updates dirty
both the old and new siteName so the original Site can repair a slice that
the new Site deletes as extra.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread cmd/unbounded-net-node/reconciliation_helpers.go Outdated
Comment thread cmd/unbounded-net-node/site_watch_reconcile.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

MTU resolution still omits applicable forwarding-path limits and includes unrelated assignment caps.

3 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread cmd/unbounded-net-node/reconciliation_helpers.go
Comment thread cmd/unbounded-net-node/site_watch_reconcile.go Outdated
Comment thread cmd/unbounded-net-node/site_watch_reconcile.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

MTU calculations still omit intermediate gateway Site caps and worker-to-gateway SitePeering caps.

2 open findings
3 resolved since last review

🧠 Review effort: Balanced

Comment thread cmd/unbounded-net-node/mtu.go Outdated
Comment thread cmd/unbounded-net-node/reconciliation_helpers.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved MTU and partial-teardown recovery issues can affect traffic, and repeated cache scans add avoidable scaling costs.

2 open issues
2 previously missed issues

In code that hasn't changed since last review

Medium severity Index gateway pool MTUs to avoid repeated hop scans

cmd/​unbounded-net-node/​reconciliation_helpers.go:356

Every adjacent pool hop scans all GatewayPoolPeerings through gatewayPoolPeeringTunnelMTU (reconciliation_helpers.go:264-296). With R advertised paths, H pool hops per path, and P two-pool peerings, this adds O(RHP) work per gateway peer. CIDRs sharing paths repeat the scan, and it runs before the unchanged-state check (site_watch_reconcile.go:1895,2168-2197). Index enabled MTU minima by unordered pool pair once per reconciliation, as already done for assignment and SitePeering edges, and use that lookup here.

Medium severity Avoid quadratic gateway cache scans during informer resync

cmd/​unbounded-net-node/​site_watch_reconcile.go:487

This full-cache dependency check runs for every assignment update, including informer resyncs. For a worker with no matching assignment, gatewayCachesRequired parses all N objects (gateway_informer_lifecycle.go:381-388), with a JSON marshal/unmarshal for each (reconciliation_helpers.go:670-677). A resync delivering N updates therefore performs N² JSON round trips per worker outside the coalesced reconcile queue; the default resync is 600 seconds (main.go:299). Index assignments by Site, or restrict synchronous checks to events affecting the pinned Site and leave full scans to the coalesced reconcile. Preserve immediate cancellation when the last relevant assignment disappears.

2 resolved since last review
1 file not reviewed
  • api/net/v1alpha1/zz_generated.deepcopy.go: Generated file

Review effort: Balanced

Comment thread cmd/unbounded-net-node/reconciliation_helpers.go
Comment thread cmd/unbounded-net-node/wireguard_config.go
Bypass unchanged-state reconciliation after a partial programming failure, and propagate gateway policy installation errors. Index enabled pool-peering MTU caps once per reconcile. Avoid synchronous assignment-cache scans for unchanged resyncs and events unrelated to the adopted Site, preserving immediate local dependency cancellation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
@phealy

Copy link
Copy Markdown
Contributor Author

Addressed the two additional scaling findings from review 5553902702: pool-peering MTU minima are indexed by canonical unordered pool pair once per reconciliation; assignment event callbacks no longer scan the full cache for remote-Site events or unchanged informer resyncs. Local membership changes, including tombstone deletion and moves away from the adopted Site, still refresh dependency requirements synchronously. Added enabled/disabled/duplicate edge-minimum tests and event tests exercising 1,000 resyncs plus local/remote membership transitions. Node/netlink race tests and scoped lint pass.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Coupled informer lifetimes, partial dataplane recovery, and distributed MTU behavior warrant final human review.

1 open issue
1 resolved since last review
1 file not reviewed
  • api/net/v1alpha1/zz_generated.deepcopy.go: Generated file

Review effort: Balanced

Compute a common configured Site and mesh-peering MTU cap from existing caches. Shared pod-facing and tunnel interfaces require this constraint even without transitive routing. Exclude disabled, network-only, missing-Site and disconnected relationships; do not join components through gateway paths. Apply the cap to peers and the local fabric without adding API writes or watches, and cover the three-Site asymmetric case and upward recovery.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Coupled informer lifecycle, MTU propagation, and partial-cleanup recovery changes affect critical networking paths and warrant human review.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Avoid quadratic site-pair indexing during reconciles

cmd/​unbounded-net-node/​reconciliation_helpers.go:412

This eagerly builds every pair for each capped SitePeering: 1,000 distinct Sites require 1,000,000 comparisons and 499,500 map entries. The caller rebuilds the index before selecting gateway peers and before the unchanged-state check (site_watch_reconcile.go:1820,2187), so even workers without gateway assignments pay this quadratic CPU and allocation cost on periodic reconciles. Skip gateway-only indexing when no gateway peers need it, and use Site-to-peering memberships with cached lookups for pairs needed by accepted gateway paths instead of expanding every pair.

1 resolved since last review
1 file not reviewed
  • api/net/v1alpha1/zz_generated.deepcopy.go: Generated file

Review effort: Balanced

Replace eager all-pairs expansion with linear Site and GatewayPool memberships and memoized lookups for pairs actually used by accepted gateway paths. Build gateway-only indexes lazily so direct-mesh workers do not pay for them. Cover 1,000-member peerings and repeated reverse-pair cache hits without quadratic storage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
@phealy

Copy link
Copy Markdown
Contributor Author

Fixed the previously missed quadratic Site-pair indexing finding from review 5554640820 in ff09134. SitePeering and GatewayPoolPeering now share a linear membership index with memoized pair lookups, rather than eagerly expanding every pair. Gateway-only indexes are created lazily after accepting an actual gateway/path, so workers without gateway peers skip them entirely. Tests verify 1,000 members create 1,000 memberships and zero cached pairs initially, then repeated/reversed requested-pair lookups retain one cache entry. Scoped lint and node/netlink race suites pass.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Interacting informer retirement, cross-node MTU limits, and partial dataplane recovery require final human validation of network convergence.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Low severity Document direct-mesh component cap and its CNI MTU impact

docs/​content/​reference/​networking/​configuration.md:220

This reference omits the direct-mesh component cap applied in cmd/unbounded-net-node/site_watch_reconcile.go:2101-2112. For example, A-B=1200 and A-C=1400 also constrain C to 1200, even though C does not forward through B. Document this exception and its effect on CNI MTU, matching docs/net/configuration.md:307-315, so operators do not expect independent per-path limits.

1 file not reviewed
  • api/net/v1alpha1/zz_generated.deepcopy.go: Generated file

Review effort: Balanced

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
@phealy

Copy link
Copy Markdown
Contributor Author

Fixed the documentation finding from review 5554696112 in 3659bf5. The public networking configuration reference now documents the direct-mesh component cap, the A-B=1200/A-C=1400 example, the shared CNI MTU impact, excluded relationships, and that constraints propagate without adding transitive routes. It now matches docs/net/configuration.md.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved MTU recovery failures need fixes, and coordinated cache retirement and forwarding changes warrant human validation.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity MTU verification failure does not trigger dataplane repair

cmd/​unbounded-net-node/​site_watch_reconcile.go:1958

If an existing gateway tunnel's MTU drifts below the committed fabric MTU, this verification fails even on the unchanged-state path (site_watch_reconcile.go:2198-2203). That path leaves dataplaneDirty false, so later reconciles skip tunnel programming again and repeatedly fail route/MTU publication instead of repairing the interface. Mark the dataplane dirty on verification failure so the next reconcile retries programming.

1 file not reviewed
  • api/net/v1alpha1/zz_generated.deepcopy.go: Generated file

Review effort: Balanced

Comment thread cmd/unbounded-net-node/site_watch_reconcile.go
Retain the attempted fabric MTU independently from committed routing state and retry CNI/pod MTU writes after any incomplete reconciliation. Mark all error exits dirty, including unchanged-state verification failures. Preserve tighten-before-tunnel and tunnel-before-relax ordering through failed writes and topology reversions. Extend production-path tests for failed status publication, partial pod MTU updates, verification drift, and both reversion directions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: ab9b78ee-42b8-4841-8d74-cd78d002d48b
@phealy

Copy link
Copy Markdown
Contributor Author

Also fixed the previously missed MTU-verification recovery finding from review 5554744809 in 4b04cae: all error exits, including verification failure on the unchanged-state path, mark dataplaneDirty. The regression test injects MTU drift, verifies no status PATCH occurs, and confirms the next reconcile actually reprograms the dataplane rather than repeating the no-op path.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Coupled informer retirement, partial dataplane recovery, and distributed MTU handling warrant final human review.

No open issues 🎉

1 resolved since last review
1 file not reviewed
  • api/net/v1alpha1/zz_generated.deepcopy.go: Generated file

Review effort: Balanced

@phealy
Patrick W. Healy (phealy) marked this pull request as ready for review October 16, 2026 23:50
@phealy
Patrick W. Healy (phealy) requested a review from a team October 16, 2026 23:50
Preserve scoped gateway informers and forwarding MTU handling while integrating the parent's startup fixes and automatic neighbor sizing. Align neighbor-sizing regression coverage with the existing failed-reconciliation retry fence.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6b70e00f-eec0-4611-97c6-1b6200092b62

This branch has not been deployed

No deployments
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.

2 participants