Skip to content

feat: eBPF vrf_table GC sweep and control-daemon startup - #287

Merged
privateip merged 1 commit into
pr2-ebpf-datapathfrom
pr4-gc-installer-ebpf
Aug 6, 2026
Merged

feat: eBPF vrf_table GC sweep and control-daemon startup#287
privateip merged 1 commit into
pr2-ebpf-datapathfrom
pr4-gc-installer-ebpf

Conversation

@privateip

Copy link
Copy Markdown
Contributor

Summary

Adds SweepEBPFVRFTable, a GC pass that reconciles the eBPF uSID datapath's vrf_table map entries against live BGPVRFInstance CRDs, using a generation-cutoff scheme to avoid a register/sweep race. This runs from galactic-cni's run container rather than galactic-router's existing GC controller: the pinned vrf_table map only exists inside that container, which has the /sys/fs/bpf hostPath mount and CAP_BPF that galactic-router's DaemonSet doesn't need for anything else. routerNamesForNode gains a fuller sibling, routersForNode, since the sweep needs each router's full Spec.SRv6Locator, not just its name.

Wires installer.Run to load/attach/pin the eBPF datapath at startup, serve /metrics (Prometheus), report an ebpf-datapath gRPC health sub-service, and run the GC sweep on its own ticker. Adds a --metrics-port CLI flag to galactic-cni run.

Note

This depends only on #283 (the datapath control-plane packages), not on the CNI ADD cutover in #285/#286 — the sweep reconciles directly against BGPRouter/BGPVRFInstance CRD state rather than anything registerEBPFDatapath writes, so it's a sibling branch in the stack rather than stacked on top of the CNI cutover.

Test plan

  • go build ./...
  • go vet ./...
  • go test ./internal/gc/... ./internal/installer/... ./cmd/...
  • task lint (0 issues)

