perf/fix(network): cut policy apply latency and harden TAP create under density - #1318
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| // 12 CIDRs + 12 domains (incl. 2 wildcards) — medium allowlist for create-path | ||
| // CubeVS allow_out_v2 / dns_allow map updates. Hosts are stable fakes; the | ||
| // bench does not require them to resolve or be reachable. | ||
| return []string{ |
There was a problem hiding this comment.
The "rules" payload doesn't match its own test (and the README/PR description), so go test ./examples/cube-bench fails on this branch.
rulesAllowOut() returns only "1.1.1.1/32" — every other CIDR/domain is commented out — and rulesL7Rules() returns a single rule whose Action field is commented out. So networkFingerprint("rules") = {AllowOut:1, Rules:1, InjectRules:0}.
But TestBuildCreateRequestBodyRulesShape (main_test.go:156) hard-asserts:
if fp.AllowOut != 24 || fp.Rules != 6 || fp.InjectRules != 2 {
t.Fatalf("unexpected fingerprint: %+v", fp)
}1 != 24 → the test fails. The README and the PR description also promise "~24 allowOut + 6 L7 rules (2 with inject)".
Two options:
- Uncomment the intended entries (24 allowOut / 6 rules / 2 inject) so the implementation matches the documented intent, or
- if the reduced payload is deliberate, update the test assertion and README to the actual 1/1/0 fingerprint.
Note that with Action commented out, the single allow_llm_chat rule serializes as "action":{"allow":false} — a deny, with no inject — so the bench would not be exercising the allow-with-inject path described in the README either.
| } | ||
| } | ||
| } | ||
| go func(keep map[uint32]struct{}) { |
There was a problem hiding this comment.
TOCTOU: the keep snapshot is taken here, but the GC goroutine runs asynchronously and overlaps warmupTapPoolBackground and on-demand EnsureNetwork creates.
Every TAP created after this snapshot (pool warmup fills, pool-miss creates) has an ifindex that is not in keep. gcStaleOuterKeys iterates the HashOfMaps and deletes any key whose ifindex is absent — so a freshly warmed/created TAP's deny_out/allow_out/dns_allow outer keys can be deleted by this background GC while the TAP is still live. The comment at line 71-73 acknowledges "Warmup only adds new ifindexes later" but the GC is exactly when those late keys are at risk.
The concrete impact: a Ready pool TAP whose deny_out outer key was removed by the GC can be assigned to a sandbox that never receives the private-range deny defaults — the normal (non-replace) applyNetPolicy path does not re-add alwaysDeniedSandboxEntries (only effectiveDenyOutEntriesForReplace does). The sandbox would then be missing deny rules for 10.0.0.0/8, 169.254.0.0/16, etc.
The window is largest exactly in the pathological case this PR targets (thousands of stale inners → slow Iterate/Delete → long overlap with warmup). Consider computing keep inside the goroutine after warmup completes, or skipping the GC when the tap listing / keep-set is known-incomplete.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := outerMap.Put(&ifindex, created); err != nil { |
There was a problem hiding this comment.
Concurrency hazard in the create path: two goroutines that both miss the cache for the same (mapName, ifindex) and both reach newInner() can leave the cache and the outer map pointing at different inner maps.
Interleaving:
- G1:
Lookup→ErrKeyNotExist, G2:Lookup→ErrKeyNotExist - G1:
created1 = newInner();outer.Put(ifindex, created1) - G2:
created2 = newInner();outer.Put(ifindex, created2)→ overwrites G1's entry in the outer map - G1:
LoadOrStore(key, created1)→ stores, returns created1 - G2:
LoadOrStore(key, created2)→loaded=true, closes created2, returns created1
End state: the process-wide cache holds created1, while the kernel's HashOfMaps entry (what the eBPF programs see) points to created2. Every subsequent policy write through the cached created1 is invisible to the dataplane, and the divergence persists until releaseCachedInner (TAP destroy). The pre-refactor code re-read the outer map on each call, so it self-healed; the cache removes that.
Worth serializing creation per (mapName, ifindex) (e.g., a per-key mutex around the lookup→create→put), or re-reading the outer value after a LoadOrStore miss and reconciling which inner is actually live in the outer map.
AI-Generated Code Review — PR #1318: perf/fix(network): cut policy apply latency and harden TAP create under densityThis is an automated review produced by an AI code reviewer. It is a static analysis of the diff against the base branch; it has not been run against the PR head tree, and no tests were executed. Overall: This is a well-structured performance PR. The core idea — caching HashOfMaps inner-map FDs process-wide in 1. Medium — Process-global inner-map cache leaks across tests;
|
9afc903 to
a4613ad
Compare
| } | ||
| } | ||
| } | ||
| go func(keep map[uint32]struct{}) { |
There was a problem hiding this comment.
The keep set is snapshotted once at the end of recover(), but GC runs in this background goroutine and is deliberately slow ("pathologically slow" when thousands of stale inners leak). warmupTapPoolBackground() starts right after recover() returns, and on-demand creates begin once the API server is up — so TAPs created after the snapshot but before GC finishes iterating are not in keep. gcStaleOuterKeys will then delete their allow_out_v2/deny_out/dns_allow outer keys and close their cached inners while the TAP is live.
This is security-relevant: the datapath is allow_out_v2 > deny_out > default allow (CubeNet/src/session.h), and applyNetPolicy with replace=false skips the deny_out block entirely when the plan has no deny entries. An allow-internet sandbox relies on the pool-stage default-deny entries (alwaysDeniedSandboxEntries) installed in deny_out; with that outer key gone, traffic to 10.0.0.0/8, 169.254.0.0/16, etc. silently becomes allowed.
Suggest rebuilding keep inside the goroutine right before iterating (ideally under s.mu), or re-verifying at delete time that the ifindex is still absent from the live TAP set / pool.
| // After the netdev is gone it deletes HashOfMaps outer policy keys so destroyed | ||
| // ifindexes cannot accumulate stale allow_out_v2 / deny_out / dns_allow inners. | ||
| func destroyTap(ifIdx int) error { | ||
| link, err := netlinkLinkByIndex(ifIdx) |
There was a problem hiding this comment.
Two related problems in the new destroyTap policy cleanup:
-
This branch treats any
netlinkLinkByIndexerror (transientErrDumpInterruptedafter retries, EPERM, ENODEV…) as "netdev already gone" and deletes the TAP's policy maps — and discards the cleanup error (_ =). If the error was transient and the device is still up, its policy is silently dropped (datapath falls back to default allow). Consider deleting policy only when the link is confirmed absent (netlink.LinkNotFoundError). -
Asymmetry with the success path below: there, a
DeleteTAPDevicePolicyMapsfailure now fails the entire destroy, so a netdev that was successfully destroyed is reported as a destroy error — and the cached inners were already released. Under a degraded/missing-BPF condition everyDestroywould start failing. Prefer best-effort policy cleanup that does not change the destroy result, or at least keep the two branches consistent.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := outerMap.Put(&ifindex, created); err != nil { |
There was a problem hiding this comment.
The create-and-Put critical section is unsynchronized per key. Two concurrent acquireInnerMap calls for the same (mapName, ifindex) can both miss the cache, both create inners, and both outerMap.Put — the outer ends up referencing one inner while LoadOrStore keeps the other in the cache (closing the loser). Subsequent cache hits then write policy to an inner the outer no longer references (silent policy loss, since the datapath looks up the outer), and the superseded kernel inner leaks.
Separately, a Lookup result cached after a concurrent releaseCachedInner/outer-delete leaves a stale FD in the cache; a later TAP reusing the ifindex will hit the stale inner and write policy to a map not installed in the outer. With the DNS reaper and the new async GC now running concurrently with the create/apply paths, these races are reachable in practice. Consider guarding with per-key locks, and/or re-validating on cache hit that the outer still references the cached inner.
|
|
||
| func buildCreateRequestBody(template string, hostMount string) ([]byte, error) { | ||
| func buildCreateRequestBody(template string, hostMount string, networkPolicy string) ([]byte, error) { | ||
| reqBody := createRequest{TemplateID: template} |
There was a problem hiding this comment.
Compile break: one buildCreateRequestBody call site was not updated. This signature change (2→3 args) updated the callers in main.go and two of the three call sites in runner_test.go, but runner_test.go:13 (TestRunWarmupCompletesBeforeBenchmark) still calls buildCreateRequestBody("tpl-warmup", "") with the old arity. The examples/cube-bench test package will not compile (go vet / go test ./... fails), which breaks CI. Fix: buildCreateRequestBody("tpl-warmup", "", networkPolicyNone).
| newInner func() (*ebpf.Map, error), | ||
| ) (*ebpf.Map, error) { | ||
| key := policyInnerKey{mapName: mapName, ifindex: ifindex} | ||
| if cached, ok := policyInnerMaps.Load(key); ok { |
There was a problem hiding this comment.
FD-cache race: an unsynchronized Load can hand out an FD that a concurrent destroy/GC just closed, and the release→delete window can orphan a cached inner.
Two problems with the cache lifetime:
-
This fast-path
Loadis not covered bylockPolicyInnerKey.releaseCachedInner(LoadAndDelete→Close) can run between theLoadhere and the caller's firstinner.Update/Iterate, so the caller uses a closed FD and gets EBADF on the create/apply hot path — the exact path this PR is trying to harden. -
More seriously, the cache can be left pointing at an orphaned inner. In
DeleteTAPDevicePolicyMaps/gcStaleOuterKeys,releaseCachedInnerandouter.Deleteare separate steps and the per-key lock is released beforeouter.Delete(andloadPinnedMapruns in between). A concurrentacquireInnerMapcan take the lock in that window,outer.Lookupthe still-present key, andLoadOrStorethe about-to-be-deleted inner back into the cache; the subsequentouter.Deletethen orphans it. Any later TAP that reuses that ifindex gets the orphaned inner from the cache, and its allow/deny policy updates are written to a map the outer (dataplane) no longer references — silent policy non-enforcement.
Consider making the destroy path hold the per-key lock across releaseCachedInner + outer.Delete (e.g., a combined delete-and-release under one lock acquisition), and/or re-validating the outer key on the fast path.
| func (s *NetworkController) scheduleStaleNetPolicyMapGC() { | ||
| go func() { | ||
| keep := s.buildStaleNetPolicyKeepSet() | ||
| deleted, err := s.cubevsAdapter.GCStaleNetPolicyMaps(keep, netPolicyIfindexStillPresent) |
There was a problem hiding this comment.
TOCTOU: startup GC can wipe a freshly-created TAP's default-deny policy.
gcStaleOuterKeys re-checks stillPresent(ifindex) and then calls deleteCachedInnerAndOuter a few instructions later; the check and the delete are not atomic. If warmup / an on-demand create reuses that ifindex between the re-check and outer.Delete, this GC deletes the HashOfMaps outer keys for a TAP that just had InstallTAPDefaultDenyPolicy applied. applyNetPolicyWithMode with replace=false (normal AddTAPDevice path) only populates plan.denyOutEntries; for an allow_internet_access=true sandbox that's empty and the always-denied private-range/metadata entries are not re-installed, so the sandbox would serve egress without the denylist until the next Cubelet restart. The window is microscopic and startup-only, but the failure mode is a silently-missing security boundary.
The comment on lines 83-86 acknowledges the window; consider closing it rather than just shrinking it — e.g. re-run stillPresent after the delete and restore on conflict, serialize GC against warmup/on-demand create, or defer GC until after warmup with a fresh keep-set.
| continue | ||
| } | ||
| if err := deleteCachedInnerAndOuter(outer, mapName, ifindex); err != nil { | ||
| return deleted, fmt.Errorf("delete stale %s[%d]: %w", mapName, ifindex, err) |
There was a problem hiding this comment.
gcStaleOuterKeys aborts the whole sweep on the first per-key delete error.
GCStaleNetPolicyMaps already accumulates errors per outer map, but this inner loop returns immediately on the first failed deleteCachedInnerAndOuter. Because this GC only runs once at Cubelet startup, a single transient error (e.g. EBUSY from a concurrently-running reaper, or ENOMEM) leaves every remaining stale key undeleted until the next restart — which is exactly the leak this PR is trying to drain. Suggest errs = append(errs, fmt.Errorf(...)); continue and errors.Join(errs...) at the end, mirroring the caller.
| // | ||
| // Callers must not use the returned map across TAP destroy / stale-outer GC for | ||
| // the same ifindex; Cubelet serializes those against apply for a given TAP. | ||
| func acquireInnerMap(outerMap *ebpf.Map, ifindex uint32, mapName string, |
There was a problem hiding this comment.
Design note: unbounded FD cache with no invalidation hook, and no direct unit tests.
Two things worth addressing before this lands:
-
FD budget. Every
{mapName, ifindex}now pins one open inner-map FD for the life of the TAP (3 maps × active TAPs), and the DNS reaper'snewInner=nilacquires additionally warm the cache for any active ifindex that was created before this cache existed. On a large host this adds a steady ~3×-active-TAP-count to Cubelet's FD usage with no eviction other than TAP destroy / startup GC. TheLookup-then-cache path (line 56) makes even read-only consumers pin FDs. A bounded cache (or at least a documented FD-accounting expectation) would make the tradeoff explicit. -
No reload invalidation. The cache is process-global and keyed only by
{mapName, ifindex}— if the pinned outer maps are ever recreated (a future BPF reload path),acquireInnerMapwould keep handing out FDs to maps no longer in the dataplane. The current codebase has no reload path so this is latent, but a generation counter or aclearPolicyInnerCachehook would make it safe.
Also: this is the highest-risk new file in the PR (per-key locking + FD ownership invariants) but has no direct unit tests — the invariants are only exercised indirectly through netpolicy/dnspolicy tests. A focused test for the lock-ordering / cache-vs-outer consistency would be valuable.
| // flushed, so restricting to ifindex_to_mvmmeta matches Active (and in-flight | ||
| // Cleaning) sandboxes without iterating every HashOfMaps outer key. | ||
| func reapDNSLearnedPolicies(now uint64) { | ||
| meta, err := loadPinnedMap(MapNameIfindexToMVMMetadata) |
There was a problem hiding this comment.
DNS reaper now fails closed on ifindex_to_mvmmeta load — stale DNS-learned allow rules can accumulate.
Previously reapDNSLearnedPolicies reaped allow_out_v2 independently; the metadata map wasn't involved. Now a failure to load ifindex_to_mvmmeta (transient pin-path issue, ENOENT after an upgrade, permissions) makes reapDNSLearnedPolicies return early, so no expired DNS-learned entries are removed for that tick — and for every subsequent tick while the failure persists, since each tick reloads the map. The leaked entries are exactly the DNS-learned allow rules this reaper is supposed to expire. Consider warning and continuing (falling back to iterating allow_out_v2) instead of failing closed, or at least make the metadata load failure a per-tick warn so it's visible.
| @@ -563,22 +622,17 @@ func isValidDNSDomainName(domain string) bool { | |||
| // populateInnerMap inserts pre-parsed deny_out entries into the inner LPM trie | |||
| // map for the specified ifindex. | |||
| func populateInnerMap(outerMap *ebpf.Map, ifindex uint32, entries []denyOutPolicyEntry) error { | |||
There was a problem hiding this comment.
Dead code: populateInnerMap / populateAllowOutInnerMap wrappers are no longer called.
After this PR refactored applyNetPolicyWithMode and InstallTAPDefaultDenyPolicy to call populateDenyOutInner / populateAllowOutInner directly with the cached inner, neither populateInnerMap (here) nor populateAllowOutInnerMap (below) has any remaining callers in the package. These are the same class of unused helper this PR removes on the Cubelet side ("remove an unused Cubelet helper left over"). Worth deleting to avoid maintaining two code paths for the same populate operation — one of which still does the old acquire-then-populate dance.
| return fmt.Errorf("failed to open allow_out_v2 inner map: %w", err) | ||
| } | ||
| defer inner.Close() | ||
| return reapDNSLearnedPoliciesForInner(inner, now) |
There was a problem hiding this comment.
Concurrency hazard: the DNS reaper uses a cached inner map that a concurrent TAP destroy / stale-GC can Close out from under it.
acquireInnerMap returns the process-wide cached *ebpf.Map and releases the per-key lock before the caller uses it. Here the reaper then iterates and deletes entries on that map via reapDNSLearnedPoliciesForInner with no lock held. Meanwhile destroyTap → DeleteTAPDevicePolicyMaps → deleteCachedInnerAndOuter takes the same per-key lock and Close()s that exact FD. The reaper runs on a 5s ticker goroutine (StartSessionReaper/doReap) and is not serialized against destroy for a given ifindex — the comment in acquireInnerMap ("Callers must not use the returned map across TAP destroy / stale-outer GC for the same ifindex") does not protect this caller.
Failure modes: an iteration on a closed FD returns EBADF (iter.Err() is logged and that tick's reap for the ifindex is silently lost), or worse — if the freed FD number is reused by another map creation in the window, the reaper's inner.Delete targets an unrelated map and can corrupt a live sandbox's allow_out policy.
Suggestion: hold the per-key lock for the duration of the inner iteration (the destroy path already takes the same lock, so it would wait out the reap before closing), or defer the Close in deleteCachedInnerAndOuter until no reader can be in flight.
| // churn and surfaces as netlink ErrDumpInterrupted under density load. | ||
| func (s *NetworkController) cleanupConflictingTap(ip net.IP) error { | ||
| taps, err := s.tapAdapter.List() | ||
| tap, err := s.tapAdapter.GetByName(tapName(ip.String())) |
There was a problem hiding this comment.
The "not LinkList, RTM_GETLINK by name" claim is incorrect — this still performs a full link-table dump.
realTapDeviceAdapter.GetByName → getTapByName → netlinkLinkByName → netlink.LinkByName. In vishvananda/netlink (v1.3.1 here), LinkByName is implemented as LinkList() (a full RTM_GETLINK dump) followed by a linear name scan — there is no single-link RTM_GETLINK-by-name lookup. So switching from tapAdapter.List() to GetByName(tapName(...)) does not stop doing a full LinkList on every pool-miss create, and the latency win claimed in the comment above ("Lookup is by deterministic tap name (RTM_GETLINK), not LinkList. A full dump of every host interface ... races with concurrent TAP churn") is not realized by this change. What does fix the ErrDumpInterrupted/EINTR failure is the new WithDumpRetry wrapper.
Worth correcting the comment and re-measuring the create-path delta so the benchmark result isn't attributed to the wrong optimization. (Minor behavior delta: getTapByName now returns a hard "%s is not tap" error for a same-named non-TAP device, where the old List()-based path silently skipped it.)
| // (pre-shard or downgrade-written) files first, then each shard in name order. | ||
| // Invalid names are ignored so unrelated files in the state directory do not | ||
| // break runtime startup. | ||
| // Scan returns every valid state file under hash shards in deterministic |
There was a problem hiding this comment.
This is an unrelated behavioral change that contradicts the code's own downgrade-compatibility story — please verify the "never released" claim.
Removing migrateFlatStateFiles and the root-level flat scan changes startup recovery semantics. The removed migration's doc comment said it existed to pick up "files written by a downgraded binary," and persistedState.MarshalJSON in this same file still explicitly preserves cubevsContext "while preserving downgrade/read compatibility during the transition." If a downgraded build ever wrote root-level *.json state files, this change silently orphans them on the next upgrade — those sandboxes would fail to recover (their records are neither migrated nor scanned).
The "Flat (unsharded) layout was never released" assertion is a claim about release history that a reviewer can't verify from this diff, and it sits in a PR titled "perf/fix(network)". If the claim is not airtight, this should either stay behind a migration or be called out explicitly in the PR body as an intentional compat break.
747f3df to
0ca5c85
Compare
| newInner func() (*ebpf.Map, error), | ||
| ) (*ebpf.Map, error) { | ||
| key := policyInnerKey{mapName: mapName, ifindex: ifindex} | ||
| if cached, ok := policyInnerMaps.Load(key); ok { |
There was a problem hiding this comment.
HIGH — stale-cache TOCTOU: a reused ifindex can silently lose all allow/deny policy.
The cache-hit path returns the cached inner without verifying the outer HashOfMaps still maps this ifindex to it. The DNS reaper (dns_reaper.go fast path) is a new background reader that populates this cache every 5s via acquireInnerMap(allowOut, ifindex, ..., nil), and Lookup → LoadOrStore is not atomic with deleteCachedInnerAndOuter's outer.Delete:
- Reaper:
outerMap.Lookup(X)succeeds → inner = A - Destroy:
deleteCachedInnerAndOuterdoesLoadAndDelete(X)(cache miss) thenouter.Delete(X) - Reaper:
LoadOrStore(X, A)re-caches an ifindex whose outer key is now gone
Nothing evicts it (destroy already ran its eviction; GC only runs at startup). When the kernel reuses ifindex X for a new TAP, acquireInnerMap cache-hits A and all policy setup writes into a map the dataplane never looks up — the create succeeds but egress policy is silently detached, with no error and no recovery until restart. deleteCachedInnerAndOuter has the mirror window: its LoadAndDelete runs before outer.Delete, so a concurrent acquire landing between them re-populates the cache after eviction.
The comment at lines 34–36 justifies skipping a per-key lock with the "no concurrent ownership of one ifindex" lifecycle invariant — but that invariant covers two creators of the same ifindex, not a background reader racing a destroy, which is precisely what the reaper now is.
Suggested fix: per-key serialization around acquire/release (per-key mutex, or lock-per-key via sync.Map), or in deleteCachedInnerAndOuter re-evict any entry stored between the first LoadAndDelete and outer.Delete; validating the outer on cache-hit works but defeats the Lookup-cost goal.
| _ = created.Close() | ||
| return nil, fmt.Errorf("map.Put failed: %w, name: %s", err, mapName) | ||
| } | ||
| actual, loaded := policyInnerMaps.LoadOrStore(key, created) |
There was a problem hiding this comment.
MEDIUM — creation branch is only safe under an unenforced external invariant.
If two goroutines create the same (mapName, ifindex) concurrently (both newInner non-nil), both miss the Lookup, both Put; the second Put overwrites the outer key with the loser's map, then the loser's LoadOrStore reports loaded and closes its map while the outer still references it, and the cache returns the winner's map which the outer no longer references — policy writes then go to a detached map.
Today the only concurrent background reader (the reaper) passes nil newInner, so this isn't triggerable — but the "no concurrent ownership" invariant is external to this helper and untested. A per-key lock (or only Put the outer when the cache-store wins) makes the helper correct independent of caller discipline. Consider folding this into the fix for the stale-cache TOCTOU at line 41.
| // | ||
| // Lookup is by deterministic tap name (RTM_GETLINK), not LinkList. A full dump | ||
| // of every host interface on every pool-miss create races with concurrent TAP | ||
| // churn and surfaces as netlink ErrDumpInterrupted under density load. |
There was a problem hiding this comment.
LOW/MEDIUM — this claim doesn't hold for netlink v1.3.1, and the conflict path has no test coverage.
In vishvananda/netlink v1.3.1 (per Cubelet/go.mod), LinkByName and LinkByIndex are implemented on top of a full LinkList() (RTM_GETLINK dump) that they iterate to find the name/index. So tapAdapter.GetByName(...) still performs a full host-interface dump on every pool-miss create — the "not LinkList" rationale (also in the PR description: "stop doing a full LinkList on every pool-miss create") doesn't hold for this version. What actually fixes the ErrDumpInterrupted density failure is the new WithDumpRetry wrapper around netlinkLinkByName/netlinkLinkByIndex (tap_device.go:1350-1362), not the avoidance of the dump; per-create dump traffic is unchanged.
Also: this conflict-detection path has no direct unit coverage — the test fakes return nil, nil from GetByName, so neither the not-found branch nor the checkTapConflict branch is exercised. Worth rewording the comment and adding a test that makes GetByName return a colliding tap.
| // Scan returns every valid state file under hash shards in deterministic | ||
| // shard-name then filename order. Invalid names are ignored so unrelated files | ||
| // in the state directory do not break runtime startup. Flat (unsharded) layout | ||
| // was never released, so root-level "*.json" state files are not scanned. |
There was a problem hiding this comment.
LOW — removing flat-layout handling depends on a "never released" claim.
Dropping migrateFlatStateFiles() (and the root-level scan here) means any state file still in the pre-shard flat layout is silently ignored — not migrated, not scanned, not loadable. The removed helper explicitly handled "files written by a downgraded binary," implying a downgrade scenario was considered real. If the "never released" claim is wrong for any deployment path, those sandboxes will fail to restore on upgrade and their TAPs will be treated as stale and destroyed. Please confirm the claim against release history, or keep a one-time migration for installs that ran a pre-shard build.
| } | ||
| return 42, nil | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
Use testify packages for assertions.
| } | ||
| } | ||
| } else { | ||
| CubeLog.WithContext(context.Background()).Warnf( |
There was a problem hiding this comment.
If we reach here, all inner maps will be deleted?
There was a problem hiding this comment.
Agreed — if List() fails the keep set is incomplete, so we now skip this GC pass entirely instead of treating live TAP ifindexes as stale.
| func flushInnerEntries(inner *ebpf.Map, value any) error { | ||
| var key lpmKey | ||
| iter := inner.Iterate() | ||
| for iter.Next(&key, value) { |
There was a problem hiding this comment.
We should use a 2-pass flush here, collect keys and then delete.
eea191c to
b6ca369
Compare
|
|
||
| // deleteCachedInnerAndOuter removes the outer key and its cached userspace FD. | ||
| func deleteCachedInnerAndOuter(outer *ebpf.Map, mapName string, ifindex uint32) error { | ||
| key := policyInnerKey{mapName: mapName, ifindex: ifindex} |
There was a problem hiding this comment.
Finding (Medium, PLAUSIBLE): use-after-close on a shared cached FD — the 5s DNS reaper can hold a pointer to an inner that deleteCachedInnerAndOuter has just closed.
deleteCachedInnerAndOuter does LoadAndDelete + Close() on the cached inner FD, while the reaper (dns_reaper.go → acquireInnerMap(allowOut, ifindex, MapNameAllowOutV3, nil) → iterate) fetches the same cache entry and uses it without holding any lock. The exclusivity reasoning in the acquireInnerMap doc comment (line 32–36: "TAP lifecycle prevents concurrent ownership of one ifindex") holds for the create/apply path, but the background reaper is a separate goroutine sharing the same cache entries: its tick can snapshot an ifindex, then TAP destroy closes the cached FD, then the reaper iterates/updates the closed map → EBADF; in the worst case the FD number is reused and the reaper touches an unrelated kernel object.
Pre-PR each reaper pass opened its own FD via ebpf.NewMapFromID and closed it locally, so this race did not exist. Impact is bounded (a destroyed TAP's entries are moot), but this is a new concurrency hazard on shared mutable state and will fire under the create/destroy churn this PR targets. Consider per-key refcounting (or a per-key mutex spanning Load→use→release, with the destroy path taking the same lock), or having the reaper recover from EBADF by re-acquiring the outer.
| defer allowOut.Close() | ||
|
|
||
| meta, err := loadPinnedMap(MapNameIfindexToMVMMetadata) | ||
| if err != nil { |
There was a problem hiding this comment.
Finding (Low, PLAUSIBLE): the fast path only reaps ifindices present in ifindex_to_mvmmeta; the allow_out_v3 outer scan is now reachable only when the metadata map fails to load, not when an ifindex is simply missing from it.
If the "metadata deleted ⟺ allow_out flushed" invariant is ever violated (crash or failed cleanup between the two steps, partial pool cleanup failure), an allow_out_v3 inner whose ifindex is absent from mvmmeta will never have its expired DNS-learned entries reaped — the previous outer-scan covered every outer key. The fallback covers map-load failures only. Low likelihood given the invariant, but it's a silent robustness regression in exactly the partial-failure modes the old code tolerated. Consider also reaping allow_out outers whose ifindex is not in the metadata set, or documenting/asserting that the invariant is enforced atomically.
| dest, err := lookupInnerMap(newOuter, ifindex) | ||
| dest, err := lookupInnerMap(newOuter, ifindex, MapNameDNSAllowV2) | ||
| if err != nil { | ||
| t.Fatalf("lookupInnerMap: %v", err) |
There was a problem hiding this comment.
Finding (Medium, CONFIRMED by inspection): cross-test cache contamination makes this assertion vacuous.
policyInnerMaps is a process-global sync.Map keyed only by (mapName, ifindex) — not by the outer map object — and clearPolicyInnerCacheForTest() (inner_cache.go:304) is never called by any test. This test reuses ifindex = 42 with dns_allow_v2, as do TestMigrateDNSAllowInnerMapFromLegacy (line 102) and TestMigrateDNSAllowInnerMapFromCurrent (line 220), which run earlier and populate the cache.
So in TestMigrateDNSAllowMapOuterWithBpffs:
migrateDNSAllowMap→ensureDNSAllowInnerMap(newOuter, 42)→acquireInnerMapcache-hits the earlier test's inner FD and skipsouterMap.Put(&ifindex, ...)— the freshly pinneddns_allow_v2outer is never populated.lookupInnerMap(newOuter, 42, MapNameDNSAllowV2)returns that same stale inner.- The assertions read the stale inner, so the test passes while verifying nothing about the pinned
newOutermap the test is named after.
Please call clearPolicyInnerCacheForTest() in setup (or scope the cache per-test / make the key include the outer map identity) so these tests actually exercise the maps they construct.
| // Scan returns every valid state file under hash shards in deterministic | ||
| // shard-name then filename order. Invalid names are ignored so unrelated files | ||
| // in the state directory do not break runtime startup. Flat (unsharded) layout | ||
| // was never released, so root-level "*.json" state files are not scanned. |
There was a problem hiding this comment.
Finding (Low, verify before merge): the removed flat-layout migration also served as a downgrade backstop, not just legacy-release support.
The deleted migrateFlatStateFiles doc comment read: "files written by a downgraded binary are picked up on the next startup." If any released build ever wrote root-level *.json state files (or a binary downgrade writes them), removing this migration and the flat-root scan in Scan() silently stops recovering those sandbox states at startup — a recovery regression that would surface only after an operator downgrades. Please confirm the "Flat (unsharded) layout was never released" claim against released tags before merging, or keep the root-level scan as a compatibility shim.
…nsity Avoid hot-path outer Lookups that trigger synchronize_rcu, delete policy maps on TAP destroy, and GC leftover HashOfMaps keys so network-policy create latency stays near the no-policy baseline. Retry dump-style netlink reads on ErrDumpInterrupted/EINTR and stop doing a full LinkList on every pool-miss create, so EnsureNetwork no longer fails with "results may be incomplete or inconsistent" under high-concurrency TAP churn. Skip stale-policy GC when listing TAPs fails, and flush inner maps with a two-pass collect-then-delete of key values. Signed-off-by: yarrischen <yarrischen@tencent.com> Co-authored-by: Cursor <cursoragent@cursor.com>
5e96bb0 to
8069992
Compare
| return nil | ||
| } | ||
|
|
||
| func clearPolicyInnerCacheForTest() { |
There was a problem hiding this comment.
clearPolicyInnerCacheForTest is defined but never called anywhere in the PR, so the process-global policyInnerMaps cache leaks across tests in the cubevs package. The migration tests reuse ifindex = 42 (six tests) and 43, all with the same map names (MapNameDNSAllowV2 / MapNameAllowOutV3), and each test creates a fresh outer map. With the old lookupInnerMap(outerMap, ifindex) the inner was resolved from that test's own outer and every test was isolated; now acquireInnerMap/lookupInnerMap return the cached inner FD for (name, ifindex) from whichever test ran first — an FD belonging to a different, already-closed outer. The removed defer dest.Close() calls in migration_test.go masked this: writes and the subsequent dest.Lookup assertions both go through the stale cached FD, so the tests still pass but now validate a detached map the outer under test never references. Please wire t.Cleanup(clearPolicyInnerCacheForTest) into the cache-touching tests (or clear in TestMain), and consider adding the outer map's ID to the cache key so an entry can never be served for a different outer.
| _ = created.Close() | ||
| return nil, fmt.Errorf("map.Put failed: %w, name: %s", err, mapName) | ||
| } | ||
| actual, loaded := policyInnerMaps.LoadOrStore(key, created) |
There was a problem hiding this comment.
Create race: if two goroutines both miss the cache and both observe ErrKeyNotExist before either Puts, then G1 Puts I1 and wins LoadOrStore while G2 Puts I2 (overwriting the outer reference) and then loses LoadOrStore and closes I2. The cache returns I1, but the outer — and the kernel datapath — references I2, whose only userspace FD was just closed. Policy applied through the cached I1 is invisible to the datapath and I2 is orphaned. The comment at lines 34-36 documents the "no concurrent ownership" assumption, but nothing here enforces it, and the DNS reaper now runs concurrently on a ticker (though it passes nil newInner). Since the failure mode is silent policy divergence, consider a per-key singleflight or a test that drives two concurrent creators for the same key to pin down the expected behavior.
| mvmMeta mvmMetadata | ||
| ) | ||
| iter := meta.Iterate() | ||
| for iter.Next(&ifindex, &mvmMeta) { |
There was a problem hiding this comment.
The fast path only reaps ifindices present in ifindex_to_mvmmeta. Any allow_out_v3 outer key whose ifindex lost its metadata (invariant violation, e.g. a partially-cleaned TAP) is never visited here, and the fallback reapDNSLearnedPoliciesFromAllowOutOuter only triggers when the metadata map fails to load, not when a specific key is missing. The previous code iterated allow_out_v3 outers directly and reaped every ifindex with an outer key, so expired DNS-learned entries for metadata-less ifindices now persist until the next startup stale-GC. Worth either iterating the outer when the two maps disagree, or documenting the invariant. Also note this new fast path (and the fallback trigger) has no direct unit test — the kept reapDNSLearnedPoliciesForInnerMap is exercised, but not the mvmmeta iteration.
| // churn and surfaces as netlink ErrDumpInterrupted under density load. | ||
| func (s *NetworkController) cleanupConflictingTap(ip net.IP) error { | ||
| taps, err := s.tapAdapter.List() | ||
| tap, err := s.tapAdapter.GetByName(tapName(ip.String())) |
There was a problem hiding this comment.
The comment above claims this lookup is "by deterministic tap name (RTM_GETLINK), not LinkList," and the PR description credits GetByName with "stop doing a full LinkList on every pool-miss create." Please verify against the vendored vishvananda/netlink v1.3.1 (per Cubelet/go.mod): netlink.LinkByName is implemented via LinkList() plus a linear search — it is not a single RTM_GETLINK. If so, GetByName still performs a full link dump on every pool-miss create, and the latency win here comes from WithDumpRetry on interruption rather than from avoiding the dump. (The PR's own tap_device.go comment labels netlinkLinkByName a "dump-style read" that needs ErrDumpInterrupted retry — which is consistent with the dump-based implementation and contradicts this comment.)
3 participants
Summary
Follow-up to the network-agent → Cubelet embed refactor (#1285). This PR tightens create-path latency when network policy is enabled, and hardens TAP allocation under high-concurrency density load.
Lookups that triggersynchronize_rcu(tens of ms). Delete policy maps on TAP destroy, and limit the DNS reaper to live ifindices so create latency with policy stays near the no-policy baseline.ErrDumpInterrupted/EINTR, and stop doing a fullLinkListon every pool-miss create —cleanupConflictingTaponly needs a by-name lookup. This stopsEnsureNetworkfailing with "results may be incomplete or inconsistent" under concurrent create pressure.