cgidmap: Add cgroupfs fallback for pod association - #5231
Conversation
Extract the queue/worker from the CRI-specific resolver into a backend-agnostic resolver parameterized by a container-path function, with the CRI as the only backend. Derive the cgroup id from the path based on whether cgroup tracker ids are enabled, and make cgtracker.AddCgroupTrackerPath a no-op when they are not, so callers cannot block on a map that is never created. Simplify the enqueue backpressure to uniformly drop the oldest ids over capacity, skip ids that are already pending, and wrap resolve errors with their failing step. Signed-off-by: Tam Mach <tam.mach@cilium.io>
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
sayboras
added
the
release-note/minor
sayboras
changed the title
Without --enable-cri, cgidmap could not associate existing pods. Add a cgroupfs-scan resolver backend and use it as the fallback when the CRI is disabled, mirroring policyfilter. Skip crio-conmon directories in the fsscan root scan and reject empty container ids in the scanner so a scan cannot return an unrelated cgroup. Add cgfs_cgidmap metrics and update the Helm chart note. Signed-off-by: Tam Mach <tam.mach@cilium.io>
Verify pod association with cgidmap enabled and the CRI disabled: a pod created before the agent starts must be associated through the cgroupfs-scan fallback resolver. Check that exec events carry pod info and that the cgfs_cgidmap resolution metrics increase. Signed-off-by: Tam Mach <tam.mach@cilium.io>
191b11f to
b76adaa
Compare
Why do you consider this best effort? What's an example scenario where cgroupfs-scan would not work correctly? Is it possible to make the cgoupfs-scan method reliable and always use that method regardless if |
| mu sync.Mutex | ||
| cond sync.Cond | ||
| // unresolvedIDs holds pending ids, oldest first; the worker pops the | ||
| // newest from the end (LIFO). |
There was a problem hiding this comment.
Why is LIFO preferable to FIFO?
There was a problem hiding this comment.
The thinking is that it's better to drop old request where things might have changed rather than new requests.
| Feature() | ||
|
|
||
| checkMetrics := features.New("Check cgfs resolver metrics"). | ||
| Assess("Wait for event checker", rpcChecker.Wait(60*time.Second)). |
There was a problem hiding this comment.
I think the idea here is that we want to check metrics after all the events have arrived.
But Wait() will return after the event checker starts, not after the event checker concludes. So, it seems like we can have a race where the event checker has started, but we haven't received events yet, we check metrics and see the value is 0, and then the first event is generated.
| return fmt.Errorf("get cgroup id: %w", err) | ||
| } | ||
|
|
||
| if err := cgtracker.AddCgroupTrackerPath(path); err != nil { |
There was a problem hiding this comment.
What if we pass this helper just like you did for criContainerPath? Ie: we do not invoke directly the cgtracker.Foo helper, but instead we receive the function to be called in the newResolver constructor?
At that point, you could drop AddCgroupTrackerPath changes and it's IMO cleaner since resolver is not going to depend upon cgtracker anymore.
| tetragon.WithHelmOptions(map[string]string{ | ||
| "tetragon.exportAllowList": "", | ||
| "tetragon.cgidmap.enabled": "true", | ||
| // tetragon.cri.enabled stays false (chart default) |
There was a problem hiding this comment.
I would be explicit; if we would ever update the default we would no more test the case here.
kkourt
left a comment
kkourt
left a comment
There was a problem hiding this comment.
Thanks for working on this!
cgidmap: generalize the async resolver
The above patch is not easy for me to review. Can you split it into patches: One where you introduce the "backend-agnostic" resolver (presumably, with no functional changes in the code), and one or more patches with the rest of the changes?
4 participants
Description
Without
--enable-cri, cgidmap could not associate existing pods. Generalize the async resolver into a backend-agnostic queue/worker and add a best-effort cgroupfs-scan fallback (mirroring policyfilter), so pod association keeps working for existing pods when the CRI is disabled. Covered by a new e2e test.Fixes #3381
Changelog