Part of the eBPF uSID datapath cutover stack (base: #283).

Adds SweepEBPFVRFTable, a GC pass that reconciles the eBPF uSID
datapath's vrf_table map entries against live BGPVRFInstance CRDs
using a generation-cutoff scheme to avoid a register/sweep race. This
runs from galactic-cni's "run" container rather than
galactic-router's existing GC controller: the pinned vrf_table map
only exists inside that container, which has the /sys/fs/bpf hostPath
mount and CAP_BPF galactic-router's DaemonSet does not need for
anything else. routerNamesForNode gains a fuller sibling,
routersForNode, since the sweep needs each router's full
Spec.SRv6Locator, not just its name.

Wires installer.Run to load/attach/pin the eBPF datapath at startup
(startEBPFDatapath), serve /metrics (Prometheus), report an
"ebpf-datapath" gRPC health sub-service, and run the GC sweep on its
own ticker. Adds the --metrics-port CLI flag to galactic-cni run.

Depends only on #283 (attach/usidmap/metrics/uformat) -- no
dependency on the CNI ADD cutover in #285/#286, since this reconciles
directly against BGPRouter/BGPVRFInstance CRD state rather than
anything registerEBPFDatapath writes to resourceTracker.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@privateip
privateip marked this pull request as ready for review August 5, 2026 02:01
@privateip
privateip requested a review from a team as a code owner August 5, 2026 02:01
@privateip
privateip requested review from 0xmc, ecv, mattdjenkinson, scotwells and slindseysr and removed request for a team August 5, 2026 02:01
@ecv

ecv commented Aug 5, 2026

Copy link
Copy Markdown

Rewritten after reading the rest of the stack, #283 through #289; one of my original questions was already answered by #288, and two others turned out to have direct evidence behind them.

Two things worth crediting first: capturing the generation cutoff before listing the CRDs, and bailing out entirely when zero BGPRouters come back rather than treating that as "zero live attachments." The second is the kind of guard that usually only gets written after it has already wiped a node once.

  1. The sweep reconciles vrf_table only, but registration writes all three maps. LocatorTable and FunctionTable both already implement Unregister in feat: eBPF uSID datapath control-plane packages #283 — as far as I can tell nothing calls either one, and only VRFTable has a Reconcile. So locator and function entries accumulate with no reclamation path at all, against caps of 64 and 128. A node whose SRv6Locator changes — renumbering, or the secondary /48 block that feat: Datum Cloud IPv6 addressing plan enhancements#740 documents as the route past 4,095 instances — strands the old block's entries permanently, and at 64 accumulated blocks registration starts failing, which the cutover surfaces as a failed pod ADD rather than a datapath drop. Is locator/function GC deliberately deferred, or is the assumption that locators never change in practice?

  2. The zero-routers guard protects against a mass wipe, but the same case one level down isn't guarded, and Reconcile has no internal safety of its own — anything absent from live and below the cutoff is deleted unconditionally. So an instance skipped by the caller for an empty or unparseable SRv6Locator, or an out-of-range VRFID, is not merely ignored: it is reaped on that same tick, and a live pod loses its datapath entry over a transient or malformed CRD field. Worth noting the contrast with fix: SEG6 encap mode, IPv4 SID advertisement, reconcile skip-on-error #289, which deliberately adopts skip-and-continue in the router's advertisement build — there the cost of skipping is "this path isn't advertised," here it's "this entry is deleted." Same idiom, very different blast radius. Should a skip for any reason other than "not one of our routers" abort the reconcile for that tick?

  3. feat: Grant CNI DaemonSet BPF privileges and wire up e2e #288 wires ebpf-datapath to both a liveness and a readiness probe, 10s period, no failureThreshold (so the default 3). The health check re-resolves the interface set on every call and reports unhealthy on a transient ResolveInterfaces failure — its own comment calls that "occasionally noisy" and correct for a readiness signal, which I agree with. On liveness it means roughly 30 seconds without a default route restarts the container, on every affected node at once. Restarting does re-attach, so it self-heals, but the netlink watch loop already exists to do exactly that. Is liveness on this signal deliberate, or would readiness alone (plus the watch loop) cover it with less amplification?

  4. The metrics server starts after the datapath, and a datapath failure is fatal to Run. So the failure mode most worth observing — preflight rejection, crashlooping container — is the one where nothing is ever scraped and the only signal is container logs. Worth starting the metrics listener first so a failed startup is still visible on /metrics, or is the crashloop considered signal enough?

  5. The type assertion on the datapath start's return value means the test fake, a plain closer, leaves metrics, health, and GC wiring inert — as your own comment notes. That's honest, but it also means the tests cover "Run starts and shuts down" rather than any of the three tick paths this PR adds. Would having the start function return an interface the fake can satisfy be worth it to get the health and sweep branches under test?

Smaller: the resolved interface list is stored in the datapath state and never read again, since the health handle re-resolves internally. And the reconcile error path bumps the error count by one regardless of how many deletions failed, so the count reads as "this sweep had errors" rather than a magnitude.

Same CI note as the rest of the stack: this targets pr2-ebpf-datapath, and the workflow's branches: [main] filter means none of the tests here have actually run in CI yet.

@privateip
privateip merged commit d7c822b into pr2-ebpf-datapath Aug 6, 2026
5 checks passed
privateip added a commit that referenced this pull request Aug 6, 2026
Grants the CNI DaemonSet's credential-refresh container CAP_BPF and
CAP_NET_ADMIN plus a bpf-fs hostPath mount (/sys/fs/bpf), and adds an
"ebpf-datapath" gRPC health sub-service to its liveness/readiness
probes -- required unconditionally now that this container also hosts
the eBPF/TC-BPF uSID datapath's load/attach/pin control daemon
(installer.Run, #287); there's no flag left to gate this behind, since
the datapath is the only forwarding path.

Pins GALACTIC_CNI_EBPF_INTERFACES=eth1 for the containerlab topology,
where every lab node is dual-homed and interface auto-detection picks
the wrong (management-bridge) link over the actual transit-fabric one.

Mounts bpffs on Kind nodes in CI (scripts/ci.sh) and updates
TestCNITapInterface to run a privileged e2e pod with its own bpf-fs
mount, starting the eBPF control daemon and waiting for vrf_table to
be pinned before exercising CNI ADD -- the datapath being the only
forwarding path means the e2e pod needs the same maps a production
node's DaemonSet would already have pinned. Adds test-unit-root to
test-e2e's dependency list.

Stacked on #286 (CNI eBPF cutover + gateway fix) and #287 (GC/installer
wiring) -- the visible diff includes both until they merge upstream;
review focuses on the deploy/e2e files listed above.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@privateip
privateip deleted the pr4-gc-installer-ebpf branch August 6, 2026 12:31
privateip added a commit that referenced this pull request Aug 6, 2026
Grants the CNI DaemonSet's credential-refresh container CAP_BPF and
CAP_NET_ADMIN plus a bpf-fs hostPath mount (/sys/fs/bpf), and adds an
"ebpf-datapath" gRPC health sub-service to its liveness/readiness
probes -- required unconditionally now that this container also hosts
the eBPF/TC-BPF uSID datapath's load/attach/pin control daemon
(installer.Run, #287); there's no flag left to gate this behind, since
the datapath is the only forwarding path.

Pins GALACTIC_CNI_EBPF_INTERFACES=eth1 for the containerlab topology,
where every lab node is dual-homed and interface auto-detection picks
the wrong (management-bridge) link over the actual transit-fabric one.

Mounts bpffs on Kind nodes in CI (scripts/ci.sh) and updates
TestCNITapInterface to run a privileged e2e pod with its own bpf-fs
mount, starting the eBPF control daemon and waiting for vrf_table to
be pinned before exercising CNI ADD -- the datapath being the only
forwarding path means the e2e pod needs the same maps a production
node's DaemonSet would already have pinned. Adds test-unit-root to
test-e2e's dependency list.

Stacked on #286 (CNI eBPF cutover + gateway fix) and #287 (GC/installer
wiring) -- the visible diff includes both until they merge upstream;
review focuses on the deploy/e2e files listed above.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 6, 2026
…t EEXIST

Three independent attach-path fixes from ecv's review of #283, bundled
together since all three land in attach.go/usid.c:

1. (point 2) usid_ingress's fail-open paths (not IPv6, too short to
   parse, no locator_table match -- design plan R6) returned
   TC_ACT_OK. This filter attaches direct-action at a fixed tc
   priority, and Cilium attaches its own tc/bpf programs to the same
   native-device ingress hook on these hosts; in direct-action mode
   TC_ACT_OK is a final verdict that ends the qdisc's filter chain, so
   a packet this program doesn't claim would never reach a colocated
   Cilium filter at a later priority. Switched every pre-locator-match
   fail-open path to TC_ACT_UNSPEC, which hands off to the next filter
   instead. Every fail-open path *after* the locator_table match (this
   program has claimed the packet) is unaffected -- those are, and
   remain, TC_ACT_SHOT. Also made the filter's tc priority overridable
   via GALACTIC_CNI_EBPF_FILTER_PRIORITY (default unchanged at 1),
   mirroring the same override internal/vmtap/config.go already
   exposes for its own Cilium-priority-collision risk.

