MG-233: compress service and window node logs - #554
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@praveencodes: This pull request references MG-233 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughService-log collection now accepts ChangesService log compression
Estimated code review effort: 2 (Simple) | ~8 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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_service_logs_util`:
- Around line 33-39: The issue is that when the oc adm node-logs commands on
lines 36 and 39 are piped through ${LOG_PIPE} and backgrounded, the wait command
will only capture the exit status of the last command in the pipeline (the
LOG_PIPE command), not the oc adm node-logs command itself. If oc adm node-logs
fails but LOG_PIPE succeeds, the overall exit status appears successful, masking
the failure. To fix this, enable pipefail mode at the beginning of the script
using set -o pipefail, which ensures that if any command in the pipeline fails,
the entire pipeline returns a non-zero exit status that will be properly
captured by wait.
In `@collection-scripts/gather_windows_node_logs`:
- Line 39: The backgrounded pipeline at line 39 containing the oc adm node-logs
command can hide failures from the oc command itself because the wait builtin
only checks the exit status of the last command in the pipeline (the
WIN_LOG_PIPE variable containing cat/gzip). To fix this, enable pipefail in the
script shell options so that if oc adm node-logs fails, the entire pipeline
fails and the error is not masked. Add set -o pipefail near the beginning of the
script before this command executes to ensure any failure in the oc adm
node-logs command will properly fail the backgrounded job and be caught by the
wait command.
🪄 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: 79cd11fd-66cb-43f7-a49e-20a3489955b0
📒 Files selected for processing (2)
collection-scripts/gather_service_logs_utilcollection-scripts/gather_windows_node_logs
| for service in "${@}"; do | ||
| echo "INFO: Collecting host service logs for $service" | ||
| if [[ ${selector} =~ '--role=' ]]; then | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -l kubernetes.io/os=linux -u "${service}" >"${DIR_PATH}/${service}_service.log" & | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -l kubernetes.io/os=linux -u "${service}" | ${LOG_PIPE} >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" & | ||
| PIDS+=($!) | ||
| else | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -u "${service}" >"${DIR_PATH}/${service}_service.log" & | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -u "${service}" | ${LOG_PIPE} >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" & |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
oc adm node-logs failures can be masked by the LOG_PIPE pipeline.
Because Line 36 and Line 39 run in a pipeline and are backgrounded, wait can report success from cat/gzip while oc adm node-logs failed, which weakens error detection in log collection.
Suggested fix
for service in "${@}"; do
echo "INFO: Collecting host service logs for $service"
if [[ ${selector} =~ '--role=' ]]; then
- oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -l kubernetes.io/os=linux -u "${service}" | ${LOG_PIPE} >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" &
+ if [[ "${LOG_PIPE}" == "cat" ]]; then
+ oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -l kubernetes.io/os=linux -u "${service}" >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" &
+ else
+ oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -l kubernetes.io/os=linux -u "${service}" | gzip >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" &
+ fi
PIDS+=($!)
else
- oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -u "${service}" | ${LOG_PIPE} >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" &
+ if [[ "${LOG_PIPE}" == "cat" ]]; then
+ oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -u "${service}" >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" &
+ else
+ oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -u "${service}" | gzip >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" &
+ fi
PIDS+=($!)
fi
done🤖 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_service_logs_util` around lines 33 - 39, The issue
is that when the oc adm node-logs commands on lines 36 and 39 are piped through
${LOG_PIPE} and backgrounded, the wait command will only capture the exit status
of the last command in the pipeline (the LOG_PIPE command), not the oc adm
node-logs command itself. If oc adm node-logs fails but LOG_PIPE succeeds, the
overall exit status appears successful, masking the failure. To fix this, enable
pipefail mode at the beginning of the script using set -o pipefail, which
ensures that if any command in the pipeline fails, the entire pipeline returns a
non-zero exit status that will be properly captured by wait.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== gather_service_logs_util outline =='
ast-grep outline collection-scripts/gather_service_logs_util --items all || true
printf '%s\n' '== collect_service_logs call sites and PID waits =='
rg -n -C 4 '\bcollect_service_logs\s*\(|\bwait\b|\bPIDS\b|set\s+(-o\s+pipefail|-[A-Za-z]*o?[A-Za-z]*\b)' \
-g '*.sh' -g '*.bats' .
printf '%s\n' '== pipefail declarations =='
rg -n -C 3 'pipefail' -g '*.sh' -g '*.bats' .Length of output: 5638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Files related to the service-log utility =='
fd -HI -t f . | rg '(^|/)(gather_service_logs_util|gather_service_logs|common)(\.[^/]+)?$|service_logs'
printf '%s\n' '== Source/call sites, PID initialization, and waits (all text files) =='
rg -n -i -C 5 \
'source[[:space:]].*gather_service_logs_util|(^|[[:space:];])\.[[:space:]].*gather_service_logs_util|\bcollect_service_logs[[:space:]]|(^|[^[:alnum:]_])PIDS=\(|\bwait[[:space:]].*PIDS|set[[:space:]]+(-[[:alpha:]]*o[[:alpha:]]*|-[[:space:]]*o[[:space:]]+pipefail)' \
--glob '!**/.git/**' .
printf '%s\n' '== Relevant production-script headers =='
for f in $(rg -l -i \
'source[[:space:]].*gather_service_logs_util|(^|[[:space:];])\.[[:space:]].*gather_service_logs_util|\bcollect_service_logs[[:space:]]' \
--glob '!**/.git/**' .); do
echo "--- $f"
sed -n '1,100p' "$f"
doneLength of output: 53367
@ardaguclu, yes—the underlying finding is valid. collection-scripts/gather_service_logs neither enables pipefail nor checks failures after Line 41, so a failed oc adm node-logs can appear successful when cat or gzip exits successfully.
One important correction: adding only set -o pipefail is not sufficient here:
- The script has no
errexitand does not inspectwait’s status, so it would still continue after failure. wait "${PIDS[@]}"returns only the status of the last PID; a failed earlier service-log collection can still be hidden.
The robust minimal approach is to enable pipefail in collection-scripts/gather_service_logs and wait for each PID individually, tracking/reporting any failures before exiting non-zero. The branch split in the original suggested patch does not solve the gzip pipeline case.
You are interacting with an AI system.
| LOG_FILE_DIR=${WINDOWS_NODE_LOGS}/log_files/$(dirname "$log") | ||
| mkdir -p "${LOG_FILE_DIR}" | ||
| /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" >"${LOG_FILE_DIR}/$(basename "$log")" & | ||
| /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" | ${WIN_LOG_PIPE} >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" & |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Backgrounded pipeline can hide oc adm node-logs errors.
At Line 39, wait tracks pipeline status from cat/gzip; failures from /usr/bin/oc adm node-logs may not fail the job, so missing logs can look successful.
Suggested fix
- /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" | ${WIN_LOG_PIPE} >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" &
+ if [[ "${WIN_LOG_PIPE}" == "cat" ]]; then
+ /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" &
+ else
+ /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" | gzip >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" &
+ fi📝 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.
| /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" | ${WIN_LOG_PIPE} >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" & | |
| if [[ "${WIN_LOG_PIPE}" == "cat" ]]; then | |
| /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" & | |
| else | |
| /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" | gzip >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" & | |
| 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_windows_node_logs` at line 39, The backgrounded
pipeline at line 39 containing the oc adm node-logs command can hide failures
from the oc command itself because the wait builtin only checks the exit status
of the last command in the pipeline (the WIN_LOG_PIPE variable containing
cat/gzip). To fix this, enable pipefail in the script shell options so that if
oc adm node-logs fails, the entire pipeline fails and the error is not masked.
Add set -o pipefail near the beginning of the script before this command
executes to ensure any failure in the oc adm node-logs command will properly
fail the backgrounded job and be caught by the wait command.
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| LOG_SUFFIX=".log" | ||
| LOG_PIPE="cat" | ||
| else | ||
| LOG_SUFFIX=".log.gz" |
There was a problem hiding this comment.
would this be problematic for customers with existing scripts who rely on these assumptions? should we gate it behind a flag?
There was a problem hiding this comment.
Added it behind the flag REDUCE_LOGS. It not accepts both skip_rotated_logs and compress_service_logs.
MG-233 / PR #554 — gzip host service logs size comparisonCompared baseline (stock Full
|
| Window | Baseline | PR #554 | Saved | Reduction |
|---|---|---|---|---|
24h (--since=24h) |
6.40 GiB | 6.38 GiB | ~23 MiB | ~0.35% |
30d (--since=720h) |
28.85 GiB | 28.39 GiB | ~472 MiB | ~1.6% |
host_service_logs only (subset of full gather)
| Window | Baseline | PR #554 | Saved | Reduction |
|---|---|---|---|---|
| 24h | 50.85 MiB | 3.56 MiB | ~47 MiB | ~93.0% |
| 30d | 450.22 MiB | 29.33 MiB | ~421 MiB | ~93.5% |
gather_service_logs only (masters)
| Window | Baseline | PR #554 | Reduction |
|---|---|---|---|
| 24h | 50.52 MiB | 3.42 MiB | 93.2% |
| 30d | 611.98 MiB | 42.60 MiB | 93.0% |
Largest contributors (masters, gather_service_logs):
- 24h: kubelet 49.42 → 3.30 MiB; crio 1.04 MiB → 116 KiB
- 30d: kubelet 575.93 → 38.90 MiB; crio 34.39 → 3.53 MiB
Notes
- Host service log compression is ~93% effective, but overall must-gather savings are small because
namespacesdominates (~28 GiB of the 30d artifact). - 30d PR full-artifact size was measured on-pod (local
occopy hung after gather completed); baseline 30d was from a completed local download earlier. gather_windows_node_logsnot size-tested (no Windows nodes).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
collection-scripts/gather_windows_node_logs (1)
33-35: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate background collection failures.
wait "${PIDS[@]}"only returns the last PID’s status, so an earlier failedoc adm node-logs | gzipjob can still reachsyncand print a successful completion message. Wait on each PID, accumulate failures, and exit non-zero if any job fails.🤖 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_windows_node_logs` around lines 33 - 35, Update the background process wait logic in gather_windows_node_logs to wait for every PID in PIDS and accumulate whether any job failed, rather than relying on wait "${PIDS[@]}" returning only the last status. Preserve the completion timing output, but exit non-zero after reporting completion when any collection job fails.
🤖 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.
Outside diff comments:
In `@collection-scripts/gather_windows_node_logs`:
- Around line 33-35: Update the background process wait logic in
gather_windows_node_logs to wait for every PID in PIDS and accumulate whether
any job failed, rather than relying on wait "${PIDS[@]}" returning only the last
status. Preserve the completion timing output, but exit non-zero after reporting
completion when any collection job fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 95db3cf0-e37d-48ff-90b6-5228192b178e
📒 Files selected for processing (1)
collection-scripts/gather_windows_node_logs
Keep host service logs uncompressed by default for existing tooling, and compress only when REDUCE_LOGS includes compress_service_logs (alone or with skip_rotated_logs). Co-authored-by: Cursor <cursoragent@cursor.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: praveencodes 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 |
|
/cc @ardaguclu |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/service_logs_util.bats (1)
153-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the non-role collection branch.
All new cases use
--role=master, so the changed non-role branch incollect_service_logsremains untested. Add a case using a node selector such astestnodefor both uncompressed and compressed output.Also applies to: 170-175, 187-192
🤖 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 `@tests/service_logs_util.bats` around lines 153 - 158, Extend the collect_service_logs tests to cover the non-role collection path by adding cases that pass a node selector such as testnode, verifying both uncompressed and compressed output. Update the scenarios corresponding to the default and compressed log tests while preserving their existing assertions and role-based coverage.
🤖 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/common.sh`:
- Around line 44-47: Update the REDUCE_LOGS parsing loop to tokenize the
comma/space-separated value into an array without pathname expansion, then
iterate over the resulting elements with quoted expansion. Preserve the existing
normalization and option validation behavior while removing the unquoted
${REDUCE_LOGS//,/ } iteration in the reduce_logs_option loop.
In `@tests/service_logs_util.bats`:
- Around line 159-162: Update the compression test cases around the PIDS wait
blocks to remove “|| true” so background failures propagate, and strengthen each
compressed-artifact assertion beyond file existence by validating the gzip
contents with gzip -t or gzip -dc. Apply the same changes to the cases at the
referenced later blocks while preserving the existing uncompressed and
absent-gzip checks.
---
Nitpick comments:
In `@tests/service_logs_util.bats`:
- Around line 153-158: Extend the collect_service_logs tests to cover the
non-role collection path by adding cases that pass a node selector such as
testnode, verifying both uncompressed and compressed output. Update the
scenarios corresponding to the default and compressed log tests while preserving
their existing assertions and role-based coverage.
🪄 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: f8894f8f-bf65-48eb-93d5-957d1f2f2e45
📒 Files selected for processing (4)
collection-scripts/common.shcollection-scripts/gather_service_logs_utiltests/common.batstests/service_logs_util.bats
| # Normalize commas to spaces, then validate each option. | ||
| # Intentional word-splitting of comma/space separated options. | ||
| # shellcheck disable=SC2086 | ||
| for reduce_logs_option in ${REDUCE_LOGS//,/ }; do |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid pathname expansion while parsing REDUCE_LOGS.
The unquoted expansion on Line 47 performs pathname expansion after word splitting. A value such as REDUCE_LOGS='*' can therefore be rewritten to filenames before validation, producing misleading errors or accidentally matching an allowed option. Tokenize into an array and iterate over it quoted.
Suggested fix
- # Normalize commas to spaces, then validate each option.
- # Intentional word-splitting of comma/space separated options.
- # shellcheck disable=SC2086
- for reduce_logs_option in ${REDUCE_LOGS//,/ }; do
+ # Normalize commas to spaces without pathname expansion.
+ local -a reduce_logs_options
+ read -r -a reduce_logs_options <<< "${REDUCE_LOGS//,/ }"
+ for reduce_logs_option in "${reduce_logs_options[@]}"; do📝 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.
| # Normalize commas to spaces, then validate each option. | |
| # Intentional word-splitting of comma/space separated options. | |
| # shellcheck disable=SC2086 | |
| for reduce_logs_option in ${REDUCE_LOGS//,/ }; do | |
| # Normalize commas to spaces without pathname expansion. | |
| local -a reduce_logs_options | |
| read -r -a reduce_logs_options <<< "${REDUCE_LOGS//,/ }" | |
| for reduce_logs_option in "${reduce_logs_options[@]}"; do |
🤖 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/common.sh` around lines 44 - 47, Update the REDUCE_LOGS
parsing loop to tokenize the comma/space-separated value into an array without
pathname expansion, then iterate over the resulting elements with quoted
expansion. Preserve the existing normalization and option validation behavior
while removing the unquoted ${REDUCE_LOGS//,/ } iteration in the
reduce_logs_option loop.
| wait \"\${PIDS[@]}\" 2>/dev/null || true | ||
| PIDS=() | ||
| [[ -f \"$TEST_TMPDIR/service_logs/masters/testservice_service.log\" ]] && echo 'uncompressed log present' | ||
| [[ ! -f \"$TEST_TMPDIR/service_logs/masters/testservice_service.log.gz\" ]] && echo 'gzip absent' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make compression tests validate the collection result.
Each test suppresses wait failures with || true, and compressed cases only check that .log.gz exists. A failed or invalid gzip operation can still leave a file behind. Let wait propagate failures and add gzip -t or gzip -dc assertions for compressed artifacts.
Also applies to: 176-179, 193-195
🤖 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 `@tests/service_logs_util.bats` around lines 159 - 162, Update the compression
test cases around the PIDS wait blocks to remove “|| true” so background
failures propagate, and strengthen each compressed-artifact assertion beyond
file existence by validating the gzip contents with gzip -t or gzip -dc. Apply
the same changes to the cases at the referenced later blocks while preserving
the existing uncompressed and absent-gzip checks.
| # REDUCE_LOGS: unset = defaults. Comma or space separated list of: | ||
| # skip_rotated_logs - omit --rotated-pod-logs from oc adm inspect | ||
| # compress_service_logs - gzip host service logs (see gather_service_logs_util) | ||
| # shellcheck disable=SC2034 |
There was a problem hiding this comment.
Running make lint gives the warning.
In collection-scripts/common.sh line 48:
rotated_pod_logs_arg=""
^------------------^ SC2034 (warning): rotated_pod_logs_arg appears unused. Verify use (or export if used externally).
Hence added the shellcheck disable=SC2034 line to avoid this warning.
There was a problem hiding this comment.
It wasn't failing. Why now it started failing?
| # compress_service_logs - gzip host service logs (see gather_service_logs_util) | ||
| # shellcheck disable=SC2034 | ||
| rotated_pod_logs_arg="--rotated-pod-logs" | ||
| # shellcheck disable=SC2034 |
There was a problem hiding this comment.
Running make lint gives the warning.
In collection-scripts/common.sh line 51:
compress_service_logs=true
^-------------------^ SC2034 (warning): compress_service_logs appears unused. Verify use (or export if used externally).
Hence added the shellcheck disable=SC2034 line to avoid this warning.
| rotated_pod_logs_arg="" | ||
| ;; | ||
| compress_service_logs) | ||
| compress_service_logs=1 |
There was a problem hiding this comment.
Would it be better to use true instead of 1?
| compress_service_logs=1 | |
| compress_service_logs=true |
| mkdir -p "${DIR_PATH}" | ||
| # Default: uncompressed .log for compatibility with existing tooling. | ||
| # Opt in via REDUCE_LOGS=compress_service_logs (parsed by get_log_collection_args). | ||
| if [[ -n "${compress_service_logs:-}" ]]; then |
There was a problem hiding this comment.
It would be better to check this true or not
| for service in "${@}"; do | ||
| echo "INFO: Collecting host service logs for $service" | ||
| if [[ ${selector} =~ '--role=' ]]; then | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -l kubernetes.io/os=linux -u "${service}" >"${DIR_PATH}/${service}_service.log" & | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -l kubernetes.io/os=linux -u "${service}" | ${LOG_PIPE} >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" & | ||
| PIDS+=($!) | ||
| else | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -u "${service}" >"${DIR_PATH}/${service}_service.log" & | ||
| oc adm node-logs "${node_log_collection_args:---since=${SINCE_TIMEFRAME:--7d}}" "${selector}" -u "${service}" | ${LOG_PIPE} >"${DIR_PATH}/${service}_service${LOG_SUFFIX}" & |
| fi | ||
|
|
||
| PIDS=() | ||
| WINDOWS_NODE_LOGS_START=$(date +%s) |
There was a problem hiding this comment.
This change should be separate and is not related to the motivation of this PR
There was a problem hiding this comment.
Ok. I thought it would be better to know the actual time it takes to get the windows node logs for future analysis. We can remove it if you feel it is unnecessary.
| LOG_FILE_DIR=${WINDOWS_NODE_LOGS}/log_files/$(dirname "$log") | ||
| mkdir -p "${LOG_FILE_DIR}" | ||
| /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" >"${LOG_FILE_DIR}/$(basename "$log")" & | ||
| /usr/bin/oc adm node-logs ${node_log_collection_args:+"$node_log_collection_args"} -l kubernetes.io/os=windows --path="$log" | gzip >"${LOG_FILE_DIR}/$(basename "$log").gz" & |
There was a problem hiding this comment.
Why do we directly use gzip here?
There was a problem hiding this comment.
As the CI requires .log files for host_service_logs, added the flag for service logs only. Do you think this also needs to be flagged?
We already have this gzip functionality in gather_kas_startup_termination_logs. Hence, add the gzip here to reduce the log size of windows node logs.
|
|
||
| @test "get_log_collection_args accepts both REDUCE_LOGS tokens" { | ||
| run bash -c " | ||
| export REDUCE_LOGS='skip_rotated_logs,compress_service_logs' |
There was a problem hiding this comment.
I assume that REDUCE_LOGS='compress_service_logs' can also be used (without skip_rotated_logs)?
There was a problem hiding this comment.
You are right.
Examples:
Skip rotated pod logs
oc adm must-gather -- REDUCE_LOGS=skip_rotated_logs /usr/bin/gather
Compress host service logs
oc adm must-gather -- REDUCE_LOGS=compress_service_logs /usr/bin/gather
Both
oc adm must-gather -- REDUCE_LOGS=skip_rotated_logs,compress_service_logs /usr/bin/gather
|
|
||
| assert_failure | ||
| assert_output --partial "ERROR" | ||
| assert_output --partial "skip_rotated_logs,other" |
There was a problem hiding this comment.
What is the reason of this change?
There was a problem hiding this comment.
That assertion was narrowed when comma-separated REDUCE_LOGS became valid.
Before: REDUCE_LOGS accepted only a single value. The test was essentially “comma-separated fails,” and it checked for the whole string:
assert_output --partial "skip_rotated_logs,other"
After: lists are allowed (skip_rotated_logs,compress_service_logs).
The same input still fails, but only because of the unknown token 'other'. The test was renamed to “contains an unknown token,” and the assertion became:
assert_output --partial "other"
So it checks that the bad token is reported, not that commas are rejected. The error still includes the full got: [...] value; the test just no longer keys off that older “no commas” meaning.
|
|
||
| assert_failure | ||
| assert_output --partial "ERROR" | ||
| assert_output --partial "skip_rotated_logs" |
There was a problem hiding this comment.
What is the reason of this change?
|
@praveencodes: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
Summary by CodeRabbit
.log.gzfiles when enabled and uncompressed.logfiles by default..gzfiles.