From eba53e91c71f92211e7973f5e790d7378921047c Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 22:34:35 -0400 Subject: [PATCH 1/2] fix: Recreate only the eBPF maps whose layout changed When a new build's map spec no longer matched a pinned map, the uSID, edge and NAT loaders recovered by unpinning every map in the collection and loading fresh. Every writer's state was lost, not just the changed map's. On a compute node that is fatal for the uSID datapath. locator_table, function_table, vrf_table, ifindex_vrf_table and ifindex_egress_kind_table are written only by CNI ADD, so after the wipe every SRv6 packet to the node missed locator_table and failed open, and every VM stayed unreachable until it was recreated. That happened in staging on 2026-10-07 at 02:02 UTC when #768 grew vip_xlat_table's key from 16 to 32 bytes: both compute nodes lost every row, Envoy returned 503 UF for every VM backend, and recovery took recreating 44 VMs. Edge nodes recovered by themselves because their return loop and the ingress sidecar re-register their own rows. The new mappin package checks each pinned map against the compiled spec with MapSpec.Compatible and unpins only the ones that differ, skipping any the caller does not own (the gateway's dispatcher maps). All three loaders use it, and their warning names the maps they recreated. With this, #768's change would have recreated vip_xlat_table alone, which galactic-router refills from ServiceVIPBindings. A map whose own layout changes is still recreated empty. When that map is one only CNI ADD writes, its rows stay missing until each attachment re-attaches; rebuilding those rows at galactic-cni startup is follow-up work. The new attach test reproduces the incident: it pins an old-layout vip_xlat_table beside seeded vrf_table and locator_table rows and reloads. On the previous loader both rows are gone; with this change only vip_xlat_table is recreated. Related to #774 Co-Authored-By: Claude Opus 5.5 --- docs/agents/ARCHITECTURE-CNI.md | 2 +- internal/gc/gc.go | 11 +- internal/plumbing/ebpf/attach/attach.go | 50 +++---- internal/plumbing/ebpf/attach/attach_test.go | 4 +- .../ebpf/attach/schema_change_test.go | 78 +++++++++++ internal/plumbing/ebpf/edgeattach/attach.go | 34 +---- internal/plumbing/ebpf/mappin/mappin.go | 70 ++++++++++ internal/plumbing/ebpf/mappin/mappin_test.go | 125 ++++++++++++++++++ internal/plumbing/ebpf/natattach/attach.go | 31 +---- internal/plumbing/ebpf/prog/usid.c | 10 +- 10 files changed, 312 insertions(+), 103 deletions(-) create mode 100644 internal/plumbing/ebpf/attach/schema_change_test.go create mode 100644 internal/plumbing/ebpf/mappin/mappin.go create mode 100644 internal/plumbing/ebpf/mappin/mappin_test.go diff --git a/docs/agents/ARCHITECTURE-CNI.md b/docs/agents/ARCHITECTURE-CNI.md index d4936956..4e868438 100644 --- a/docs/agents/ARCHITECTURE-CNI.md +++ b/docs/agents/ARCHITECTURE-CNI.md @@ -465,7 +465,7 @@ auto-detect directly (see Configuration above). - **Base62 interface names.** Kernel interface names use the format `G{9-char-vpc-base62}{3-char-att-base62}{suffix}` (suffix: `V` = VRF, `H` = host veth/tap, `G` = guest veth pre-move), fitting in the 15-character kernel limit. The hex form is used for BGP route targets; base62 for kernel interfaces. - **VRF/route-target model via BGPVRFInstance.** `galactic-bgp` creates a `BGPVRFInstance` (RouteDistinguisher + import/export Route Targets, all set to the derived RT) before the `BGPAdvertisement`; `galactic-router`'s GoBGP runtime applies VRFs before originating EVPN paths. - **Every VRF table ends in an unreachable default, per family.** `vrf.Add` installs `unreachable default` for IPv4 and IPv6 at metric `4278198272` (`vrf.UnreachableDefaultMetric`, the value the kernel's VRF documentation recommends), on every call, not only when it creates the VRF. Without it, a lookup that misses in the VRF's table falls through the rules after the l3mdev rule to the main table, and the host routes a tenant packet the eBPF datapath didn't claim out its own default interface with the tenant's source address. The high metric means any real default in the table, such as the ingress sidecar's, still wins. Route sweepers must leave it alone: `hostgw` only removes unicast routes on the attachment's own device, and the installer's sidecar-return prune only touches tables from `0xF000` up. Each has a test pinning that. -- **TCP MSS is clamped in the datapath, not by the tenant or the fabric MTU.** A tenant packet crosses the fabric inside a 40-byte outer IPv6 header with no SRH, so a tenant interface at the uplink's own MTU advertises an MSS the path cannot carry, and nothing on the path fragments or reports an oversized segment. `clamp_tcp_mss` in `usid.c` lowers the MSS of every SYN and SYN-ACK at three points: `usid_egress` just before it encapsulates, `usid_egress`'s DSR reply redirect (the client's segments reach the backend encapsulated by the gateway), and `usid_ingress` just after it decapsulates (the only place a SYN-ACK returning through an egress shard can be clamped, since the shard does not). The limits are per tenant family, 1400 for IPv6 and 1420 for IPv4 at a 1500-byte fabric, and the IPv6 value also covers NAT64. `galactic-cni run` sizes them from the smallest uplink MTU (`internal/plumbing/ebpf/mssclamp`) into `mss_clamp_table`, overridable with `GALACTIC_CNI_TCP_MSS_CLAMP` (see [docs/cni/environment-variables.md](../cni/environment-variables.md#galactic_cni_tcp_mss_clamp)). The map and its counters (`mss_clamp_stats`, sized with spare slots) are new maps rather than new fields or `drop_reasons` slots, because changing any existing pinned map's layout makes `attach.Load` recreate every pinned map empty. `clamp_tcp_mss` is a global BPF function, verified once on its own, and walks at most 16 options: past about 20 steps the verifier's cost jumps from thousands of instructions to over its 1M limit. `usid_verifier_cost_test.go` fails if either program's verifier cost passes 16,000 instructions. +- **TCP MSS is clamped in the datapath, not by the tenant or the fabric MTU.** A tenant packet crosses the fabric inside a 40-byte outer IPv6 header with no SRH, so a tenant interface at the uplink's own MTU advertises an MSS the path cannot carry, and nothing on the path fragments or reports an oversized segment. `clamp_tcp_mss` in `usid.c` lowers the MSS of every SYN and SYN-ACK at three points: `usid_egress` just before it encapsulates, `usid_egress`'s DSR reply redirect (the client's segments reach the backend encapsulated by the gateway), and `usid_ingress` just after it decapsulates (the only place a SYN-ACK returning through an egress shard can be clamped, since the shard does not). The limits are per tenant family, 1400 for IPv6 and 1420 for IPv4 at a 1500-byte fabric, and the IPv6 value also covers NAT64. `galactic-cni run` sizes them from the smallest uplink MTU (`internal/plumbing/ebpf/mssclamp`) into `mss_clamp_table`, overridable with `GALACTIC_CNI_TCP_MSS_CLAMP` (see [docs/cni/environment-variables.md](../cni/environment-variables.md#galactic_cni_tcp_mss_clamp)). The map and its counters (`mss_clamp_stats`, sized with spare slots) are new maps rather than new fields or `drop_reasons` slots, because changing an existing pinned map's layout makes `attach.Load` recreate that map empty, and the rows only CNI ADD writes do not come back until each attachment is re-added. `clamp_tcp_mss` is a global BPF function, verified once on its own, and walks at most 16 options: past about 20 steps the verifier's cost jumps from thousands of instructions to over its 1M limit. `usid_verifier_cost_test.go` fails if either program's verifier cost passes 16,000 instructions. - **A packet too big for the fabric gets an ICMP error from the datapath, not a silent drop at the uplink.** `usid_egress` calls `send_too_big` after the MSS clamp and before it encapsulates. A packet longer than `encap_mtu_table`'s limit (the uplink MTU less 40, so 1460 at a 1500-byte fabric) is rewritten in place into an ICMPv6 Packet Too Big, or an ICMPv4 Fragmentation Needed for an IPv4 tenant with DF set, and redirected back out the interface it arrived on. The error carries the limit, comes from the attachment's own IPAM gateway (`tenant_gw_table`, keyed by host-side ifindex and written by `galactic-bgp` at ADD, removed by `galactic-veth`/`galactic-tap` at DEL), and quotes the packet's first 68 bytes with NPTv6 undone, so the tenant's path MTU discovery matches it to the socket that sent it. A GSO packet is judged by its segment length. No error is sent for an IPv4 packet without DF, for an ICMP error, past a per-CPU token bucket of 1,000 a second with a burst of 100 (`pmtu_icmp_bucket`), or for an attachment with no gateway in that family; those are dropped and counted as `fib_frag_needed`. Each outcome is counted in `pmtu_stats`, exported as `galactic_usid_pmtu_packets_total{result}`, and the limit as `galactic_usid_pmtu_limit_bytes`. The same `mssclamp.Reconciler` writes `encap_mtu_table` and `mss_clamp_table` from one `GALACTIC_CNI_TCP_MSS_CLAMP` setting, so the two cannot disagree. A pod's or guest's TCP and UDP packets usually arrive `CHECKSUM_PARTIAL`, which no BPF helper can clear, and a device without checksum offload (a tap whose guest did not negotiate it) would finish that checksum into the middle of the error. So the error is laid out with room pushed at the front until the kernel's checksum spot (the original transport header plus 16 for TCP, 6 otherwise) lands on a trailing slot word, with a pad word chosen so the sum the kernel writes there equals what is already there. `usid_pmtu_tap_test.go` checks this against the kernel through a `virtio_net_hdr` tap, with offload on and off. - **CRD-driven config, no sidecar gRPC.** `galactic-bgp` writes `BGPVRFInstance`/`BGPAdvertisement` CRDs directly via a bare controller-runtime client; `galactic-router`'s reconciler picks them up. No in-node gRPC calls between any of the CNI-chain binaries and `galactic-router`. diff --git a/internal/gc/gc.go b/internal/gc/gc.go index f0f259a3..4eca087a 100644 --- a/internal/gc/gc.go +++ b/internal/gc/gc.go @@ -601,8 +601,8 @@ func SweepEBPFVRFTable(ctx context.Context, k8s client.Client, namespace, nodeNa } } - // Repair entries a live BGPVRFInstance expects but vrf_table lacks. An - // incompatible-schema reload wipes every pinned map, and nothing + // Repair entries a live BGPVRFInstance expects but vrf_table lacks. A + // reload that changes vrf_table's layout recreates it empty, and nothing // repopulates a pre-existing attachment: registerEBPFDatapath runs once, // at CNI ADD, and is never re-invoked for an attachment that already // succeeded. Reconcile above only deletes, so this is vrf_table's only @@ -649,10 +649,9 @@ func SweepEBPFVRFTable(ctx context.Context, k8s client.Client, namespace, nodeNa // vpc_attribution_table shares vrf_table's exact (Block, Argument) key // and lifecycle (see its own doc comment in usid.c), so it reconciles // against the same live set and cutoff captured above. It has no repair - // step of its own: unlike vrf_table, an incompatible-schema wipe of this - // brand-new map cannot happen on a fresh rollout, and a missing row - // simply reports as unattributed (internal/plumbing/ebpf/metrics) until - // the next CNI ADD/DEL repopulates it. + // step of its own: a missing row only reports as unattributed + // (internal/plumbing/ebpf/metrics) until the next CNI ADD/DEL repopulates + // it. attrRemoved, attrErr := reg.VPCAttribution.Reconcile(live, cutoff) for _, e := range attrRemoved { slog.Info("GC: removed stale eBPF vpc_attribution_table entry", "block", e.Block, "argument", e.Argument) diff --git a/internal/plumbing/ebpf/attach/attach.go b/internal/plumbing/ebpf/attach/attach.go index b0bdab3f..b2415a1d 100644 --- a/internal/plumbing/ebpf/attach/attach.go +++ b/internal/plumbing/ebpf/attach/attach.go @@ -19,6 +19,7 @@ import ( "golang.org/x/sys/unix" "go.datum.net/galactic/internal/config" + "go.datum.net/galactic/internal/plumbing/ebpf/mappin" "go.datum.net/galactic/internal/plumbing/ebpf/preflight" "go.datum.net/galactic/internal/plumbing/ebpf/prog" ) @@ -135,14 +136,20 @@ func Load(pinDir string) (objs *prog.UsidObjects, err error) { loadErr := spec.LoadAndAssign(&loaded, opts) if loadErr != nil && errors.Is(loadErr, ebpf.ErrMapIncompatible) { // A pin left by a previous version no longer matches the compiled map - // spec, after a changed value size or entry count, and cannot be - // reused. Every map here is control-plane-owned and reconstructable - // from CRD state, with GC sweeping anything stale, so recreating it is - // safe. Leaving this fatal would crashloop every node on the first - // schema change until an operator deleted the pins by hand. - slog.Warn("attach: pinned eBPF map incompatible with the newly compiled map spec, recreating "+ - "(control-plane state will repopulate on the next CNI ADD/GC sweep)", "pinDir", pinDir, "err", loadErr) - if unpinErr := unpinIncompatibleMaps(spec, pinDir); unpinErr != nil { + // spec, after a changed key size, value size or entry count, and + // cannot be reused. Leaving this fatal would crashloop every node on + // the first schema change until an operator deleted the pins by hand. + // + // Only the maps whose layout changed are recreated. Several maps here, + // locator_table, function_table, vrf_table, ifindex_vrf_table and + // ifindex_egress_kind_table among them, are written by CNI ADD alone. + // Recreating them empties them on a compute node until every workload + // re-attaches, which blackholes every attachment on the node. An + // unchanged map keeps its rows. + unpinned, unpinErr := mappin.UnpinIncompatible(spec, pinDir, nil) + slog.Warn("attach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them", + "pinDir", pinDir, "maps", unpinned, "err", loadErr) + if unpinErr != nil { err = fmt.Errorf("attach: recreate incompatible pinned maps: %w", unpinErr) return nil, err } @@ -185,33 +192,6 @@ func Load(pinDir string) (objs *prog.UsidObjects, err error) { // from every map name in the same directory so the two can never collide. const UsidEgressPinName = "usid_egress_prog" -// unpinIncompatibleMaps removes the on-disk pin for every map spec names, if one -// exists under pinDir, so the retry that follows an incompatible-map load creates -// each map fresh instead of failing against the stale pin again. -// -// A map with no existing pin is not an error: that one would have loaded fine. -// Clearing all of them unconditionally is simpler and equally safe, since every -// one is reconstructable from control-plane state. -func unpinIncompatibleMaps(spec *ebpf.CollectionSpec, pinDir string) error { - var errs []error - for name := range spec.Maps { - path := filepath.Join(pinDir, name) - m, err := ebpf.LoadPinnedMap(path, nil) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - continue - } - errs = append(errs, fmt.Errorf("load pinned map %q for recreation: %w", name, err)) - continue - } - if err := m.Unpin(); err != nil { - errs = append(errs, fmt.Errorf("unpin stale map %q: %w", name, err)) - } - _ = m.Close() - } - return errors.Join(errs...) -} - // Attach attaches program to the ingress hook of each named interface, through a // clsact qdisc and a direct-action BPF filter, creating the qdisc if it does not // exist. diff --git a/internal/plumbing/ebpf/attach/attach_test.go b/internal/plumbing/ebpf/attach/attach_test.go index f345d3bc..4488447e 100644 --- a/internal/plumbing/ebpf/attach/attach_test.go +++ b/internal/plumbing/ebpf/attach/attach_test.go @@ -300,8 +300,8 @@ func TestResolveFilterPriority_EnvOverride(t *testing.T) { // TestLoad_AddedMapKeepsExistingPinnedState covers rolling out a datapath build // that adds a map. The previous build's pins lack it, and that alone must not -// count as incompatible, which would recreate every map empty and drop live -// routing state until each attachment is re-added. +// count as incompatible, which would recreate maps empty and drop live routing +// state until each attachment is re-added. func TestLoad_AddedMapKeepsExistingPinnedState(t *testing.T) { requireRoot(t) diff --git a/internal/plumbing/ebpf/attach/schema_change_test.go b/internal/plumbing/ebpf/attach/schema_change_test.go new file mode 100644 index 00000000..cc7b5d83 --- /dev/null +++ b/internal/plumbing/ebpf/attach/schema_change_test.go @@ -0,0 +1,78 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package attach + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/cilium/ebpf" + + "go.datum.net/galactic/internal/plumbing/ebpf/prog" +) + +// A new version that changes one map's layout recreates that map alone. Every +// other map keeps its rows across the reload. This is the case where +// vip_xlat_table's key grew: recreating every map with it emptied the tables +// only CNI ADD writes, and every attachment on the node went dark until its +// workload re-attached. +func TestLoad_SchemaChangeRecreatesOnlyTheChangedMap(t *testing.T) { + requireRoot(t) + pinDir := filepath.Join("/sys/fs/bpf", fmt.Sprintf("galactic-schema-test-%d", os.Getpid())) + t.Cleanup(func() { _ = os.RemoveAll(pinDir) }) + + objs, err := Load(pinDir) + if err != nil { + t.Fatalf("first Load: %v", err) + } + vrfKey := uint64(0x123) + if err := objs.VrfTable.Put(vrfKey, prog.UsidVrfValue{VrfTableId: 42}); err != nil { + t.Fatalf("seed vrf_table: %v", err) + } + if err := objs.LocatorTable.Put(uint64(0x456), prog.UsidLocatorValue{Generation: 1}); err != nil { + t.Fatalf("seed locator_table: %v", err) + } + newKeySize := objs.VipXlatTable.KeySize() + _ = objs.Close() + + // Stand in for the previous version's vip_xlat_table: same map, older, + // smaller key. + vipPin := filepath.Join(pinDir, prog.UsidMapVipXlatTable) + if err := os.Remove(vipPin); err != nil { + t.Fatalf("remove vip_xlat_table pin: %v", err) + } + old, err := ebpf.NewMap(&ebpf.MapSpec{ + Name: "vip_xlat_table", Type: ebpf.Hash, KeySize: newKeySize / 2, ValueSize: 8, MaxEntries: 16, + }) + if err != nil { + t.Fatalf("create old-layout vip_xlat_table: %v", err) + } + if err := old.Pin(vipPin); err != nil { + t.Fatalf("pin old-layout vip_xlat_table: %v", err) + } + _ = old.Close() + + objs, err = Load(pinDir) + if err != nil { + t.Fatalf("Load over an old-layout vip_xlat_table: %v", err) + } + defer objs.Close() //nolint:errcheck // test-owned objects + + if got := objs.VipXlatTable.KeySize(); got != newKeySize { + t.Errorf("vip_xlat_table key size = %d after reload, want the new layout's %d", got, newKeySize) + } + var vrf prog.UsidVrfValue + if err := objs.VrfTable.Lookup(vrfKey, &vrf); err != nil { + t.Errorf("vrf_table lost its row across the reload: %v", err) + } else if vrf.VrfTableId != 42 { + t.Errorf("vrf_table row = table %d, want 42", vrf.VrfTableId) + } + var loc prog.UsidLocatorValue + if err := objs.LocatorTable.Lookup(uint64(0x456), &loc); err != nil { + t.Errorf("locator_table lost its row across the reload: %v", err) + } +} diff --git a/internal/plumbing/ebpf/edgeattach/attach.go b/internal/plumbing/ebpf/edgeattach/attach.go index ed29cdaf..a5137cef 100644 --- a/internal/plumbing/ebpf/edgeattach/attach.go +++ b/internal/plumbing/ebpf/edgeattach/attach.go @@ -19,6 +19,7 @@ import ( "go.datum.net/galactic/internal/plumbing/bond" "go.datum.net/galactic/internal/plumbing/ebpf/edgepreflight" "go.datum.net/galactic/internal/plumbing/ebpf/edgeprog" + "go.datum.net/galactic/internal/plumbing/ebpf/mappin" "go.datum.net/galactic/internal/plumbing/ebpf/xdpattach" ) @@ -103,9 +104,11 @@ func Load(pinDir string, dispatchMaps map[string]*ebpf.Map) (*edgeprog.EdgedsrOb // startup. A stale pin from an incompatible layout is safe to // recreate rather than fatal. The dispatcher's maps are not ours and // are never touched here. - slog.Warn("edgeattach: pinned eBPF map incompatible with the newly compiled map spec, recreating "+ - "(control-plane state will repopulate on the next NetworkRule reconcile)", "pinDir", pinDir, "err", loadErr) - if unpinErr := unpinIncompatibleMaps(spec, pinDir); unpinErr != nil { + unpinned, unpinErr := mappin.UnpinIncompatible(spec, pinDir, func(name string) bool { return dispatchMapNames[name] }) + slog.Warn("edgeattach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them "+ + "(control-plane state will repopulate on the next NetworkRule reconcile)", + "pinDir", pinDir, "maps", unpinned, "err", loadErr) + if unpinErr != nil { return nil, fmt.Errorf("edgeattach: recreate incompatible pinned maps: %w", unpinErr) } loadErr = spec.LoadAndAssign(&loaded, opts) @@ -120,31 +123,6 @@ func Load(pinDir string, dispatchMaps map[string]*ebpf.Map) (*edgeprog.EdgedsrOb return &loaded, nil } -// unpinIncompatibleMaps mirrors internal/plumbing/ebpf/attach's identical -// helper -- see that function's doc comment. -func unpinIncompatibleMaps(spec *ebpf.CollectionSpec, pinDir string) error { - var errs []error - for name := range spec.Maps { - if dispatchMapNames[name] { - continue - } - path := filepath.Join(pinDir, name) - m, err := ebpf.LoadPinnedMap(path, nil) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - continue - } - errs = append(errs, fmt.Errorf("load pinned map %q for recreation: %w", name, err)) - continue - } - if err := m.Unpin(); err != nil { - errs = append(errs, fmt.Errorf("unpin stale map %q: %w", name, err)) - } - _ = m.Close() - } - return errors.Join(errs...) -} - // ResolveTargets resolves ifaceName to the interface names Attach should // actually attach the XDP program to. // diff --git a/internal/plumbing/ebpf/mappin/mappin.go b/internal/plumbing/ebpf/mappin/mappin.go new file mode 100644 index 00000000..8be020e6 --- /dev/null +++ b/internal/plumbing/ebpf/mappin/mappin.go @@ -0,0 +1,70 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +// Package mappin recovers a pinned eBPF collection whose map layout changed +// between versions. +// +// Loading a collection with pin-by-name reuses each existing pin, and fails +// with ebpf.ErrMapIncompatible when any one pin no longer matches the compiled +// spec. The recovery is to remove that pin so the retry creates the map fresh. +// Removing only the pins that no longer match matters: every map holds state +// some writer put there, and a map whose layout did not change has no reason to +// lose it. Some of that state, such as the uSID datapath's per-attachment rows, +// is written once at CNI ADD and never again, so recreating a map that did not +// need it blackholes traffic until every workload on the node re-attaches. +package mappin + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "sort" + + "github.com/cilium/ebpf" +) + +// UnpinIncompatible removes the pin under pinDir of every map in spec whose +// pinned copy no longer matches spec, and returns their names, sorted. A map +// with no pin, or whose pin still matches, is left alone. skip, when non-nil, +// excludes maps the caller does not own. +func UnpinIncompatible(spec *ebpf.CollectionSpec, pinDir string, skip func(name string) bool) ([]string, error) { + names := make([]string, 0, len(spec.Maps)) + for name := range spec.Maps { + if skip == nil || !skip(name) { + names = append(names, name) + } + } + sort.Strings(names) + + var unpinned []string + var errs []error + for _, name := range names { + m, err := ebpf.LoadPinnedMap(filepath.Join(pinDir, name), nil) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + continue + } + errs = append(errs, fmt.Errorf("load pinned map %q: %w", name, err)) + continue + } + compatErr := spec.Maps[name].Compatible(m) + if compatErr == nil { + _ = m.Close() + continue + } + if !errors.Is(compatErr, ebpf.ErrMapIncompatible) { + errs = append(errs, fmt.Errorf("check pinned map %q: %w", name, compatErr)) + _ = m.Close() + continue + } + if err := m.Unpin(); err != nil { + errs = append(errs, fmt.Errorf("unpin incompatible map %q: %w", name, err)) + } else { + unpinned = append(unpinned, name) + } + _ = m.Close() + } + return unpinned, errors.Join(errs...) +} diff --git a/internal/plumbing/ebpf/mappin/mappin_test.go b/internal/plumbing/ebpf/mappin/mappin_test.go new file mode 100644 index 00000000..0f76781e --- /dev/null +++ b/internal/plumbing/ebpf/mappin/mappin_test.go @@ -0,0 +1,125 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package mappin + +import ( + "fmt" + "os" + "path/filepath" + "slices" + "testing" + + "github.com/cilium/ebpf" + "github.com/cilium/ebpf/rlimit" +) + +func requireRoot(t *testing.T) { + t.Helper() + if os.Geteuid() != 0 { + t.Skip("test requires root to create and pin BPF maps; re-run via sudo") + } + if err := rlimit.RemoveMemlock(); err != nil { + t.Fatalf("rlimit.RemoveMemlock: %v", err) + } +} + +// pinDir returns a fresh directory on the host's bpffs, removed on cleanup. +func pinDir(t *testing.T) string { + t.Helper() + dir := filepath.Join("/sys/fs/bpf", fmt.Sprintf("galactic-mappin-test-%d", os.Getpid())) + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Skipf("no writable bpffs at /sys/fs/bpf: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + return dir +} + +func hashSpec(name string, keySize uint32) *ebpf.MapSpec { + return &ebpf.MapSpec{Name: name, Type: ebpf.Hash, KeySize: keySize, ValueSize: 8, MaxEntries: 16} +} + +// pin creates spec's map and pins it under dir by its name, holding value +// under key so a test can tell a kept map from a recreated one. +func pin(t *testing.T, dir string, spec *ebpf.MapSpec, key, value []byte) { + t.Helper() + m, err := ebpf.NewMap(spec) + if err != nil { + t.Fatalf("create map %s: %v", spec.Name, err) + } + defer m.Close() //nolint:errcheck // test-owned descriptor + if err := m.Put(key, value); err != nil { + t.Fatalf("seed map %s: %v", spec.Name, err) + } + if err := m.Pin(filepath.Join(dir, spec.Name)); err != nil { + t.Fatalf("pin map %s: %v", spec.Name, err) + } +} + +// One map changing shape removes only that map's pin. The unchanged map keeps +// its pin and contents, a map the caller skips is left alone even though it +// changed, and a map with no pin yet is not an error. +func TestUnpinIncompatible_RemovesOnlyChangedMaps(t *testing.T) { + requireRoot(t) + dir := pinDir(t) + + value := []byte{1, 2, 3, 4, 5, 6, 7, 8} + pin(t, dir, hashSpec("kept", 8), make([]byte, 8), value) + pin(t, dir, hashSpec("grown", 16), make([]byte, 16), value) + pin(t, dir, hashSpec("foreign", 16), make([]byte, 16), value) + + spec := &ebpf.CollectionSpec{Maps: map[string]*ebpf.MapSpec{ + "kept": hashSpec("kept", 8), + "grown": hashSpec("grown", 32), + "foreign": hashSpec("foreign", 32), + "new": hashSpec("new", 8), + }} + + unpinned, err := UnpinIncompatible(spec, dir, func(name string) bool { return name == "foreign" }) + if err != nil { + t.Fatalf("UnpinIncompatible: %v", err) + } + if !slices.Equal(unpinned, []string{"grown"}) { + t.Errorf("unpinned = %v, want [grown]", unpinned) + } + + if _, err := os.Stat(filepath.Join(dir, "grown")); !os.IsNotExist(err) { + t.Errorf("grown's pin still exists (stat err %v), want it removed", err) + } + if _, err := os.Stat(filepath.Join(dir, "foreign")); err != nil { + t.Errorf("foreign's pin is gone (%v), want a skipped map left alone", err) + } + + kept, err := ebpf.LoadPinnedMap(filepath.Join(dir, "kept"), nil) + if err != nil { + t.Fatalf("kept's pin is gone: %v", err) + } + defer kept.Close() //nolint:errcheck // test-owned descriptor + got := make([]byte, 8) + if err := kept.Lookup(make([]byte, 8), &got); err != nil { + t.Fatalf("kept lost its row: %v", err) + } + if !slices.Equal(got, value) { + t.Errorf("kept's row = %v, want %v", got, value) + } +} + +// With every pin matching, nothing is removed. +func TestUnpinIncompatible_CompatiblePinsUntouched(t *testing.T) { + requireRoot(t) + dir := pinDir(t) + pin(t, dir, hashSpec("a", 8), make([]byte, 8), make([]byte, 8)) + + spec := &ebpf.CollectionSpec{Maps: map[string]*ebpf.MapSpec{"a": hashSpec("a", 8)}} + unpinned, err := UnpinIncompatible(spec, dir, nil) + if err != nil { + t.Fatalf("UnpinIncompatible: %v", err) + } + if len(unpinned) != 0 { + t.Errorf("unpinned = %v, want none", unpinned) + } + if _, err := os.Stat(filepath.Join(dir, "a")); err != nil { + t.Errorf("a's pin is gone: %v", err) + } +} diff --git a/internal/plumbing/ebpf/natattach/attach.go b/internal/plumbing/ebpf/natattach/attach.go index 200d1164..bef6216c 100644 --- a/internal/plumbing/ebpf/natattach/attach.go +++ b/internal/plumbing/ebpf/natattach/attach.go @@ -9,7 +9,6 @@ import ( "fmt" "log/slog" "os" - "path/filepath" "github.com/cilium/ebpf" "github.com/cilium/ebpf/link" @@ -17,6 +16,7 @@ import ( "github.com/vishvananda/netlink" "go.datum.net/galactic/internal/plumbing/bond" + "go.datum.net/galactic/internal/plumbing/ebpf/mappin" "go.datum.net/galactic/internal/plumbing/ebpf/natprog" "go.datum.net/galactic/internal/plumbing/ebpf/xdpattach" ) @@ -65,9 +65,10 @@ func Load(pinDir string) (*natprog.NatObjects, error) { // pin from an incompatible layout is safe to recreate. A compatible // restart keeps every session; this one drops them, and each flow's // next packet claims a new one. - slog.Warn("natattach: pinned eBPF map incompatible with the newly compiled map spec, recreating "+ - "(control-plane state will repopulate at next startup)", "pinDir", pinDir, "err", loadErr) - if unpinErr := unpinIncompatibleMaps(spec, pinDir); unpinErr != nil { + unpinned, unpinErr := mappin.UnpinIncompatible(spec, pinDir, nil) + slog.Warn("natattach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them "+ + "(control-plane state will repopulate at next startup)", "pinDir", pinDir, "maps", unpinned, "err", loadErr) + if unpinErr != nil { return nil, fmt.Errorf("natattach: recreate incompatible pinned maps: %w", unpinErr) } loadErr = spec.LoadAndAssign(&loaded, opts) @@ -82,28 +83,6 @@ func Load(pinDir string) (*natprog.NatObjects, error) { return &loaded, nil } -// unpinIncompatibleMaps mirrors internal/plumbing/ebpf/edgeattach's -// identical helper -- see that function's doc comment. -func unpinIncompatibleMaps(spec *ebpf.CollectionSpec, pinDir string) error { - var errs []error - for name := range spec.Maps { - path := filepath.Join(pinDir, name) - m, err := ebpf.LoadPinnedMap(path, nil) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - continue - } - errs = append(errs, fmt.Errorf("load pinned map %q for recreation: %w", name, err)) - continue - } - if err := m.Unpin(); err != nil { - errs = append(errs, fmt.Errorf("unpin stale map %q: %w", name, err)) - } - _ = m.Close() - } - return errors.Join(errs...) -} - // PopulateProgArray fills the nat_progs tail-call array with every // translation leaf the dispatcher hands packets to. // diff --git a/internal/plumbing/ebpf/prog/usid.c b/internal/plumbing/ebpf/prog/usid.c index 60860aec..97d3ff23 100644 --- a/internal/plumbing/ebpf/prog/usid.c +++ b/internal/plumbing/ebpf/prog/usid.c @@ -782,8 +782,8 @@ struct { // cannot describe a VPC mixing tap and veth attachments on one node. // // A separate map rather than a field on ifindex_vrf_value: changing a pinned -// map's layout makes the loader recreate every map empty, wiping live routing -// state on every node until each attachment is re-added. +// map's layout makes the loader recreate that map empty, and ifindex_vrf_table's +// rows come back only when each attachment is re-added. // // A miss uses plain bpf_redirect, which delivers to either kind. Into a veth it // transmits on the host-side end, whose xmit forwards into the peer's @@ -951,7 +951,7 @@ struct mss_clamp_value { // // Its own map rather than a field on any existing one, so adding it does not // change an existing pinned map's layout. That would make the loader recreate -// every pinned map empty (see attach.Load). +// that map empty (see attach.Load). struct { __uint(type, BPF_MAP_TYPE_ARRAY); __uint(max_entries, 1); @@ -961,7 +961,7 @@ struct { // enum mss_clamp_stat indexes mss_clamp_stats, which is observability only. // Not drop_reasons slots, for the same reason mss_clamp_table is its own map: -// growing drop_reasons' entry count would recreate every pinned map. +// growing drop_reasons' entry count would recreate it empty. enum mss_clamp_stat { // A SYN's MSS was above the limit and was rewritten to it. MSS_CLAMP_STAT_CLAMPED_IPV4 = 0, @@ -990,7 +990,7 @@ enum mss_clamp_stat { // mss_clamp_stats is sized above __MSS_CLAMP_STAT_COUNT on purpose. A new stat // then fits in the existing map, rather than changing the pinned map's entry -// count and recreating every map with it. +// count and recreating it empty. #define USID_MSS_CLAMP_STATS_SLOTS 16 struct { From 01ba03d3315c81589fabb158cad587a64454373a Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 22:36:38 -0400 Subject: [PATCH 2/2] fix: Refuse a map recovery that would recreate nothing If the load reports an incompatible map but no pin under the directory fails MapSpec.Compatible, retrying cannot succeed. The loaders now return the original error, naming the directory, instead of logging "recreating them" with an empty list and failing the same way again. The warning is logged only after the unpin succeeds, and a failed retry is wrapped with the maps that were recreated, so the final error says what the recovery changed. The package doc now says only a map's shape is compared, so a change to what a same-size value means needs a new map name. Related to #774 Co-Authored-By: Claude Opus 5.5 --- internal/plumbing/ebpf/attach/attach.go | 13 ++++++++++--- internal/plumbing/ebpf/edgeattach/attach.go | 12 +++++++++--- internal/plumbing/ebpf/mappin/mappin.go | 5 +++++ internal/plumbing/ebpf/natattach/attach.go | 12 +++++++++--- 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/internal/plumbing/ebpf/attach/attach.go b/internal/plumbing/ebpf/attach/attach.go index b2415a1d..bc037912 100644 --- a/internal/plumbing/ebpf/attach/attach.go +++ b/internal/plumbing/ebpf/attach/attach.go @@ -147,13 +147,20 @@ func Load(pinDir string) (objs *prog.UsidObjects, err error) { // re-attaches, which blackholes every attachment on the node. An // unchanged map keeps its rows. unpinned, unpinErr := mappin.UnpinIncompatible(spec, pinDir, nil) - slog.Warn("attach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them", - "pinDir", pinDir, "maps", unpinned, "err", loadErr) if unpinErr != nil { err = fmt.Errorf("attach: recreate incompatible pinned maps: %w", unpinErr) return nil, err } - loadErr = spec.LoadAndAssign(&loaded, opts) + if len(unpinned) == 0 { + err = fmt.Errorf("attach: load reported an incompatible map, but every pin under %s matches: %w", + pinDir, loadErr) + return nil, err + } + slog.Warn("attach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them", + "pinDir", pinDir, "maps", unpinned, "err", loadErr) + if loadErr = spec.LoadAndAssign(&loaded, opts); loadErr != nil { + loadErr = fmt.Errorf("after recreating %v: %w", unpinned, loadErr) + } } if loadErr != nil { var ve *ebpf.VerifierError diff --git a/internal/plumbing/ebpf/edgeattach/attach.go b/internal/plumbing/ebpf/edgeattach/attach.go index a5137cef..555c908b 100644 --- a/internal/plumbing/ebpf/edgeattach/attach.go +++ b/internal/plumbing/ebpf/edgeattach/attach.go @@ -105,13 +105,19 @@ func Load(pinDir string, dispatchMaps map[string]*ebpf.Map) (*edgeprog.EdgedsrOb // recreate rather than fatal. The dispatcher's maps are not ours and // are never touched here. unpinned, unpinErr := mappin.UnpinIncompatible(spec, pinDir, func(name string) bool { return dispatchMapNames[name] }) + if unpinErr != nil { + return nil, fmt.Errorf("edgeattach: recreate incompatible pinned maps: %w", unpinErr) + } + if len(unpinned) == 0 { + return nil, fmt.Errorf("edgeattach: load reported an incompatible map, but every pin under %s matches: %w", + pinDir, loadErr) + } slog.Warn("edgeattach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them "+ "(control-plane state will repopulate on the next NetworkRule reconcile)", "pinDir", pinDir, "maps", unpinned, "err", loadErr) - if unpinErr != nil { - return nil, fmt.Errorf("edgeattach: recreate incompatible pinned maps: %w", unpinErr) + if loadErr = spec.LoadAndAssign(&loaded, opts); loadErr != nil { + loadErr = fmt.Errorf("after recreating %v: %w", unpinned, loadErr) } - loadErr = spec.LoadAndAssign(&loaded, opts) } if loadErr != nil { var ve *ebpf.VerifierError diff --git a/internal/plumbing/ebpf/mappin/mappin.go b/internal/plumbing/ebpf/mappin/mappin.go index 8be020e6..8430f781 100644 --- a/internal/plumbing/ebpf/mappin/mappin.go +++ b/internal/plumbing/ebpf/mappin/mappin.go @@ -13,6 +13,11 @@ // lose it. Some of that state, such as the uSID datapath's per-attachment rows, // is written once at CNI ADD and never again, so recreating a map that did not // need it blackholes traffic until every workload on the node re-attaches. +// +// Only a map's shape is compared: its type, key size, value size, entry count +// and flags. A map that keeps its shape keeps its rows, so a change to what +// those rows mean, such as a repurposed field of the same size, needs a new map +// name rather than an edit to the existing one. package mappin import ( diff --git a/internal/plumbing/ebpf/natattach/attach.go b/internal/plumbing/ebpf/natattach/attach.go index bef6216c..ffc7205d 100644 --- a/internal/plumbing/ebpf/natattach/attach.go +++ b/internal/plumbing/ebpf/natattach/attach.go @@ -66,12 +66,18 @@ func Load(pinDir string) (*natprog.NatObjects, error) { // restart keeps every session; this one drops them, and each flow's // next packet claims a new one. unpinned, unpinErr := mappin.UnpinIncompatible(spec, pinDir, nil) - slog.Warn("natattach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them "+ - "(control-plane state will repopulate at next startup)", "pinDir", pinDir, "maps", unpinned, "err", loadErr) if unpinErr != nil { return nil, fmt.Errorf("natattach: recreate incompatible pinned maps: %w", unpinErr) } - loadErr = spec.LoadAndAssign(&loaded, opts) + if len(unpinned) == 0 { + return nil, fmt.Errorf("natattach: load reported an incompatible map, but every pin under %s matches: %w", + pinDir, loadErr) + } + slog.Warn("natattach: pinned eBPF maps incompatible with the newly compiled map spec, recreating them "+ + "(control-plane state will repopulate at next startup)", "pinDir", pinDir, "maps", unpinned, "err", loadErr) + if loadErr = spec.LoadAndAssign(&loaded, opts); loadErr != nil { + loadErr = fmt.Errorf("after recreating %v: %w", unpinned, loadErr) + } } if loadErr != nil { var ve *ebpf.VerifierError