in_podman_metrics: fix multiple cgroup v2 issues - #11719
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPodman metrics now guard image-name parsing, convert CPU values by cgroup version, correct cgroup v2 keys and paths, and interpret ChangesPodman metrics updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This change corrects cgroup v2 CPU, memory, limit, and network metric collection without a supplied indication of new correctness or deployment risk; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6c742a1 to
34b3477
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/in_podman_metrics/podman_metrics.c (1)
122-123: Avoid hard-coded terminator index for"unknown".Using a literal index is brittle; derive it from the string length for safer maintenance.
♻️ Suggested refactor
- strncpy(image_name, "unknown", IMAGE_NAME_SIZE - 1); - image_name[7] = '\0'; + strncpy(image_name, "unknown", IMAGE_NAME_SIZE - 1); + image_name[sizeof("unknown") - 1] = '\0';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_podman_metrics/podman_metrics.c` around lines 122 - 123, The code in podman_metrics.c sets image_name to "unknown" then hard-codes image_name[7] = '\0'; which is brittle; in the block that assigns image_name (use the same spot that calls strncpy(image_name, "unknown", IMAGE_NAME_SIZE - 1)), replace the fixed index with a derived terminator using the actual string length (e.g., size_t n = strlen("unknown"); if (n >= IMAGE_NAME_SIZE) n = IMAGE_NAME_SIZE - 1; image_name[n] = '\0') or use a bounded formatting function like snprintf to write and terminate safely; update the logic around image_name, IMAGE_NAME_SIZE, and the strncpy call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/in_podman_metrics/podman_metrics_data.c`:
- Around line 372-387: The function read_from_sysfs_or_max currently ignores
fgets() failures/empty reads; update it to explicitly handle fgets returning
NULL by checking feof()/ferror(), calling fclose(fp), logging a warning via
flb_plg_warn (include path and reason e.g., strerror(errno) or mention empty
file), and returning UINT64_MAX; keep the existing cgroup v2 "max" handling and
successful-read path intact and reference buf, fp, path, flb_plg_warn, and
flb_plg_debug to locate the change.
---
Nitpick comments:
In `@plugins/in_podman_metrics/podman_metrics.c`:
- Around line 122-123: The code in podman_metrics.c sets image_name to "unknown"
then hard-codes image_name[7] = '\0'; which is brittle; in the block that
assigns image_name (use the same spot that calls strncpy(image_name, "unknown",
IMAGE_NAME_SIZE - 1)), replace the fixed index with a derived terminator using
the actual string length (e.g., size_t n = strlen("unknown"); if (n >=
IMAGE_NAME_SIZE) n = IMAGE_NAME_SIZE - 1; image_name[n] = '\0') or use a bounded
formatting function like snprintf to write and terminate safely; update the
logic around image_name, IMAGE_NAME_SIZE, and the strncpy call accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11fd1877-05ab-4428-b4d0-5c5d7c213869
📒 Files selected for processing (3)
plugins/in_podman_metrics/podman_metrics.cplugins/in_podman_metrics/podman_metrics_config.hplugins/in_podman_metrics/podman_metrics_data.c
|
Addressed both CodeRabbit review comments in commit a35a87d:
|
|
The last comment does not have Signed-off line so we need to add it at first. |
a35a87d to
acaa540
Compare
Fix five bugs in the podman_metrics input plugin:
1. CPU counter division: cgroup v2 cpu.stat reports usage in
microseconds, not nanoseconds like cgroup v1 cpuacct.
Use the correct divisor (1e6) when converting to seconds.
2. RSS memory key: cgroup v2 memory.stat does not have a "rss"
field. The equivalent metric is "anon" (anonymous memory).
Add V2_STAT_KEY_RSS and use it in the v2 collection path.
3. memory.max "max" keyword: cgroup v2 uses the literal string
"max" in memory.max when the memory limit is unlimited.
read_from_file() fails to parse this with fscanf("%lu"),
causing spurious warnings. Add read_from_sysfs_or_max()
helper that returns 0 for "max" (unlimited).
4. PID alt path typo: V2_SYSFS_FILE_PIDS_ALT was set to
"containers/cgroup.procs" (plural) but the actual cgroup v2
subdirectory is "container/cgroup.procs" (singular). This
caused PID lookup to fail for all containers, which in turn
prevented all network metrics from being collected.
5. Image name NULL safety: when parsing container metadata JSON,
strstr() for the closing quote of the image name field can
return NULL if the metadata is malformed or truncated. The
result was used directly in pointer arithmetic and strncpy(),
causing undefined behaviour and potential crashes. Add a NULL
guard that falls back to image="unknown" when parsing fails.
GitHub issue fluent#7769.
Signed-off-by: stondo <stondo@gmail.com>
- read_from_sysfs_or_max: refactor to guard-clause pattern; return
UINT64_MAX immediately when fgets() returns NULL instead of silently
falling through and returning whatever value was last read.
- collect_container_data: use sizeof("unknown") - 1 instead of the
magic literal 7 when NUL-terminating image_name after strncpy so the
index stays correct if the fallback string ever changes.
Signed-off-by: Stefano Tondo <stondo@gmail.com>
acaa540 to
af0de5b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto current master, conflicts resolved ( |
Keep the new container/cgroup.procs fallback from the rebase, but also try the older containers/cgroup.procs layout still used by the runtime fixture and some Podman trees. This restores PID discovery for the cgroup-v2 test path without regressing the typo fix. Signed-off-by: Stefano Tondo <stondo@gmail.com>
|
Follow-up after the latest push ( The original regression from the rebase is fixed. The previously failing Ubuntu unit-test jobs are now green on the current head, including the The fix keeps the corrected The remaining red jobs do not look specific to this PR:
So the Linux regression introduced by the rebase appears resolved on the current branch head. |
2 participants
Summary
Fix four cgroup v2 bugs in the
in_podman_metricsinput plugin that causedmost container metrics to be absent or incorrect on systems using cgroup v2
(unified hierarchy).
All four bugs are confirmed on an ARM64 embedded device running
Fluent Bit 4.2.0/5.0.2 with Podman and cgroup v2 unified hierarchy.
Bugs fixed
1. CPU counter division (incorrect unit conversion)
create_counter()divides raw CPU values by 1,000,000,000 (nanoseconds)unconditionally. This is correct for cgroup v1 (
cpuacct.usagereportsnanoseconds) but wrong for cgroup v2 (
cpu.statreportsusage_usecand
user_usecin microseconds).On v2, integer division truncates all values below 1e9 usec (~16.7 min of
CPU time) to zero. In practice,
container_cpu_usage_seconds_totalandcontainer_cpu_user_seconds_totalalways read 0.Fix: Check
ctx->cgroup_versionand use the correct divisor:1e9 for v1 (nanoseconds), 1e6 for v2 (microseconds).
2. RSS memory key name (wrong key for v2)
STAT_KEY_RSSis defined as"rss", which is correct for cgroup v1memory.stat. However, cgroup v2memory.statdoes not have arssfield; the equivalent is
anon(anonymous memory pages).Result:
container_memory_rssgauge is never reported for anycontainer on v2, with a
[warn] rss not found in .../memory.statlogmessage emitted per container per scrape.
Fix: Add
V2_STAT_KEY_RSS "anon"and use it infill_counters_with_sysfs_data_v2().3. memory.max "max" keyword (parse failure)
cgroup v2
memory.maxcontains the literal string"max"when thememory limit is unlimited (no limit set).
read_from_file()usesfscanf(fp, "%lu", &value)which fails to parse"max", returningUINT64_MAX and logging a spurious warning per affected container per
scrape.
Result:
container_spec_memory_limit_bytesis missing forcontainers without an explicit memory limit. Warning spam in logs.
Fix: Add
read_from_sysfs_or_max()helper that parses the "max"keyword and returns 0 (unlimited), matching the convention used by
cAdvisor and other container metric exporters.
4. PID fallback path typo (singular vs. plural)
V2_SYSFS_FILE_PIDS_ALTis defined as"containers/cgroup.procs"(plural), but the actual cgroup v2 subdirectory is
"container/cgroup.procs"(singular).On v2,
cgroup.procsat the scope level is empty for all containers.Processes live only in the
container/cgroup.procssubdirectory. Theplugin correctly tries the alt path, but the typo means it always fails.
Result: PID lookup fails for all containers, which prevents
get_net_data_from_proc()from being called. All fourcontainer_network_*metrics are completely absent.Fix: Change
"containers/cgroup.procs"to"container/cgroup.procs".Before/After (gw-cloud-connector container, ARM64 device)
container_cpu_usage_seconds_totalcontainer_cpu_user_seconds_totalcontainer_memory_rsscontainer_spec_memory_limit_bytescontainer_network_receive_bytes_totalcontainer_network_transmit_bytes_totalAfter the fix, all 10 metric types are successfully emitted for all 9
containers, with zero warnings in the log.
Testing
Tested on:
Verified:
cpu.statdivided by 1e6anonfield inmemory.statFixes #7769
Summary by CodeRabbit