must-gather/gather_sriov: parallelize and optimize per-node SR-IOV collection - #540
Open
gavrielg1 wants to merge 3 commits into
Open
must-gather/gather_sriov: parallelize and optimize per-node SR-IOV collection#540gavrielg1 wants to merge 3 commits into
gavrielg1 wants to merge 3 commits into
Conversation
openshift-ci
Bot
added
the
needs-ok-to-test
Contributor
|
Hi @gavrielg1. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
gavrielg1
changed the title
WalkthroughAdds SRIOV_JOURNAL_SINCE and kernel_journal_since_args(); parallelizes oc adm inspect; refactors per-pod collection into gather_sriov_for_pod() that runs concurrent per-pod captures (journalctl -k --since, multus, netns, ethtool, BF/lspci, service journals) and tracks PIDs. ChangesSR-IOV must-gather
Sequence Diagram(s)sequenceDiagram
participant Script as gather_sriov
participant OC as oc (CLI)
participant Pod as Daemon Pod
participant FS as Collection Directory
Script->>OC: query resources (pods, must-gather resources)
alt resources found
Script->>OC: launch `oc adm inspect` per resource (background, track PIDs)
end
Script->>OC: for each pod -> start gather_sriov_for_pod(pod) (background)
par per-pod concurrent work
OC->>Pod: oc exec -> journalctl -k --since=...
OC->>Pod: oc exec -> test/copy /var/log/multus.log
OC->>Pod: oc exec -> run netns/ethtool/bf helper scripts
OC->>Pod: oc exec -> journalctl -u <service> [-b -N]
OC->>Pod: oc cp /var/lib/cni/sriov -> FS (non-fatal)
Pod-->>FS: write gzipped outputs (dmesg.gz, multus-log.gz, netns, ethtool, hw info)
end
Script->>Script: wait on accumulated PIDs (inspects + per-pod jobs)
OC-->>Script: return statuses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 (1)
collection-scripts/gather_sriov (1)
160-170: Consider putting a cap on the new parallel fan-out.Each daemon pod now spawns multiple concurrent
oc exec/oc cpcalls, and the outer loop runs every pod in parallel too. On larger clusters that can turn into hundreds of simultaneous API calls from one must-gather, which makes throttling/timeouts more likely.Also applies to: 175-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collection-scripts/gather_sriov` around lines 160 - 170, The parallel fan-out spawns unbounded background jobs (gather_netns_ip_a, gather_ethtool, gather_bf_info, gather_hardware_info) and appends PIDs to gather_sriov_pids which can create hundreds of simultaneous oc calls; cap concurrency by introducing a semaphore/worker limit (e.g., MAX_JOBS variable) and run these gather_* calls through a helper that acquires/releases the slot (or use xargs -P / a job queue) so only N oc exec/oc cp jobs run concurrently per must-gather run, and update places where these functions are launched (the calls that append to gather_sriov_pids around lines shown) to use that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@collection-scripts/gather_sriov`:
- Around line 147-154: The gzip pipelines for oc exec (writing to
"${DMESG_LOG_PATH}" and "${MULTUS_LOG_PATH}" and adding PIDs to
gather_sriov_pids) can produce empty .gz files when oc exec fails or the file is
missing; change each pipeline to first run the oc exec/chroot command and
capture its exit status and output to a temporary file (or check its exit code)
before compressing to the final "${DMESG_LOG_PATH}" / "${MULTUS_LOG_PATH}", only
start gzip (and only append to gather_sriov_pids) if oc exec succeeded and the
output is non-empty; if oc exec fails or the file is missing, remove any
partial/temp files and do not add the background PID to gather_sriov_pids (refer
to CONFIG_DAEMON_POD, gather_sriov_pids, DMESG_LOG_PATH, and MULTUS_LOG_PATH to
locate the affected blocks).
- Around line 27-33: The CONFIG_DAEMON_PODS array is being filled from command
substitution of "oc -n \"${operator_ns}\" get pods ..." but the script never
checks whether that oc command succeeded, so failures are treated as “no pods
found”; change the logic to run the oc command separately, capture its output
and exit code (e.g., store output in a variable like PODS_OUT and check $?),
then only populate CONFIG_DAEMON_PODS from PODS_OUT if the oc call succeeded; if
oc failed, log an error mentioning the oc failure and exit non‑zero instead of
printing the harmless skip and exiting 0. Ensure you reference the existing
symbols operator_ns and CONFIG_DAEMON_PODS and preserve the existing behavior
when oc succeeds (empty pod list -> skip with exit 0).
---
Nitpick comments:
In `@collection-scripts/gather_sriov`:
- Around line 160-170: The parallel fan-out spawns unbounded background jobs
(gather_netns_ip_a, gather_ethtool, gather_bf_info, gather_hardware_info) and
appends PIDs to gather_sriov_pids which can create hundreds of simultaneous oc
calls; cap concurrency by introducing a semaphore/worker limit (e.g., MAX_JOBS
variable) and run these gather_* calls through a helper that acquires/releases
the slot (or use xargs -P / a job queue) so only N oc exec/oc cp jobs run
concurrently per must-gather run, and update places where these functions are
launched (the calls that append to gather_sriov_pids around lines shown) to use
that helper.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c27e9e25-cbbb-4c83-813c-64bf0370e04a
📒 Files selected for processing (1)
collection-scripts/gather_sriov
gavrielg1
force-pushed
the
improve-gather-sriov
branch
from
April 19, 2026 10:06
770dd28 to
be23579
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
collection-scripts/gather_sriov (3)
21-25: Use${BASE_COLLECTION_PATH}instead of hardcodingmust-gather.
BASE_COLLECTION_PATHis defined at line 4 precisely for this; hardcoding it here diverges from the rest of the script and makes it painful to re‑target the destination.Proposed change
- oc adm inspect ${log_collection_args} --dest-dir must-gather --all-namespaces ${resource} & + oc adm inspect ${log_collection_args} --dest-dir "${BASE_COLLECTION_PATH}" --all-namespaces ${resource} &🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collection-scripts/gather_sriov` around lines 21 - 25, The oc adm inspect call hardcodes "must-gather" as the destination while the script defines BASE_COLLECTION_PATH earlier; update the oc adm inspect invocation inside the for loop (where resources is iterated and PIDS is appended) to use ${BASE_COLLECTION_PATH} instead of "must-gather" so all collections target the configured base path consistently.
124-175: Indentation is inconsistent insidegather_sriov_for_pod.Lines 124, 165, 168, 171 use spaces (2 or tab+space) while the rest of the function uses tabs. Not a bug, but the mixed style is noisy in diffs and makes future edits harder to line up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collection-scripts/gather_sriov` around lines 124 - 175, The indentation inside gather_sriov_for_pod is mixed (some lines use spaces) — normalize to the file's tab-based style: replace the leading spaces before the local declaration and the lines that start the background calls (gather_netns_ip_a, gather_ethtool, gather_bf_info, gather_hardware_info and their gather_sriov_pids+=($! ) lines) with tabs so they align with the other tab-indented blocks; keep existing trailing whitespace and ampersands unchanged and ensure indentation for the oc cp and previous command groups matches the surrounding tab indentation.
27-36: Silent exit when no daemon pods are found.When
oc get podssucceeds but returns an empty list,CONFIG_DAEMON_PODSbecomes an empty array and the per‑pod loop is simply skipped with no log line — the previous behavior printed a clear "no pods found" skip message. Consider keeping an informational message so users can distinguish "operator not installed / no daemonset pods scheduled" from "script silently did nothing".Possible addition
read -r -a CONFIG_DAEMON_PODS <<< "${pod_names}" + if [ "${`#CONFIG_DAEMON_PODS`[@]}" -eq 0 ]; then + echo "INFO: no sriov-network-config-daemon pods found in namespace ${operator_ns}; skipping per-pod collection" + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collection-scripts/gather_sriov` around lines 27 - 36, When `oc -n "${operator_ns}" get pods -l app=sriov-network-config-daemon` returns no pods the script silently proceeds with an empty CONFIG_DAEMON_PODS; after you populate pod_names (or after read -r -a CONFIG_DAEMON_PODS <<< "${pod_names}") check if CONFIG_DAEMON_PODS is empty (e.g. if [ "${`#CONFIG_DAEMON_PODS`[@]}" -eq 0 ]); if so, print an informational message like "INFO: no sriov-network-config-daemon pods found in ${operator_ns}, skipping" and exit 0 (or return) so callers see a clear skip message instead of silent no-op. Ensure this uses the same operator_ns and label selectors (app=sriov-network-config-daemon) and does not treat an empty list as an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@collection-scripts/gather_sriov`:
- Around line 135-151: The current use of printf '%q' to build since_quoted
collapses multi-token results from kernel_journal_since_args into a single
argument, causing journalctl to receive a malformed timestamp string; change the
code so kernel_journal_since_args is passed as true separate tokens to the inner
shell: either assign a simple string (since_args="$(kernel_journal_since_args)")
and let the inner chroot/bash command use unquoted expansion (journalctl -k
${since_args} --no-pager) if kernel_journal_since_args is guaranteed to emit a
single token, or return an array (since_args=( $(kernel_journal_since_args) ) or
have kernel_journal_since_args itself populate an array) and expand it as
"${since_args[@]}" in the journalctl invocation inside the subshell that creates
DMESG_LOG_PATH; update references to since_quoted and the oc exec / journalctl
command accordingly so journalctl receives separate --since/--until tokens.
---
Nitpick comments:
In `@collection-scripts/gather_sriov`:
- Around line 21-25: The oc adm inspect call hardcodes "must-gather" as the
destination while the script defines BASE_COLLECTION_PATH earlier; update the oc
adm inspect invocation inside the for loop (where resources is iterated and PIDS
is appended) to use ${BASE_COLLECTION_PATH} instead of "must-gather" so all
collections target the configured base path consistently.
- Around line 124-175: The indentation inside gather_sriov_for_pod is mixed
(some lines use spaces) — normalize to the file's tab-based style: replace the
leading spaces before the local declaration and the lines that start the
background calls (gather_netns_ip_a, gather_ethtool, gather_bf_info,
gather_hardware_info and their gather_sriov_pids+=($! ) lines) with tabs so they
align with the other tab-indented blocks; keep existing trailing whitespace and
ampersands unchanged and ensure indentation for the oc cp and previous command
groups matches the surrounding tab indentation.
- Around line 27-36: When `oc -n "${operator_ns}" get pods -l
app=sriov-network-config-daemon` returns no pods the script silently proceeds
with an empty CONFIG_DAEMON_PODS; after you populate pod_names (or after read -r
-a CONFIG_DAEMON_PODS <<< "${pod_names}") check if CONFIG_DAEMON_PODS is empty
(e.g. if [ "${`#CONFIG_DAEMON_PODS`[@]}" -eq 0 ]); if so, print an informational
message like "INFO: no sriov-network-config-daemon pods found in ${operator_ns},
skipping" and exit 0 (or return) so callers see a clear skip message instead of
silent no-op. Ensure this uses the same operator_ns and label selectors
(app=sriov-network-config-daemon) and does not treat an empty list as an error.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d3e9f39-1080-4a8d-8a8d-51b7eaa031ea
📒 Files selected for processing (1)
collection-scripts/gather_sriov
gavrielg1
force-pushed
the
improve-gather-sriov
branch
from
April 19, 2026 10:36
be23579 to
398b99c
Compare
gavrielg1
force-pushed
the
improve-gather-sriov
branch
from
April 19, 2026 10:51
398b99c to
95dd7a1
Compare
In case systemd mode enabled sriov logs collected by journalctl with max boots of 5
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
collection-scripts/gather_sriov (1)
98-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse
mstconfigfailures into "no devices detected".This command substitution only preserves the final
awkstatus, so anoc execormstconfig qfailure still leavesdevicesempty and hits the "No Mellanox devices detected" path. That makes a collection failure look like a clean "no hardware" result.Suggested fix
- local devices - devices=$(oc exec -n "${operator_ns}" "${pod}" -c sriov-network-config-daemon -- mstconfig q 2>/dev/null | grep "Device:" | awk 'BEGIN {FS="/";} { print $6 }') + local mstconfig_output devices + if ! mstconfig_output=$(oc exec -n "${operator_ns}" "${pod}" -c sriov-network-config-daemon -- mstconfig q 2>/dev/null); then + echo "Failed to query Mellanox devices via mstconfig" | tee -a "${MSTCONFIG_LOG_PATH}" "${MSTFLINT_LOG_PATH}" >/dev/null + return + fi + devices=$(printf '%s\n' "${mstconfig_output}" | awk '/Device:/ { split($0, parts, "/"); print parts[6] }') if [ -z "${devices}" ]; then echo "No Mellanox devices detected by mstconfig" | tee -a "${MSTCONFIG_LOG_PATH}" "${MSTFLINT_LOG_PATH}" >/dev/null return fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@collection-scripts/gather_sriov` around lines 98 - 103, The command substitution that sets the devices variable hides failures from oc exec or mstconfig because only the final awk exit code is kept; change the logic so failures are detected explicitly: either enable pipefail (set -o pipefail) in this scope before running the pipeline or run oc exec and mstconfig in separate commands, capture their exit codes and stderr to MSTCONFIG_LOG_PATH, and only run the awk extraction if the preceding commands succeeded; ensure you check the exit status of the oc exec/mstconfig steps (the commands that feed into awk) and log their errors instead of falling through to the "No Mellanox devices detected" message when they fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@collection-scripts/gather_sriov`:
- Around line 226-228: The loop that starts gather_sriov_for_pod for each
CONFIG_DAEMON_POD currently launches all pods in parallel and can cause
unbounded nested parallelism; introduce a concurrency limit (e.g.,
MAX_POD_FANOUT) and throttle the loop so you only start a new background
gather_sriov_for_pod when the number of in-flight jobs is below that limit—use
the existing PIDS array (or job control with wait -n) to track running jobs,
wait for one to finish when the limit is reached, then push the new PID into
PIDS; update the loop that iterates CONFIG_DAEMON_PODS and any bookkeeping
around PIDS accordingly.
- Around line 198-201: The chrooted command in the block that collects
multus.log uses a relative path "var/log/multus.log", which can fail if the
chrooted process's working directory is not root; update the chrooted test and
cat invocations executed by the oc exec for CONFIG_DAEMON_POD (container
sriov-network-config-daemon in namespace operator_ns) to use the absolute path
/var/log/multus.log so MULTUS_LOG_PATH reliably receives the gzipped content
when the file exists.
---
Duplicate comments:
In `@collection-scripts/gather_sriov`:
- Around line 98-103: The command substitution that sets the devices variable
hides failures from oc exec or mstconfig because only the final awk exit code is
kept; change the logic so failures are detected explicitly: either enable
pipefail (set -o pipefail) in this scope before running the pipeline or run oc
exec and mstconfig in separate commands, capture their exit codes and stderr to
MSTCONFIG_LOG_PATH, and only run the awk extraction if the preceding commands
succeeded; ensure you check the exit status of the oc exec/mstconfig steps (the
commands that feed into awk) and log their errors instead of falling through to
the "No Mellanox devices detected" message when they fail.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1c733888-dd94-4f4e-961f-8efc98beea60
📒 Files selected for processing (1)
collection-scripts/gather_sriov
Comment on lines
+198
to
+201
| # Collect multus.log if exists. | ||
| ( set -o pipefail; oc exec -n "${operator_ns}" "${CONFIG_DAEMON_POD}" -c sriov-network-config-daemon -- chroot /host /bin/bash -c \ | ||
| "test -f var/log/multus.log && cat var/log/multus.log" 2>/dev/null | gzip >"${MULTUS_LOG_PATH}" ) \ | ||
| || rm -f "${MULTUS_LOG_PATH}" & |
There was a problem hiding this comment.
Use an absolute path for multus.log inside the chroot.
Line 200 checks var/log/multus.log, so this capture depends on the daemon container's working directory after chroot. If that directory is not /, the probe fails and multus-log.gz gets dropped even though /var/log/multus.log exists.
Suggested fix
( set -o pipefail; oc exec -n "${operator_ns}" "${CONFIG_DAEMON_POD}" -c sriov-network-config-daemon -- chroot /host /bin/bash -c \
- "test -f var/log/multus.log && cat var/log/multus.log" 2>/dev/null | gzip >"${MULTUS_LOG_PATH}" ) \
+ "test -f /var/log/multus.log && cat /var/log/multus.log" 2>/dev/null | gzip >"${MULTUS_LOG_PATH}" ) \
|| rm -f "${MULTUS_LOG_PATH}" &📝 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.
Suggested change
| # Collect multus.log if exists. | |
| ( set -o pipefail; oc exec -n "${operator_ns}" "${CONFIG_DAEMON_POD}" -c sriov-network-config-daemon -- chroot /host /bin/bash -c \ | |
| "test -f var/log/multus.log && cat var/log/multus.log" 2>/dev/null | gzip >"${MULTUS_LOG_PATH}" ) \ | |
| || rm -f "${MULTUS_LOG_PATH}" & | |
| # Collect multus.log if exists. | |
| ( set -o pipefail; oc exec -n "${operator_ns}" "${CONFIG_DAEMON_POD}" -c sriov-network-config-daemon -- chroot /host /bin/bash -c \ | |
| "test -f /var/log/multus.log && cat /var/log/multus.log" 2>/dev/null | gzip >"${MULTUS_LOG_PATH}" ) \ | |
| || rm -f "${MULTUS_LOG_PATH}" & |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@collection-scripts/gather_sriov` around lines 198 - 201, The chrooted command
in the block that collects multus.log uses a relative path "var/log/multus.log",
which can fail if the chrooted process's working directory is not root; update
the chrooted test and cat invocations executed by the oc exec for
CONFIG_DAEMON_POD (container sriov-network-config-daemon in namespace
operator_ns) to use the absolute path /var/log/multus.log so MULTUS_LOG_PATH
reliably receives the gzipped content when the file exists.
Comment on lines
+226
to
+228
| for CONFIG_DAEMON_POD in "${CONFIG_DAEMON_PODS[@]}"; do | ||
| gather_sriov_for_pod "${CONFIG_DAEMON_POD}" & | ||
| PIDS+=($!) |
There was a problem hiding this comment.
Bound the outer pod fan-out before this overwhelms larger clusters.
Each gather_sriov_for_pod already starts a large batch of background oc exec/oc cp calls. Launching one per daemon pod with no limit turns this into unbounded nested parallelism, which can easily swamp the API server on bigger fleets or during degraded must-gather runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@collection-scripts/gather_sriov` around lines 226 - 228, The loop that starts
gather_sriov_for_pod for each CONFIG_DAEMON_POD currently launches all pods in
parallel and can cause unbounded nested parallelism; introduce a concurrency
limit (e.g., MAX_POD_FANOUT) and throttle the loop so you only start a new
background gather_sriov_for_pod when the number of in-flight jobs is below that
limit—use the existing PIDS array (or job control with wait -n) to track running
jobs, wait for one to finish when the limit is reached, then push the new PID
into PIDS; update the loop that iterates CONFIG_DAEMON_PODS and any bookkeeping
around PIDS accordingly.
SchSeba
approved these changes
Jun 24, 2026
SchSeba
left a comment
SchSeba
left a comment
There was a problem hiding this comment.
not a must-gather expert here but from sriov-operator side this LGTM it collects all the information we need to be able and debug customer issues
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gavrielg1, SchSeba The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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.
CNF Story for reference - CNF-22983
Summary by CodeRabbit
New Features
Performance
Improvements