in_podman_metrics: fix multiple cgroup v2 issues#11719
Open
stondo wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughCPU unit conversion made cgroup-version-aware; image-name extraction defaults to "unknown" when metadata end-token is missing; added cgroup v2 RSS key and adjusted alternate PIDs path; introduced a sysfs reader that treats cgroup v2 "max" as unlimited (0) and is used for memory limits. ChangesCPU & container parsing
Configuration constants
Cgroup v2 sysfs handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
stondo
force-pushed
the
fix/podman-metrics-cgroupv2
branch
from
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
Author
|
Addressed both CodeRabbit review comments in commit a35a87d:
|
Contributor
|
The last comment does not have Signed-off line so we need to add it at first. |
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>
stondo
force-pushed
the
fix/podman-metrics-cgroupv2
branch
from
a35a87d to
acaa540
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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