spike: cilium 1.17.16 → 1.19.6 (DO NOT MERGE) - #302
Conversation
Sizing spike for the cilium bump that gates #263, #273 and #287. Findings so far: 1. pkg/maps/lbmap no longer exists in 1.19.x. The types we use moved to pkg/loadbalancer/maps; aliasing the import as `lbmap` keeps the diff to one line. 2. Backend4Value / Backend6Value -- the *V2* backend map value layouts -- were deleted outright. Only V3 survives. Our code deliberately probed cilium_lb4_backends_v2 before falling back to v3, so this drops the ability to read the V2 layout from an older deployed Cilium. That is a functional regression, not a mechanical one, and is the open question on this branch. 3. Everything else we touch survives unchanged: bpf.OpenMap, ctmap.*, tuple.*, u8proto.*, loadbalancer.BackendID, NewBackend{4,6}KeyV3, the BackendValue interface, TUPLE_F_SERVICE. 4. cilium 1.19.6 pulls k8s.io/* 0.32.13 -> 0.35.6, which is what #263 and #273 need, and cilium/ebpf 0.20.0 -> 0.22.0, which is #287. That last point couples things: 1.19.6 *forces* ebpf 0.22.0, and the coroot/pyroscope/ebpf fork pinned via `replace` still calls spec.RewriteConstants and btf.FlushKernelSpec, both removed in 0.22. So the cilium bump cannot land without fixing that fork first -- the two are one change, not two.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the Cilium dependency to version 1.19.6 and the cilium/ebpf library to v0.22.0, alongside several other dependency updates in go.mod. Consequently, in containers/cilium.go, the deprecated V2 backend map definitions and value types have been removed since they are no longer supported in Cilium 1.19, leaving only the V3 layouts. There are no review comments provided, and I have no additional feedback on these changes.
My local macOS check was worthless here: containers/ failed on NVML
(cgo off) before cilium.go was ever type-checked, so it reported a clean
cilium migration that was not clean. CI on Linux with cgo found four
breaks, all in containers/cilium.go:
:116,:154 e.BackendID undefined (*ctmap.CtEntry has no field BackendID)
:128,:166 backend.GetAddress() returns clustermesh/types.AddrCluster,
not net.IP
Both have small fixes:
e.BackendID -> e.Union0[1]
backend.GetAddress() -> backend.GetAddress().AsNetIP()
CtEntry replaced `Reserved0 uint64 + BackendID uint64` with a union,
`Union0 [2]uint64`. Cilium's own CtEntry.String() prints BackendID from
c.Union0[1] (pkg/maps/ctmap/types.go:401,413), so index 1 is right --
but this is now an untyped union access with no compiler protection if
the layout shifts again.
Note also IfIndex (uint16) became NatPort (uint16) at the same offset.
Struct size is unchanged, so a version mismatch between this library and
the deployed Cilium datapath misreads fields silently rather than
failing. That reinforces the open question on this branch: we parse
these structs straight out of bpffs, so the library version is coupled
to the Cilium actually running in the cluster.
CI turned up a third blocker after the cilium fixes:
containerd/cgroups@v1.1.0/v2/devicefilter.go:170: asm.Instruction has
no field or method Sym
cilium/ebpf 0.22 replaced asm.Instruction.Sym() with WithSymbol(), and
cgroups v1.1.0 still calls the old one. cgroups/v3 already uses
WithSymbol (cgroup2/devicefilter.go:170,183), so moving to v3.1.3 fixes
it.
cgroups v1.1.0 was a *direct* require here used by exactly one file,
ebpftracer/tracer_test.go. Migrated:
cgroups.New(cgroups.V1, cgroups.StaticPath(..), ..)
-> cgroup1.New(cgroup1.StaticPath(..), ..) // hierarchy arg dropped
cgroups.Process -> cgroup1.Process
cgroups/v2 -> cgroups/v3/cgroup2 // same call shapes
Mode()/Legacy/Hybrid/Unified stay on the root package.
Side effect worth noting: cgroups/v3 pulls runtime-spec 1.1.0 -> 1.3.0,
which is exactly what PR #285 was asking for. #285 is therefore a
consequence of this work, not the separate containerd v2 migration I
assumed -- though containerd/containerd v1.7.33 still drags cgroups
v1.1.0 in as an indirect dep, so whether that is fully resolved is what
this push is testing.
Jumping straight to cgroups/v3 v3.1.3 fixed the ebpf 0.22 asm break but dragged runtime-spec 1.1.0 -> 1.3.0, which then broke containerd: containerd@v1.7.33/oci/spec_opts.go:1548:34: cannot use limit (variable of type int64) as *int64 value in assignment That is PR #285's original failure: runtime-spec 1.2.0 changed LinuxPids.Limit to *int64, and containerd v1.7.x is built against 1.1.0. containerd v1.7.34 (latest v1.7) still requires runtime-spec v1.1.0, so only containerd/v2 escapes it -- a much bigger migration than this bump needs to carry. v3.0.3 avoids the whole problem: it already uses asm WithSymbol (cgroup2/devicefilter.go:170,183), so it fixes the ebpf 0.22 break, but only requires runtime-spec v1.0.2 so nothing forces containerd forward. It was also already in the module graph as an indirect dep, so this pins what we were resolving to anyway. cgroup1.New / cgroup2.NewManager have identical signatures to v3.1.3, so the test migration is unchanged. Consequence: #285 (runtime-spec 1.3.0) is NOT resolved by this branch and genuinely does need the containerd v2 migration. It is decoupled from the cilium critical path, which is the point.
Spike complete — final resultCI on Blockers found and resolved on this branch
Resulting versions: What this means for the open dependabot PRs
Remaining workOne ~10-line patch to spec.RewriteConstants(map[string]interface{}{"global_config": X})
→ spec.Variables["global_config"].Set(X) // both sites rewrite exactly one constant
btf.FlushKernelSpec() → delete
Note the latest upstream Still unanswered (blocking, not technical)
Still DO NOT MERGE — this is a sizing artifact. |
Follow-up: the V2 backend-map question is effectively mootI flagged "dropping V2 backend-map support is a functional regression" as the blocking open question. Having traced it through cilium source, the practical risk is close to zero. V3 backend maps have existed since cilium 1.12. Checked directly:
Cilium actively migrates V2 → V3 and then deletes the V2 map. From v2BackendMapExistsV4 = lbmap.Backend4MapV2.Open() == nil
...
log.Info("Backend map v2 exists. Migrating entries to backend map v3.")
...
// V2 backend map will be removed from bpffs at this point,
// the map will be actually removed once the last program
// referencing it has been removed.
err = v2Map.Close()Only So a V2 backend map exists only transiently, on a cluster mid-upgrade from cilium ≤1.11. Cilium 1.11 went EOL in 2023. Dropping V2 read support is safe for anything realistically deployed. Bonus: the V2-first probe order looks like a latent bug today
for _, n := range []string{lbmap.Backend4MapV2Name, lbmap.Backend4MapV3Name} {
Removing V2 on this branch happens to fix that too. Where that leaves thingsOf the two blocking questions in the previous comment, #1 is answered: the V2 removal is fine. #2 stands unchanged and is the more serious one — |
1 participant
Draft sizing spike, not for merge. Opened so CI enumerates the real break list on Linux with cgo.
Why
cilium/cilium v1.17.16is the single constraint behind three blocked PRs. Its go.mod holdsk8s.io/*at 0.32.x andcilium/ebpfat 0.20.0. v1.19.6 moves those to 0.35.6 and 0.22.0 respectively — which is exactly what #263, #273 and #287 each need.v1.18.x is not a viable target: it carries
replace k8s.io/apimachinery => github.com/cilium/apimachinery(a private fork).replacedirectives in a dependency are ignored by the main module, so we would build against unpatched apimachinery.What this branch does
go get github.com/cilium/cilium@v1.19.6(dragsk8s.io/*→ 0.35.6,cilium/ebpf→ 0.22.0)pkg/maps/lbmap→pkg/loadbalancer/maps, aliased aslbmapso the diff stays smallFindings
The cilium API migration is nearly free. Everything we touch survives:
bpf.OpenMap,ctmap.*,tuple.*,u8proto.*,loadbalancer.BackendID,NewBackend{4,6}KeyV3, theBackendValueinterface,TUPLE_F_SERVICE. Only the import path changed.One real regression.
Backend4Value/Backend6Value— the V2 backend map value layouts — are deleted in 1.19, not moved.containers/cilium.godeliberately probedcilium_lb4_backends_v2before falling back to_v3, so this branch loses the ability to read the V2 layout. For a cluster running an older Cilium that still writes V2 backend maps, service-backend resolution would go dark. This is the open question — it needs an answer about the oldest Cilium version we intend to support, not a code fix.This bump and #287 are one change, not two. 1.19.6 requires
cilium/ebpf v0.22.0, and thecoroot/pyroscope/ebpffork pinned viareplacestill calls APIs removed in 0.22:Worth noting the latest upstream
coroot/pyroscopecommit (9cce3f9, 2026-04-13) still calls both and still pinscilium/ebpf v0.17.3— so repointing thereplaceat a newer commit does not fix this. The fork needs source changes:RewriteConstants→ theVariables/VariableSpecAPI, andbtf.FlushKernelSpechas no replacement.Expected CI result
Should fail on the pyroscope errors above and nothing else. If any cilium error appears, the migration is more than an import rename and this write-up is wrong.
Not addressed here
ctmap/lbmapstructs out of bpffs, so the library version needs to track the Cilium actually deployed, which compilation cannot tell us