Collect pod resource usage and cgroup metrics in must-gather#2494
Collect pod resource usage and cgroup metrics in must-gather#2494evanwang9x wants to merge 2 commits into
Conversation
| for flag in "${flags[@]}"; do | ||
| oc exec -n "${sitens}" "${routerName}" -c router -- skstat -"${flag}" > "${logPath}/skstat.${flag}" 2>&1 | ||
| done | ||
| getPodResources "${sitens}" "${routerName}" "router" |
There was a problem hiding this comment.
Suggest that resources be retrieved for all skupper containers not just the router
e.g. getSkupperResourcesInNamespace iterates over label app.kubernetes.io/part-of: skupper
that then calls to getPodResources and iterates across all containers in the pod...
There was a problem hiding this comment.
I agree with Andy, this is the only observation I have as well.
But the data being retrieved from the router container looks good to me.
Just add all the other possible containers into the list and we should be good to merge it.
📝 WalkthroughWalkthroughAdds resource collection functions to the Skupper diagnostic gather script. A new ChangesSkupper Gather Script Resource Collection
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/skupper-gather.sh (2)
50-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the already-computed
podDirNameinstead of recomputing.
podDirNameis computed at Line 23 with the same expression; recompute is redundant.♻️ Proposed fix
-oc get pod "${podName##"pod/"}" -n "${ns}" -o jsonpath="{.status.containerStatuses[?(@.name=='${container}')].lastState}" > "${resourcePath}/last.terminated" 2>/dev/null +oc get pod "${podDirName}" -n "${ns}" -o jsonpath="{.status.containerStatuses[?(@.name=='${container}')].lastState}" > "${resourcePath}/last.terminated" 2>/dev/null
56-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate PSI-check/limit pattern across three blocks.
Memory pressure, cpu.max, and cpu.pressure each re-implement a similar "check file exists, else print PSI-unavailable message" structure via separate
oc execcalls. Extracting a small helper (taking the sysfs path and output file) would reduce duplication and make future cgroup additions easier.Also applies to: 93-103, 106-112
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8d4179f-3155-4a80-860d-11ec3162c41c
📒 Files selected for processing (1)
scripts/skupper-gather.sh
| # Memory Info | ||
| oc exec -n "${ns}" "${podName}" -c "${container}" -- bash -c ' | ||
| curr=$(cat /sys/fs/cgroup/memory.current) | ||
| max=$(cat /sys/fs/cgroup/memory.max) | ||
| curr_kb=$((curr / 1024)) | ||
| curr_mb=$((curr / 1024 / 1024)) | ||
|
|
||
| if [ "$max" = "max" ]; then | ||
| limit_str="max (uncapped)" | ||
| else | ||
| max_kb=$((max / 1024)) | ||
| max_mb=$((max / 1024 / 1024)) | ||
| limit_str="$max bytes ($max_kb KiB / $max_mb MiB)" | ||
| fi | ||
|
|
||
| echo "Usage: $curr bytes ($curr_kb KiB / $curr_mb MiB) - Limit: $limit_str" > /tmp/memory.info | ||
| ' 2>/dev/null | ||
| oc exec -n "${ns}" "${podName}" -c "${container}" -- cat /tmp/memory.info > "${resourcePath}/memory.info" 2>&1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid the intermediate /tmp file and second oc exec round trip.
Unlike the other cgroup blocks (pressure, cpu.stat, cpu.max) which write directly to stdout and are captured via output redirection in a single oc exec, memory.info is written to a hardcoded /tmp/memory.info inside the container and then read back with a second oc exec. Static analysis flags this as an insecure/predictable temp file (CWE-377), and it also doubles the exec calls needlessly.
🛡️ Proposed fix: write directly to stdout like the other blocks
# Memory Info
- oc exec -n "${ns}" "${podName}" -c "${container}" -- bash -c '
+ oc exec -n "${ns}" "${podName}" -c "${container}" -- bash -c '
curr=$(cat /sys/fs/cgroup/memory.current)
max=$(cat /sys/fs/cgroup/memory.max)
curr_kb=$((curr / 1024))
curr_mb=$((curr / 1024 / 1024))
if [ "$max" = "max" ]; then
limit_str="max (uncapped)"
else
max_kb=$((max / 1024))
max_mb=$((max / 1024 / 1024))
limit_str="$max bytes ($max_kb KiB / $max_mb MiB)"
fi
- echo "Usage: $curr bytes ($curr_kb KiB / $curr_mb MiB) - Limit: $limit_str" > /tmp/memory.info
- ' 2>/dev/null
- oc exec -n "${ns}" "${podName}" -c "${container}" -- cat /tmp/memory.info > "${resourcePath}/memory.info" 2>&1
+ echo "Usage: $curr bytes ($curr_kb KiB / $curr_mb MiB) - Limit: $limit_str"
+ ' > "${resourcePath}/memory.info" 2>&1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Memory Info | |
| oc exec -n "${ns}" "${podName}" -c "${container}" -- bash -c ' | |
| curr=$(cat /sys/fs/cgroup/memory.current) | |
| max=$(cat /sys/fs/cgroup/memory.max) | |
| curr_kb=$((curr / 1024)) | |
| curr_mb=$((curr / 1024 / 1024)) | |
| if [ "$max" = "max" ]; then | |
| limit_str="max (uncapped)" | |
| else | |
| max_kb=$((max / 1024)) | |
| max_mb=$((max / 1024 / 1024)) | |
| limit_str="$max bytes ($max_kb KiB / $max_mb MiB)" | |
| fi | |
| echo "Usage: $curr bytes ($curr_kb KiB / $curr_mb MiB) - Limit: $limit_str" > /tmp/memory.info | |
| ' 2>/dev/null | |
| oc exec -n "${ns}" "${podName}" -c "${container}" -- cat /tmp/memory.info > "${resourcePath}/memory.info" 2>&1 | |
| # Memory Info | |
| oc exec -n "${ns}" "${podName}" -c "${container}" -- bash -c ' | |
| curr=$(cat /sys/fs/cgroup/memory.current) | |
| max=$(cat /sys/fs/cgroup/memory.max) | |
| curr_kb=$((curr / 1024)) | |
| curr_mb=$((curr / 1024 / 1024)) | |
| if [ "$max" = "max" ]; then | |
| limit_str="max (uncapped)" | |
| else | |
| max_kb=$((max / 1024)) | |
| max_mb=$((max / 1024 / 1024)) | |
| limit_str="$max bytes ($max_kb KiB / $max_mb MiB)" | |
| fi | |
| echo "Usage: $curr bytes ($curr_kb KiB / $curr_mb MiB) - Limit: $limit_str" | |
| ' > "${resourcePath}/memory.info" 2>&1 |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 46-46: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/memory.info
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
Source: Linters/SAST tools
| # CPU usage | ||
| oc exec -n "${ns}" "${podName}" -c "${container}" -- bash -c " | ||
| declare -A cpu_descriptions | ||
| cpu_descriptions=( | ||
| [usage_usec]='Total CPU time consumed (user + system)' | ||
| [user_usec]='Time spent in user-space (app logic)' | ||
| [system_usec]='Time spent in kernel-space (syscalls, I/O)' | ||
| [core_sched.force_idle_usec]='Time CPU forced idle for security (cross-HT side-channel protection)' | ||
| [nr_periods]='Number of CPU quota enforcement periods (0 = no quota set)' | ||
| [nr_throttled]='Times the container was throttled for exceeding CPU quota' | ||
| [throttled_usec]='Total time spent throttled/waiting for CPU quota' | ||
| [nr_bursts]='Times container used burst CPU capacity beyond quota' | ||
| [burst_usec]='Total time spent using burst CPU capacity' | ||
| ) | ||
|
|
||
| cat /sys/fs/cgroup/cpu.stat | while read line; do | ||
| val=\$(echo \$line | awk '{print \$2}') | ||
| key=\$(echo \$line | awk '{print \$1}') | ||
| desc=\${cpu_descriptions[\$key]} | ||
| if [[ \$key == *_usec ]] && [ \$val -ge 1000000 ]; then | ||
| sec=\$(awk \"BEGIN {printf \\\"%.2f\\\", \$val/1000000}\") | ||
| echo \"\$line (\$sec s) | \$desc\" | ||
| else | ||
| echo \"\$line | \$desc\" | ||
| fi | ||
| done | ||
| " > "${resourcePath}/cpu.stat" 2>&1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the Skupper container images used across pods to check shell availability assumptions.
rg -n 'FROM ' --type=dockerfile
fd -e DockerfileRepository: skupperproject/skupper
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant call sites ==\n'
rg -n 'getSkupperResourcesInNamespace|getPodResources|bash -c|declare -A|cpu\.stat' scripts/skupper-gather.sh
printf '\n== file map ==\n'
ast-grep outline scripts/skupper-gather.sh --view expanded
printf '\n== container/image hints in repo ==\n'
rg -n 'Dockerfile\.(kube-adaptor|controller|cli|system-controller|network-observer)|FROM ' -g '!**/node_modules/**' .Repository: skupperproject/skupper
Length of output: 2093
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,170p' scripts/skupper-gather.sh | cat -nRepository: skupperproject/skupper
Length of output: 7333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== script excerpt ==\n'
sed -n '1,170p' scripts/skupper-gather.sh | cat -nRepository: skupperproject/skupper
Length of output: 7355
oc exec -- bash -c does not work for the scratch-based Skupper images.
getSkupperResourcesInNamespace now iterates every container in every app.kubernetes.io/part-of=skupper pod, and the final images for Dockerfile.cli, Dockerfile.controller, Dockerfile.kube-adaptor, Dockerfile.network-observer, and Dockerfile.system-controller are FROM scratch. Those containers won’t have bash, so resource collection fails for them; a POSIX sh path or another fallback is needed.
3 participants
Must gather script can now collect memory and CPU usage via container exec and also captures cgroup limits, requests, and throttling stats.
Outputs the info to "must-gather/namespaces/"namespace"/pods/"pod-name"/"container-name"/"container-name"/resources/". Creates files to track what each does
To run the script:
Summary by CodeRabbit