[sample #10736] eBPF: Hybrid conntrack cleaner#199
Conversation
Mirrors the diff the human reviewers first saw on projectcalico#10736 ('eBPF: Hybrid conntrack cleaner' by @sridhartigera) for the Council of Claudes benchmark. base(fork)=ff1cb542767734dbb433d129e7f6f94e2e72774d head(as-reviewed)=dad1adeeb46fcf9084f85100ab7b17fb5bbf511c
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ctLogLevel = bpfconntrack.BPFLogLevelDebug | ||
| } | ||
|
|
||
| bpfCleaner, err := conntrack.NewBPFProgCleaner(int(ipFamily), config.BPFConntrackTimeouts, ctLogLevel) |
There was a problem hiding this comment.
🔎 Correctness — Creating the BPF cleaner is optional, but the scanner no longer has a fallback deletion path; if NewBPFProgCleaner() fails, bpfCleaner stays nil and the scanner will enqueue entries into the CCQ map but never delete them (since it doesn’t inline-delete during iteration).
- Impact: Expired entries will leak permanently on kernels/environments where the cleaner can’t be created or run.
- Suggested fix: Provide a fallback path. For example:
- If bpfCleaner creation fails, configure the scanner to delete inline during iteration (return maps.IterDelete on Delete verdict), or
- In the scan loop, if s.bpfCleaner == nil (or if Run() returns an error), fall back to deleting the CT entry immediately instead of (or in addition to) enqueuing into the CCQ map.
| val = cleanupv1.ValueV6{} | ||
| } | ||
|
|
||
| for _, scanner := range s.scanners { |
There was a problem hiding this comment.
🔎 Correctness — The scanner no longer deletes from the CT map inline (it used to return IterDelete). It now only enqueues into the CCQ map. If bpfCleaner is nil or Run() fails, the entries won’t be removed.
- Impact: Without a working BPF cleaner run, nothing deletes the marked entries, causing leaks.
- Suggested fix: When verdict == ScanVerdictDelete:
- If s.bpfCleaner is nil, return maps.IterDelete (old behavior) or directly call s.ctMap.Delete(k).
- If bpfCleaner.Run() returns an error later, consider retrying inline deletion or clear the CCQ map entries to avoid growth.
| } | ||
| if (cali_ct_delete_elem(rev_key) == 0) { | ||
| ictx->num_kvs_deleted_nat_rev++; | ||
| cali_ccq_delete_elem(key); |
There was a problem hiding this comment.
🔎 Correctness — The cleanup queue (CCQ) entry is only deleted from the CCQ map when the CT deletion succeeds; otherwise, it remains in the CCQ map. There are also early returns above (e.g., at lines 52, 58, and 70) that exit without removing the CCQ entry.
- Impact: Stale CCQ entries accumulate (map leakage), blocking future queueing once the CCQ map fills, and causing repeated futile reprocessing since the stored timestamps will never match.
- Suggested fix: Always remove the CCQ map entry at the end of processing regardless of whether the CT entry was deleted. For example:
- Unconditionally call cali_ccq_delete_elem(key) before returning (including in early-return paths at lines 52/58/70).
- If you want to retry later, rely on userspace to re-enqueue with updated timestamps rather than keeping stale CCQ entries.
| // process_ccq_entry processes an entry in the "cleanup queue" map. The map | ||
| // is keyed on the reverse NAT entry's key, with the value storing the forward | ||
| // entry's key (or a zero value if the forward entry is missing). | ||
| static long process_ccq_entry(void *map, struct calico_ct_key *key, struct cali_ccq_value *value, void *ctx) |
There was a problem hiding this comment.
🔎 Correctness — process_ccq_entry’s signature was changed to use concrete types and remove const from key:
static long process_ccq_entry(void *map, struct calico_ct_key *key, struct cali_ccq_value *value, void *ctx)
However, bpf_for_each_map_elem expects a callback of type:
long (*callback)(void *map, const void *key, void *value, void *ctx)
- Impact: Mismatched prototype can cause verifier or compilation issues (and violates const correctness), depending on compiler and macros.
- Suggested fix: Restore the expected prototype and cast inside the function:
static long process_ccq_entry(void *map, const void *key, void *value, void *ctx)
{
const struct calico_ct_key *ckey = key;
struct cali_ccq_value *cval = value;
...
}
There was a problem hiding this comment.
🔎 Council of Claudes — Correctness
Note
Correctness lens · bugs · completeness · concurrency · edge cases
3 potential correctness issues
- The userspace scanner no longer deletes entries during its iteration and now relies entirely on the BPF cleaner. If the BPF cleaner fails to be created or to run, expired entries will not be deleted at all, causing conntrack leaks.
- Entries placed into the cleanup queue (CCQ) are only removed from that map when deletion succeeds. If deletion does not proceed (timestamps don’t match or reverse is missing), the CCQ entry is left behind indefinitely, causing the CCQ map to fill up and future cleanups to be blocked.
- The eBPF callback’s function signature no longer matches the expected bpf_for_each_map_elem prototype, which can result in verifier or compile/link-time issues.
🤖 Council of Claudes · 4 inline comment(s)
| // process_ccq_entry processes an entry in the "cleanup queue" map. The map | ||
| // is keyed on the reverse NAT entry's key, with the value storing the forward | ||
| // entry's key (or a zero value if the forward entry is missing). | ||
| static long process_ccq_entry(void *map, struct calico_ct_key *key, struct cali_ccq_value *value, void *ctx) |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The callback signature to bpf_for_each_map_elem is conventionally (void *map, const void key, void value, void ctx). Here the function uses strongly typed arguments (struct calico_ct_key, struct cali_ccq_value). While clang will accept it, it obscures that the helper calls with void. Suggest using the standard signature and doing explicit casts at the top for readability and to match kernel idioms:
- static long process_ccq_entry(void *map, const void *key, void *value, void *ctx)
- const struct calico_ct_key *rev_key = key;
- struct cali_ccq_value *ccq_val = value;
| const struct calico_ct_key *ct_key = key; | ||
| struct calico_ct_value *ct_value = value; | ||
| struct calico_ct_value *rev_value; | ||
| bool isNatForward = false; |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Variable naming mixes styles (isNatForward) in a codebase that otherwise uses snake_case in C. For consistency and readability, prefer snake_case (e.g., is_nat_forward) in the C BPF code.
| CALI_DEBUG(" INVALID: Forward NAT entry with no reverse entry, ignoring due to creation grace period."); | ||
| break; | ||
| } | ||
| // If NAT fwd entry, do a lookup of the rev_key. |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The check for “NAT forward” is value->rev_key.protocol != 0. This sentinel is subtle and non-obvious. Please add a comment here explaining this contract (protocol==0 indicates normal/reverse entries, non-zero indicates a queued forward entry provided with its reverse key), or better, use an explicit flag in the value if available.
| if (actual_ct_value) { | ||
| if (isNatForward) { | ||
| // If the reverse key has expired, go to delete. | ||
| if (actual_ct_value->last_seen == value->rev_last_seen) { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The decision to delete when actual_ct_value->last_seen == value->rev_last_seen encodes an important invariant: equality means the entry hasn’t changed since it was queued. Please add a short comment clarifying the race you’re guarding against and why equality (not <=) is the right choice with monotonic ktime, and note units (ns).
| } else { | ||
| CALI_DEBUG(" EXPIRED: Reverse NAT entry, queued for deletion. (Forward NAT entry not yet seen.)"); | ||
| } | ||
| } else if (isNatForward) { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Comment/code mismatch: the comment says “Set the lookup_key to the forward entry and do a lookup of the fwd entry,” but the code sets lookup_key = key (which is the rev key as passed by the iteration). This is confusing for future readers. Either fix the comment to reflect the actual key being looked up or adjust the code to match the intent.
| // Race with a packet, CT entry now live again. | ||
| CALI_DEBUG(" RESURRECTED: entry no longer expired (max_age=%d).", max_age_s); | ||
| goto out; | ||
| delete: |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Minor typos in the deletion block comments (“togethere”, “Its”) make this delicate logic harder to parse. Please correct and tighten the wording since this section explains a crucial two-entry deletion sequence.
| // forward entry. Delete both the entries togethere. | ||
| // Rev_entry not present - lookup_key points to the forward entry. | ||
| // Delete it. | ||
| if(!cali_ct_delete_elem(lookup_key)) { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The CCQ map entry is only removed if cali_ct_delete_elem(lookup_key) succeeds. Previously, the queue entry was always removed. This behavior change can leave stale entries in the queue on transient failures. If the intention is to retry, please add a comment explaining the retry path and how unbounded growth is avoided (e.g., later scans reprocess and eventually purge). Otherwise, consider always removing the queue entry to avoid leaks and rely on future scanners to re-queue if needed.
| #ifdef IPVER6 | ||
| #define CCQ_MAP cali_v6_ccq | ||
| #define CCQ_MAP_V cali_v6_ccq1 | ||
| #define CCQ_MAP_V cali_v6_ccq2 |
There was a problem hiding this comment.
🧪 Maintainability & Tests — CCQ_MAP_V changed from cali_v[46]_ccq1 to cali_v[46]_ccq2. This is a versioned name change; please ensure there’s a clear migration path (e.g., old map cleanup and pin removal) and add a brief comment indicating that this is a new on-disk version to help future maintainers during upgrades/downgrades.
| #define CT_MAP_V cali_v4_ct4 | ||
| #endif | ||
|
|
||
| struct cali_ccq_value { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The new cali_ccq_value struct needs documentation. Please add a comment describing:
- rev_key semantics (zero for normal/reverse entries; non-zero for forward entries).
- last_seen and rev_last_seen units (ns) and endianness (host, written by userspace).
- How these are used by the cleanup program to detect staleness.
| // dataplane. To do that, we copy the keys to this map temporarily and then | ||
| // iterate over this map, deleting the pair together. | ||
| CALI_MAP_NAMED(CCQ_MAP, cali_ccq, 1, | ||
| CALI_MAP_NAMED(CCQ_MAP, cali_ccq, 2, |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Map definition bumps version to 2 and sets UpdatedByBPF=false. Please add a short comment explaining why UpdatedByBPF is false now (userspace writes queue entries), and note any compatibility expectations with previously pinned v1 instances.
| } | ||
|
|
||
| // BPFProgLivenessScanner is a scanner that uses a BPF program to scan the | ||
| // BPFProgCleaner uses a BPF program to scan the |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The type’s doc comment still describes scanning/expiry. Please expand it to describe the new Cleaner’s role and the two-phase approach: userspace enqueues deletions with snapshot timestamps; the BPF program performs safe, near-atomic deletions by revalidating timestamps.
| ICMPTimeout: s.timeouts.ICMPTimeout} | ||
|
|
||
| obj, err := bpf.LoadObject(binaryToLoad, ctCleanupData, ctMapParams.VersionedName()) | ||
| obj, err := bpf.LoadObject(binaryToLoad, ctCleanupData) |
There was a problem hiding this comment.
🧪 Maintainability & Tests — LoadObject is now called without the versioned map name. If this is intentional due to object changes, add a brief code comment to avoid future confusion since previous versions passed a map name to drive CO-RE map version selection.
|
|
||
| const KeySize = v3.KeySize | ||
| const ValueSize = KeySize | ||
| const KeySize = v4.KeySize |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The previous note “both our key and value are keys” no longer applies. Please add a comment documenting Value’s layout: [Key | last_seen | rev_last_seen], the sizes, units, and how the fields are used by the BPF cleaner.
| Flags: unix.BPF_F_NO_PREALLOC, | ||
| Version: 1, | ||
| UpdatedByBPF: true, | ||
| Version: 2, |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Version increased to 2 and UpdatedByBPF set to false. Add a note here clarifying that userspace writes values (with timestamps) and that BPF only reads and deletes. Also ensure release notes include the on-disk version bump.
| copy(e[0:KeySize], key) | ||
| } | ||
|
|
||
| func (e *Value) SetTS(ts uint64) { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Method names SetTS and SetRevTS are terse; consider renaming to SetLastSeen and SetReverseLastSeen (and adding comments) to prevent confusion about which timestamp is which. Similarly, consider defining named constants for the offsets.
| binary.LittleEndian.PutUint64(e[KeySize+8:], ts) | ||
| } | ||
|
|
||
| func NewValue(key []byte, ts, rev_ts uint64) Value { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — NewValue takes parameters (ts, rev_ts); please rename to (lastSeen, reverseLastSeen) and document the intended values (i.e., ctVal.LastSeen and the reverse entry’s LastSeen if applicable).
|
|
||
| const KeyV6Size = v3.KeyV6Size | ||
| const ValueV6Size = KeyV6Size | ||
| const KeyV6Size = v4.KeyV6Size |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Mirror the IPv4 cleanupv1.Value documentation: explain the ValueV6 layout, units, and semantics (the comment “both key and value are keys” no longer applies).
| Flags: unix.BPF_F_NO_PREALLOC, | ||
| Version: 1, | ||
| UpdatedByBPF: true, | ||
| Version: 2, |
There was a problem hiding this comment.
🧪 Maintainability & Tests — As with IPv4, please add a note about Version bump and UpdatedByBPF=false to aid future maintainers and release note writers.
| copy(e[0:KeyV6Size], key) | ||
| } | ||
|
|
||
| func (e *ValueV6) SetTS(ts uint64) { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Consider renaming SetTS/SetRevTS for ValueV6 to SetLastSeen/SetReverseLastSeen and add comments for clarity; also consider constants for offsets.
| binary.LittleEndian.PutUint64(e[KeyV6Size+8:], ts) | ||
| } | ||
|
|
||
| func NewValueV6(key []byte, ts, rev_ts uint64) ValueV6 { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — NewValueV6 argument names (ts, rev_ts) are opaque; rename to lastSeen/reverseLastSeen and document required values.
| @@ -77,7 +78,7 @@ type EntryGet func(KeyInterface) (ValueInterface, error) | |||
|
|
|||
| // EntryScanner is a function prototype to be called on every entry by the scanner | |||
There was a problem hiding this comment.
🧪 Maintainability & Tests — The EntryScanner interface now returns (ScanVerdict, int64) but lacks a comment specifying what the int64 represents. Please clarify:
- It’s a ktime in ns from ctVal.LastSeen or the reverse entry’s LastSeen depending on entry type.
- It must be the same clock domain as stored in CT map.
| @@ -114,11 +118,14 @@ type Scanner struct { | |||
| // EntryScanner. They are executed in the provided order on each entry. | |||
There was a problem hiding this comment.
🧪 Maintainability & Tests — NewScanner has accumulated many positional arguments (ct map, cleanup map, ip version, cleaner, etc.). Consider switching to functional options or a config struct to improve readability and reduce call-site brittleness as this evolves.
| used := 0 | ||
| cleaned := 0 | ||
|
|
||
| var val cleanupv1.ValueInterface |
There was a problem hiding this comment.
🧪 Maintainability & Tests — val is declared as a cleanupv1.ValueInterface and then conditionally assigned as Value or ValueV6 only to serialize into bytes. This indirection adds complexity. Suggest building a []byte directly for the cleanup map value (or use a small helper function that returns the correctly-sized value) to avoid an unnecessary interface abstraction.
| if s.ipVersion == 6 { | ||
| val = cleanupv1.NewValueV6(revKey, uint64(ctVal.LastSeen()), uint64(ts)) | ||
| } | ||
| err := s.ctCleanupMap.Update(ctKey.AsBytes(), val.AsBytes()) |
There was a problem hiding this comment.
🧪 Maintainability & Tests — On ctCleanupMap.Update error, only a debug log is emitted. Consider adding a metric counter and/or warning at rate-limited level to surface persistent queue issues (e.g., map full), and maybe a TODO outlining back-pressure or retry strategy.
| return &mockBPFCleaner{ctMap: ctMap, ctCleanupMap: ctCleanupMap} | ||
| } | ||
|
|
||
| func (m *mockBPFCleaner) Run(opts ...conntrack.RunOpt) (*conntrack.CleanupContext, error) { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — Nice lightweight mock for tests. Consider incrementing NumKVsCleaned in CleanupContext to simulate the real cleaner, so tests that assert “cleaned” metrics won’t have to special-case the mock behavior later if metrics are reintroduced.
| ctLogLevel = bpfconntrack.BPFLogLevelDebug | ||
| } | ||
|
|
||
| bpfCleaner, err := conntrack.NewBPFProgCleaner(int(ipFamily), config.BPFConntrackTimeouts, ctLogLevel) |
There was a problem hiding this comment.
🧪 Maintainability & Tests — If BPFProgCleaner creation fails, the code logs and continues with a nil cleaner. Please add a comment explaining the fallback (userspace scanner + no BPF batch cleanup) and consider emitting a warning metric. This will aid debugging in deployments where BPF object load fails.
| line := "" | ||
| // Program 10k tcp ct entries into map. This is done in batches of 2k. | ||
| for i := 1; i <= 10000; i++ { | ||
| for i := 1; i <= 20000; i++ { |
There was a problem hiding this comment.
🧪 Maintainability & Tests — The loop increases CT entries to 20000 from 10000. If this extends test runtime or flakiness, consider parameterizing by map size or gating on an environment flag. A brief comment about the motivation (exercising resize path under higher load) would help future maintainers.
There was a problem hiding this comment.
🧪 Council of Claudes — Maintainability & Tests
Tip
Maintainability & Tests lens · simplicity · tests · docs · idioms
Missing some design docs and observability; worthwhile tests added but gaps remain
- Architectural shift to a hybrid cleaner is sensible and simplifies the BPF side scanning, but the intent and invariants are under-documented. Please add a brief “design note” (either in the BPF C file header and/or in cleanupv1/map.go) explaining:
- What fields are stored in the cleanup map value, their units/endian, and how equality of timestamps is used to decide whether to delete.
- Expected lifecycle: userspace queues deletions with last_seen snapshots; the BPF program re-checks and deletes atomically; when entries are (not) removed from the CCQ map and why.
- Any retry semantics if deletion fails (currently, CCQ entries seem to be retained only if deletion succeeds).
- EntryScanner now returns a timestamp; update the interface comment to define what that timestamp must be (units, whether it’s the current ctVal.LastSeen vs reverse entry’s LastSeen depending on type) to make it easy to implement new scanners correctly.
- Observability regressed: the Prometheus metrics tracking seen/deleted entries were removed. Please reintroduce minimal metrics for:
- Number of entries queued for cleanup, queue update errors, queue size, and number cleaned by BPF. This will improve diagnosability if the queue fills or the BPF cleaner fails.
- The cleanupv1.Value API is a low-level byte layout with terse method names (SetTS/SetRevTS). Consider adding clearer names and comments (e.g., SetLastSeen/SetReverseLastSeen) and constants for field offsets to reduce future mistakes.
- Scanner constructor now has several positional params (ct map, cleanup map, ip version, cleaner, etc.). Consider an options struct to avoid brittle call sites as this evolves.
- Tests: good adaptations and a new UT covering the end-to-end scanner→queue→BPF-cleaner path. Additional coverage would help:
- IPv6 path (cleanupv1.ValueV6).
- NAT-forward-only and NAT-reverse-only cases, incl. the “rev missing” branch and re-try behavior if rev disappears between queue and clean.
- Queue-capacity/overflow behavior (Update errors are currently only logged at debug). A test guarding against silent failure or unbounded growth would be useful.
Line-specific findings
🤖 Council of Claudes · 27 inline comment(s)
| ctLogLevel = bpfconntrack.BPFLogLevelDebug | ||
| } | ||
|
|
||
| bpfCleaner, err := conntrack.NewBPFProgCleaner(int(ipFamily), config.BPFConntrackTimeouts, ctLogLevel) |
There was a problem hiding this comment.
🛡️ Security — The BPF cleaner is now a hard dependency for actual deletion, but cleaner creation errors are only logged and ignored. If NewBPFProgCleaner() fails, bpfCleaner remains nil, yet scanning proceeds and enqueues “to-be-deleted” entries into the cleanup map. Since inline deletion was removed and Run() is only invoked when bpfCleaner != nil, no entries are actually deleted in this case. Impact: expired/stale conntrack entries can persist indefinitely, potentially weakening isolation (e.g., allowing traffic to continue under now-stricter policy) and filling the cleanup queue map (DoS risk). Mitigations:
- Fall back to userspace deletion if the cleaner fails to load (either by retaining the prior “userspace” mode or by performing inline deletes for normal entries when cleaner == nil).
- Alternatively, short-circuit: if cleaner creation fails, don’t enqueue entries and keep previous inline-delete behavior for the current iteration; or fail/alert more loudly to avoid silent degradation.
| } | ||
|
|
||
| bpfCleaner, err := conntrack.NewBPFProgCleaner(int(ipFamily), config.BPFConntrackTimeouts, ctLogLevel) | ||
| if err != nil { |
There was a problem hiding this comment.
🛡️ Security — Only logging the cleaner creation error (and not changing behavior) allows the system to silently run without any deletion. To avoid prolonged exposure, return an error or disable the enqueue path (or use inline deletion) until the cleaner is available.
| conntrackScanner := bpfconntrack.NewScanner(bpfmaps.CtMap, ctKey, ctVal, | ||
| config.ConfigChangedRestartCallback, | ||
| config.BPFMapSizeConntrackScaling, | ||
| config.BPFMapSizeConntrackScaling, bpfmaps.CtCleanupMap, |
There was a problem hiding this comment.
🛡️ Security — A possibly-nil bpfCleaner is passed into NewScanner; downstream logic doesn’t delete entries if the cleaner is nil. Consider explicitly passing a non-nil “no-op” cleaner that triggers a clear fallback path (e.g., inline deletion for normal entries), or block scanner construction until a working cleaner is configured.
| val = cleanupv1.ValueV6{} | ||
| } | ||
|
|
||
| for _, scanner := range s.scanners { |
There was a problem hiding this comment.
🛡️ Security — Inline deletion on ScanVerdictDelete was removed in favor of enqueuing into the cleanup map. If the BPF cleaner is unavailable (or fails), entries marked for deletion will not be removed, extending the window where stale entries remain in the conntrack table. Suggestion: when cleaner is nil/unavailable, retain inline deletion for at least non-NAT entries (where safe), and/or add a configurable fallback to the prior userspace path.
| return | ||
| } | ||
|
|
||
| // Run the BPF cleanup program. |
There was a problem hiding this comment.
🛡️ Security — Deletion is performed only if s.bpfCleaner != nil (and then deferred to the BPF program). If the cleaner is nil, nothing is removed, and the cleanup queue can accumulate. Add a fallback path here to perform safe inline deletion when the cleaner is not present, or skip enqueuing entirely and signal an error so the dataplane doesn’t silently degrade.
There was a problem hiding this comment.
🛡️ Council of Claudes — Security
Caution
Security lens · validation · secrets · authz · isolation
1 potential security issue
- Verify that the pinned BPF cleanup queue (cali_v{4,6}_ccq) maps are created with restrictive filesystem permissions (root-only, not world-readable/writable). Since the cleanup program will delete arbitrary conntrack entries based on what’s queued in these maps, overly-permissive map pins could let a local attacker with access to bpffs inject deletions (disrupting traffic and potentially weakening isolation). This isn’t shown in the diff, so please double-check map pin paths and permissions.
🤖 Council of Claudes · 5 inline comment(s)
| } | ||
| if (cali_ct_delete_elem(rev_key) == 0) { | ||
| ictx->num_kvs_deleted_nat_rev++; | ||
| cali_ccq_delete_elem(key); |
There was a problem hiding this comment.
Nell — I think we now only remove the CCQ entry on successful delete(s). Previously we always deleted the scratch entry after re-check (to avoid the queue growing indefinitely). If an entry is “resurrected” (timestamps don’t match), we return 0 and leave it in CCQ forever (userspace won’t re-enqueue as it’s no longer expired). Should we always remove the CCQ item after inspection and rely on userspace to requeue when still needed (or, alternatively, explicitly remove when we detect it’s no longer eligible)?
One option (mirroring the previous out: path) would be to unconditionally delete the CCQ entry at the end of the handler:
| cali_ccq_delete_elem(key); | |
| // Always remove the entry from the cleanup queue; userspace will re-enqueue if needed. | |
| cali_ccq_delete_elem(key); | |
| return 0; |
(or, if we want to keep “retry until timestamps match”, maybe delete on “resurrected” too.)
| ctLogLevel = bpfconntrack.BPFLogLevelDebug | ||
| } | ||
|
|
||
| bpfCleaner, err := conntrack.NewBPFProgCleaner(int(ipFamily), config.BPFConntrackTimeouts, ctLogLevel) |
There was a problem hiding this comment.
Nell — If the BPF cleaner fails to create, we now continue and still enqueue into CCQ, but we’ll never drain it and we’ve removed the userspace delete path. Should we fail fast here, or fall back to the old userspace delete behaviour? Otherwise we risk leaking conntrack entries.
| // the entry for expiry. Expired normal entries are deleted inline. Expired | ||
| // NAT entries are queued for deletion in the cali_ccq map. | ||
| static long process_ct_entry(void *map, const void *key, void *value, void *ctx) | ||
| // process_ccq_entry processes an entry in the "cleanup queue" map. The map |
There was a problem hiding this comment.
Nell — The comment says CCQ is “keyed on the reverse NAT entry’s key, value stores the forward key”, but IIUC the new flow enqueues:
- Normal/NAT reverse: key = that entry, value.rev_key = zero
- NAT forward: key = forward key, value.rev_key = reverse key
Can we update the comment to reflect the new key/value and timestamp semantics?
There was a problem hiding this comment.
Council of Claudes — Nell
Important
Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments
2 potential correctness issues
High-level:
- IIUC this switches to a hybrid flow: userspace scanners enqueue into CCQ with last_seen snapshots; the BPF prog only drains CCQ and does equality checks to avoid racing. That looks neat.
- I think we have two behavioural risks:
- CCQ entries appear to be retained indefinitely if the entry is “resurrected” (timestamps don’t match) — we no longer unconditionally remove the queue item. That can leak the CCQ map over time.
- If the BPF cleaner fails to initialise, we now enqueue into CCQ but never drain it (scanner no longer deletes directly). Feels like we should either fail hard, or fall back to the previous userspace-delete path.
- A few stale/inaccurate comments: CCQ key/value semantics have changed (now key may be fwd key; value holds rev key and two timestamps). The C comment and the header still say “keyed on reverse … value is fwd key”. Also a couple of typos.
- nit: variable naming in eBPF C has drifted to camelCase; everything else around here is snake_case.
- Minor: we now log-and-continue on CCQ update failure at Debug. Do we want at least a Warn (and/or a counter)?
- Metrics/auto-resize were removed here — just checking that’s intentional; tests were tweaked to 20k entries, but autoscaling logic has gone. OK to leave that for follow-up if that’s the plan.
Other notes (not anchored to a diff line):
felix/bpf-gpl/conntrack_cleanup.c:48— “// If the reverse key has expired…” — in the new approach we’re really checking that the reverse entry’s last_seen hasn’t changed since we queued it (i.e., still safe to delete), no? Worth rewording to avoid conflating “expired” with “unchanged since scan”.felix/bpf-gpl/conntrack_cleanup.c:35— nit: camelCase stands out here; other locals/macros are snake_case. Worthis_nat_fwdfor consistency?felix/bpf-gpl/conntrack_cleanup.c:72— nit: small typos/grammar in this block (“togethere”, “Its” vs “It’s”). Maybe tighten this up since this is the key doc for the delete cases?suggestion // If the entry is Normal or Reverse, lookup_key points to that entry: // delete it and, if successful, increment the counter. // If the entry is a forward-NAT, there are two cases: // - Reverse entry present: lookup_key points to the reverse entry and key points to the // forward entry. Delete both entries together. // - Reverse entry not present: lookup_key points to the forward entry; delete it.felix/bpf/conntrack/scanner.go:189— We log CCQ update errors at Debug and continue. Should this be a Warn (or counted)? If we can’t enqueue, that entry won’t get cleaned by BPF, so we might want a louder signal.suggestion if err != nil { log.WithError(err).Warn("Failed to enqueue entry in cleanup map; entry may not be cleaned up.") }felix/bpf-gpl/conntrack_cleanup.h:30— The header still documents the old CCQ value as “NAT fwd key”. Now it’s a struct with rev_key and two timestamps. Can we refresh this comment to describe the new fields and behaviour?felix/bpf/conntrack/bpf_scanner.go:95— We dropped passing the CT map’s versioned name to LoadObject. Just checking: is the object no longer looking up pinned maps by name? If so, great; if not, do we need to keep providing the map name?felix/fv/bpf_map_resize_test.go:248— We doubled the iterations here, and we removed the autoscaling path in code. Is the intention to stop auto-resize altogether? If so, WDYT about a short note in the commit message, and maybe a follow-up to remove the now-unused sizing code paths? (Not blocking.)
🤖 Council of Claudes · 3 inline comment(s)
There was a problem hiding this comment.
Council of Claudes — Casey
Warning
Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness
3 potential correctness issues; a few API/upgrade and observability concerns.
- I think the hybrid approach makes sense overall — push selection into userspace, let BPF do the atomic pair delete if nothing has changed. Thanks for tightening that path.
- I am a little bit skeptical about removing both the auto-resize logic and the fine-grained conntrack cleaner metrics without a replacement. As-is, I don't see anything left that will ever write /var/lib/calico/bpf_ct_map_size or emit per-type deleted/seen stats. If that's intentional, can we call out what replaces those two (and how operators would tell “why” CT is filling)? Otherwise, I think we should either reintroduce a (simpler) fullness/resize check in Scanner or split that into a separate component.
- Upgrade path: bumping the CCQ map “version” (and switching the BPF object to reference cali_v[46]_ccq2) plus changing the Go map to Version: 2 looks right, but we also removed the map-name rewrite arg to LoadObject. I don’t quite recall if we rely on that to remap CO-RE map symbols to our pinned names; worth double-checking we won’t end up opening the wrong map on upgrade.
- Observability: we used to have summaryCleanerExecTime and per-type gauges/counters for cleaner activity. If we’re keeping only the existing “used/cleaned” metrics in Scanner, I think we lose visibility into NAT fwd/rev vs normal churn and cleaner runtime. Maybe that’s OK, but I’m surprised — WDYT about keeping at least the runtime metric?
- Test runtime: doubling the FV loop to 20k entries could sneakily add noticeable wall time. Do you know what operation was taking a long time here and why we need to double it? I wonder if we can keep the original count and still assert resize behavior.
Other notes (not anchored to a diff line):
felix/bpf-gpl/conntrack_cleanup.c:70— I think this leaks CCQ entries? For non-NAT-forward entries where the CT lookup fails, we hit the “else return 0;” path and never remove the queue entry. Previously, we always removed the scratch-map entry at the end of processing. Over time this could fill the cleanup map with stale keys (and then new updates will keep overwriting only if the same key is re-enqueued). - Suggestion: if the CT entry is missing, delete the CCQ entry to avoid leaks:if (!actual_ct_value) { cali_ccq_delete_elem(key); return 0; }Similarly, consider clearing CCQ in other early-return paths that won’t lead to deletion.felix/bpf/conntrack/cleanupv1/map.go:40— I wonder about UpdatedByBPF=false here. The BPF program deletes from this map (cali_ccq_delete_elem), so, by our convention, this should be true so the loader knows the map is updated by BPF. If we leave it false, do we risk the map manager or loader making wrong assumptions? - Suggest flipping to true unless there’s a strong reason not to.felix/bpf/conntrack/bpf_scanner.go:96— Removing the VersionedName() argument from LoadObject makes me a bit nervous — how are the CO-RE map symbols now matched to our pinned map names/versions (e.g., cali_v4_ccq2 vs the pinned “cali_v4_ccq” v2 path) across IPv4/IPv6? If libbpf can infer/pin correctly without the explicit map name rewrite, great, but I’m not 100% sure. Can you clarify the linking behavior here?felix/bpf-gpl/conntrack_cleanup.c:30— I think this comment is now stale/wrong for the hybrid path. The code now keys CCQ on “the entry we plan to delete” (normal, NAT rev, or NAT fwd), and packs rev_key/timestamps in the value. The comment says “map is keyed on the reverse NAT entry’s key” and “value stores forward key,” which isn’t true anymore. - Suggest rewording to reflect the new shape and the timestamp-equality contract.felix/bpf-gpl/conntrack_cleanup.c:49— nit: The comment says “If the reverse key has expired” but the code checks “rev last_seen unchanged since we queued it.” Maybe:suggestion // If the reverse entry hasn't seen any packets since we queued it, delete.Just to avoid mixing expiry vs. equality-of-last_seen.felix/bpf-gpl/conntrack_cleanup.c:77— nit: typosuggestion // forward entry. Delete both the entries together.felix/bpf/conntrack/scanner.go:191— I think this should be at least Warn or include the key + type; if the queue is full or Update fails, the entry may never be cleaned. Debug seems too quiet for an operationally-interesting failure. - Maybe:log.WithError(err).WithField("key", ctKey).Warn("Failed to enqueue CT entry for BPF cleanup; entry may not be cleaned.")felix/dataplane/linux/int_dataplane.go:2828— nit: we usually prefer WithError for consistency and to include the error as a field:suggestion log.WithError(err).Error("Failed to create BPF conntrack cleaner; falling back to userspace-only cleanup.")Also, do we want to log at Error here if we’re gracefully falling back? Info/Warn may be more appropriate.felix/fv/bpf_map_resize_test.go:248— This doubles the entries inserted to 20k. I’m a little bit skeptical this won’t extend the test runtime — these loops can add up across FV. Could we keep 10k (or gate count behind an env?) and still assert resize? Or is the higher number needed because the autoscaling trigger path has changed? In addition to the above, two broader questions: - Autoscaling/resizing: I don’t see an equivalent to the old “doubleiffull” logic anymore. Scanner still carries autoScale and maxEntries but doesn’t act on them. Is resize now handled elsewhere, or did we intentionally drop it? If dropped, we should remove the dead fields and adjust the FV to avoid implying a resize will happen. - Metrics: by removing the per-type “seen/deleted” metrics and cleaner runtime, we lose some signal that’s been useful in debugging. If we can keep at least a duration metric (and maybe a single “deleted” counter), I think that’d help, but I could be swayed either way. WDYT?
🤖 Council of Claudes · 0 inline comment(s)
There was a problem hiding this comment.
Pull request overview
This PR introduces a “hybrid” eBPF conntrack cleanup flow in Felix: userspace scanning identifies expired conntrack entries, enqueues them into a new cleanup-queue BPF map, and a BPF program performs the actual deletions (including paired NAT forward/reverse cleanup).
Changes:
- Add a new conntrack cleanup-queue BPF map (v4/v6) and wire it into map creation and unit/UT/FV tests.
- Update the conntrack scanner pipeline to enqueue deletions to the cleanup map and invoke a BPF cleaner program.
- Refactor BPF cleaner implementation (renaming/re-scoping the prior BPF liveness scanner into a cleaner) and update scanner interfaces accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| felix/fv/bpf_map_resize_test.go | Adjusts FV test load size used to trigger conntrack map resize. |
| felix/dataplane/linux/int_dataplane.go | Wires hybrid cleanup into dataplane startup (liveness scanner + BPF cleaner). |
| felix/bpf/ut/bpf_prog_test.go | Adds cleanup-queue maps to UT map initialization. |
| felix/bpf/ut/bpf_ct_cleanup_test.go | Updates UT to use the new hybrid scanner/cleaner approach. |
| felix/bpf/proxy/syncer_test.go | Updates proxy syncer tests to provide the new cleanup map + cleaner. |
| felix/bpf/mock/cleaner.go | Adds a mock Cleaner implementation for unit tests. |
| felix/bpf/conntrack/scanner.go | Implements enqueue-to-cleanup-map + BPF cleaner execution; updates scanner interfaces. |
| felix/bpf/conntrack/map.go | Exposes CleanupMap/CleanupMapV6 and exports cleanup map params. |
| felix/bpf/conntrack/inforeader.go | Updates EntryScanner interface implementation signature. |
| felix/bpf/conntrack/conntrack_test.go | Updates conntrack tests to construct scanners with cleanup map + cleaner. |
| felix/bpf/conntrack/cleanupv1/map.go | Defines v4 cleanup-queue map parameters and value encoding. |
| felix/bpf/conntrack/cleanupv1/map6.go | Defines v6 cleanup-queue map parameters and value encoding. |
| felix/bpf/conntrack/cleanup.go | Updates liveness/stale-NAT scanners to return extra timestamp data for BPF cleanup. |
| felix/bpf/conntrack/bpf_scanner.go | Refactors BPF program runner into a Cleaner that returns cleanup stats. |
| felix/bpf/bpfmap/bpf_maps.go | Adds CtCleanupMap to per-IP-family map sets and ensures creation/pinning. |
| felix/bpf-gpl/conntrack_cleanup.h | Updates cleanup queue map version/value type used by the BPF program. |
| felix/bpf-gpl/conntrack_cleanup.c | Changes BPF program to process the userspace-populated cleanup queue and delete CT entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -245,7 +245,7 @@ var _ = infrastructure.DatastoreDescribe("_BPF-SAFE_ Felix bpf conntrack table d | |||
|
|
|||
| line := "" | |||
| // Program 10k tcp ct entries into map. This is done in batches of 2k. | |||
| bpfCleaner, err := conntrack.NewBPFProgCleaner(int(ipFamily), config.BPFConntrackTimeouts, ctLogLevel) | ||
| if err != nil { | ||
| log.WithError(err).Fatal("Failed to create conntrack liveness scanner.") | ||
| log.Errorf("error creating the bpf cleaner %v", err) | ||
| } |
| for _, scanner := range s.scanners { | ||
| if verdict := scanner.Check(ctKey, ctVal, s.get); verdict == ScanVerdictDelete { | ||
| if verdict, ts := scanner.Check(ctKey, ctVal, s.get); verdict == ScanVerdictDelete { | ||
| if debug { | ||
| log.Debug("Deleting conntrack entry.") | ||
| } | ||
| cleaned++ | ||
| return maps.IterDelete | ||
| revKey := []byte{} | ||
| if ctVal.Type() == TypeNATForward { | ||
| revKey = ctVal.ReverseNATKey().AsBytes() | ||
| } | ||
|
|
||
| val = cleanupv1.NewValue(revKey, uint64(ctVal.LastSeen()), uint64(ts)) | ||
| if s.ipVersion == 6 { | ||
| val = cleanupv1.NewValueV6(revKey, uint64(ctVal.LastSeen()), uint64(ts)) | ||
| } | ||
| err := s.ctCleanupMap.Update(ctKey.AsBytes(), val.AsBytes()) | ||
| if err != nil { | ||
| log.Debugf("error updating cleanup map. Entry may not be cleaned up %v", err) | ||
| } | ||
| } | ||
| } | ||
| return maps.IterNone |
| // Run the BPF cleanup program. | ||
| if s.bpfCleaner != nil { | ||
| cr, err := s.bpfCleaner.Run() | ||
| if err != nil { | ||
| log.WithError(err).Warn("Failed to run bpf conntrack cleaner.") | ||
| } | ||
| cleaned = int(cr.NumKVsCleaned) | ||
| } |
| Version: 2, | ||
| UpdatedByBPF: false, | ||
| } |
| Flags: unix.BPF_F_NO_PREALLOC, | ||
| Version: 1, | ||
| UpdatedByBPF: true, | ||
| Version: 2, | ||
| UpdatedByBPF: false, | ||
| } |
| struct calico_ct_key, // key = NAT rev key | ||
| struct calico_ct_key, // value = NAT fwd key | ||
| struct cali_ccq_value, // value = NAT fwd key |
| // process_ccq_entry processes an entry in the "cleanup queue" map. The map | ||
| // is keyed on the reverse NAT entry's key, with the value storing the forward | ||
| // entry's key (or a zero value if the forward entry is missing). |
| // Delete the entry and if successful, increment the counter. | ||
| // If the entry is forward, we have 2 cases, | ||
| // Rev_entry present - lookup_key points to the reverse entry and key points to the | ||
| // forward entry. Delete both the entries togethere. |
| func (m *mockBPFCleaner) Run(opts ...conntrack.RunOpt) (*conntrack.CleanupContext, error) { | ||
| err := m.ctCleanupMap.Iter(func(k, v []byte) maps.IteratorAction { | ||
| if err := m.ctMap.Delete(k); err != nil { | ||
| return maps.IterNone | ||
| } | ||
| return maps.IterDelete | ||
| }) | ||
| return &conntrack.CleanupContext{}, err | ||
| } |
Benchmark sample reproducing the as-first-reviewed state of projectcalico/calico#10736 (by @sridhartigera).
This PR's diff equals what the human reviewers first saw: fork point
ff1cb5427677→ earliest human-reviewed commitdad1adeeb46f. The original PR drew 26 human top-level review comments — the ground truth to compare the Council's feedback against.🤖 Generated for the Council of Claudes benchmark.