2. (point 3) Load pinned every map by name and treated any schema
   mismatch against an existing pin (ebpf.ErrMapIncompatible -- e.g. a
   changed value struct size or max_entries) as fatal. vrf_value grew
   egress_kind this cycle and vrf_table sizing is an open question on
   #287, so the next such change would crashloop every node until an
   operator manually deleted the stale pins. Load now unpins and
   recreates any incompatible map before retrying the load once --
   every map here is control-plane-owned and reconstructable
   (usidmap.Register/the GC controller repopulate it), so losing its
   contents across a schema change is the correct trade-off against a
   crashloop.

3. (point 8) ensureClsact listed qdiscs then unconditionally added one
   if absent, racy against any other agent (notably Cilium) doing the
   same thing to the same device between the two calls. QdiscAdd
   returning EEXIST in that window is now treated as success, since
   the qdisc ensureClsact wanted to exist now does.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 6, 2026
Grants the CNI DaemonSet's credential-refresh container CAP_BPF and
CAP_NET_ADMIN plus a bpf-fs hostPath mount (/sys/fs/bpf), and adds an
"ebpf-datapath" gRPC health sub-service to its liveness/readiness
probes -- required unconditionally now that this container also hosts
the eBPF/TC-BPF uSID datapath's load/attach/pin control daemon
(installer.Run, #287); there's no flag left to gate this behind, since
the datapath is the only forwarding path.

Pins GALACTIC_CNI_EBPF_INTERFACES=eth1 for the containerlab topology,
where every lab node is dual-homed and interface auto-detection picks
the wrong (management-bridge) link over the actual transit-fabric one.

Mounts bpffs on Kind nodes in CI (scripts/ci.sh) and updates
TestCNITapInterface to run a privileged e2e pod with its own bpf-fs
mount, starting the eBPF control daemon and waiting for vrf_table to
be pinned before exercising CNI ADD -- the datapath being the only
forwarding path means the e2e pod needs the same maps a production
node's DaemonSet would already have pinned. Adds test-unit-root to
test-e2e's dependency list.

Stacked on #286 (CNI eBPF cutover + gateway fix) and #287 (GC/installer
wiring) -- the visible diff includes both until they merge upstream;
review focuses on the deploy/e2e files listed above.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
privateip added a commit that referenced this pull request Aug 6, 2026
…t EEXIST

