Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions collection-scripts/common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,34 @@ get_log_collection_args() {
fi


# REDUCE_LOGS: unset = default (--rotated-pod-logs). If set, only skip_rotated_logs is allowed.
rotated_pod_logs_arg="--rotated-pod-logs"
# 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of this?

@praveencodes praveencodes Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't failing. Why now it started failing?

rotated_pod_logs_arg="--rotated-pod-logs"
# shellcheck disable=SC2034

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

compress_service_logs=""

if [ -n "${REDUCE_LOGS:-}" ]; then
case "${REDUCE_LOGS}" in
skip_rotated_logs)
rotated_pod_logs_arg=""
;;
*)
echo "ERROR: REDUCE_LOGS must be unset or skip_rotated_logs (got: [${REDUCE_LOGS}])." >&2
exit 1
;;
esac
# 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
Comment on lines +44 to +47

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
# 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.

case "${reduce_logs_option}" in
skip_rotated_logs)
rotated_pod_logs_arg=""
;;
compress_service_logs)
compress_service_logs=true
;;
"")
;;
*)
echo "ERROR: REDUCE_LOGS unknown value '${reduce_logs_option}'. Allowed: skip_rotated_logs, compress_service_logs (got: [${REDUCE_LOGS}])." >&2
exit 1
;;
esac
done
fi

# oc adm node-logs `--since` parameter is not the same as oc adm inspect `--since`.
Expand Down
12 changes: 10 additions & 2 deletions collection-scripts/gather_service_logs_util
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,21 @@ function collect_service_logs {
fi

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).
LOG_SUFFIX=".log"
LOG_PIPE="cat"
if [[ "${compress_service_logs:-}" == "true" ]]; then
LOG_SUFFIX=".log.gz"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this be problematic for customers with existing scripts who rely on these assumptions? should we gate it behind a flag?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it behind the flag REDUCE_LOGS. It not accepts both skip_rotated_logs and compress_service_logs.

LOG_PIPE="gzip"
fi
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}" &
Comment on lines 33 to +39

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make sense?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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"
done

Length 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:

  1. The script has no errexit and does not inspect wait’s status, so it would still continue after failure.
  2. 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.

PIDS+=($!)
fi
done
Expand Down
2 changes: 1 addition & 1 deletion collection-scripts/gather_windows_node_logs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ echo INFO: Collecting logs for all Windows nodes
for log in "${LOGS[@]}"; do
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" &

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we directly use gzip here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PIDS+=($!)
done
wait "${PIDS[@]}"
Expand Down
50 changes: 48 additions & 2 deletions tests/common.bats
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,57 @@ load test_helper
source \"$SCRIPT_DIR/common.sh\"
get_log_collection_args
echo \"rotated_pod_logs_arg=[\$rotated_pod_logs_arg]\"
echo \"compress_service_logs=[\$compress_service_logs]\"
"

assert_success
assert_output --partial "rotated_pod_logs_arg=[]"
assert_output --partial "compress_service_logs=[]"
}

@test "get_log_collection_args fails when REDUCE_LOGS is comma-separated" {
@test "get_log_collection_args enables compress_service_logs when REDUCE_LOGS is compress_service_logs" {
run bash -c "
export REDUCE_LOGS='compress_service_logs'
source \"$SCRIPT_DIR/common.sh\"
get_log_collection_args
echo \"rotated_pod_logs_arg=\$rotated_pod_logs_arg\"
echo \"compress_service_logs=\$compress_service_logs\"
"

assert_success
assert_output --partial "rotated_pod_logs_arg=--rotated-pod-logs"
assert_output --partial "compress_service_logs=true"
}

@test "get_log_collection_args accepts both REDUCE_LOGS tokens" {
run bash -c "
export REDUCE_LOGS='skip_rotated_logs,compress_service_logs'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that REDUCE_LOGS='compress_service_logs' can also be used (without skip_rotated_logs)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

source \"$SCRIPT_DIR/common.sh\"
get_log_collection_args
echo \"rotated_pod_logs_arg=[\$rotated_pod_logs_arg]\"
echo \"compress_service_logs=\$compress_service_logs\"
"

assert_success
assert_output --partial "rotated_pod_logs_arg=[]"
assert_output --partial "compress_service_logs=true"
}

@test "get_log_collection_args accepts both REDUCE_LOGS tokens in either order" {
run bash -c "
export REDUCE_LOGS='compress_service_logs,skip_rotated_logs'
source \"$SCRIPT_DIR/common.sh\"
get_log_collection_args
echo \"rotated_pod_logs_arg=[\$rotated_pod_logs_arg]\"
echo \"compress_service_logs=\$compress_service_logs\"
"

assert_success
assert_output --partial "rotated_pod_logs_arg=[]"
assert_output --partial "compress_service_logs=true"
}

@test "get_log_collection_args fails when REDUCE_LOGS contains an unknown token" {
run bash -c "
export REDUCE_LOGS='skip_rotated_logs,other'
source \"$SCRIPT_DIR/common.sh\"
Expand All @@ -130,7 +174,7 @@ load test_helper

assert_failure
assert_output --partial "ERROR"
assert_output --partial "skip_rotated_logs,other"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_output --partial "other"
}

@test "get_log_collection_args fails when REDUCE_LOGS is unknown" {
Expand All @@ -142,7 +186,9 @@ load test_helper

assert_failure
assert_output --partial "ERROR"
assert_output --partial "invalid"
assert_output --partial "skip_rotated_logs"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of this change?

assert_output --partial "compress_service_logs"
}

@test "get_log_collection_args formats node_log_collection_args from MUST_GATHER_SINCE" {
Expand Down
49 changes: 49 additions & 0 deletions tests/service_logs_util.bats
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,52 @@ run_service_logs_test() {
assert_success
assert_output --partial "INFO: Collecting host service logs for testservice"
}

@test "collect_service_logs writes uncompressed .log by default" {
run_service_logs_test "
unset REDUCE_LOGS
source \"$SCRIPT_DIR/common.sh\"
get_log_collection_args
collect_service_logs --role=master testservice 2>&1
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'
Comment on lines +159 to +162

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

"

assert_success
assert_output --partial "uncompressed log present"
assert_output --partial "gzip absent"
}

@test "collect_service_logs writes .log.gz when REDUCE_LOGS is compress_service_logs" {
run_service_logs_test "
export REDUCE_LOGS='compress_service_logs'
source \"$SCRIPT_DIR/common.sh\"
get_log_collection_args
collect_service_logs --role=master testservice 2>&1
wait \"\${PIDS[@]}\" 2>/dev/null || true
PIDS=()
[[ -f \"$TEST_TMPDIR/service_logs/masters/testservice_service.log.gz\" ]] && echo 'gzip log present'
[[ ! -f \"$TEST_TMPDIR/service_logs/masters/testservice_service.log\" ]] && echo 'uncompressed absent'
"

assert_success
assert_output --partial "gzip log present"
assert_output --partial "uncompressed absent"
}

@test "collect_service_logs writes .log.gz when REDUCE_LOGS includes compress_service_logs with skip_rotated_logs" {
run_service_logs_test "
export REDUCE_LOGS='skip_rotated_logs,compress_service_logs'
source \"$SCRIPT_DIR/common.sh\"
get_log_collection_args
collect_service_logs --role=master testservice 2>&1
wait \"\${PIDS[@]}\" 2>/dev/null || true
PIDS=()
[[ -f \"$TEST_TMPDIR/service_logs/masters/testservice_service.log.gz\" ]] && echo 'gzip log present'
"

assert_success
assert_output --partial "gzip log present"
}