Three independent attach-path fixes from ecv's review of #283, bundled
together since all three land in attach.go/usid.c:

1. (point 2) usid_ingress's fail-open paths (not IPv6, too short to
   parse, no locator_table match -- design plan R6) returned
   TC_ACT_OK. This filter attaches direct-action at a fixed tc
   priority, and Cilium attaches its own tc/bpf programs to the same
   native-device ingress hook on these hosts; in direct-action mode
   TC_ACT_OK is a final verdict that ends the qdisc's filter chain, so
   a packet this program doesn't claim would never reach a colocated
   Cilium filter at a later priority. Switched every pre-locator-match
   fail-open path to TC_ACT_UNSPEC, which hands off to the next filter
   instead. Every fail-open path *after* the locator_table match (this
   program has claimed the packet) is unaffected -- those are, and
   remain, TC_ACT_SHOT. Also made the filter's tc priority overridable
   via GALACTIC_CNI_EBPF_FILTER_PRIORITY (default unchanged at 1),
   mirroring the same override internal/vmtap/config.go already
   exposes for its own Cilium-priority-collision risk.

2. (point 3) Load pinned every map by name and treated any schema
   mismatch against an existing pin (ebpf.ErrMapIncompatible -- e.g. a
   changed value struct size or max_entries) as fatal. vrf_value grew
   egress_kind this cycle and vrf_table sizing is an open question on
   #287, so the next such change would crashloop every node until an
   operator manually deleted the stale pins. Load now unpins and
   recreates any incompatible map before retrying the load once --
   every map here is control-plane-owned and reconstructable
   (usidmap.Register/the GC controller repopulate it), so losing its
   contents across a schema change is the correct trade-off against a
   crashloop.

3. (point 8) ensureClsact listed qdiscs then unconditionally added one
   if absent, racy against any other agent (notably Cilium) doing the
   same thing to the same device between the two calls. QdiscAdd
   returning EEXIST in that window is now treated as success, since
   the qdisc ensureClsact wanted to exist now does.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

5 participants