Skip to content

MG-233: compress service and window node logs #554

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
praveencodes wants to merge 4 commits into
base: main
Choose a base branch
from
Open

MG-233: compress service and window node logs #554

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd488df
MG-233: compress service and window node logs
praveencodes Jun 23, 2026
6ac1d3e
remove the CI condition for window node logs
praveencodes Jul 28, 2026
e8ce490
MG-233: make service log compression opt-in via REDUCE_LOGS
praveencodes Jul 29, 2026
49394e6
address the review comments
praveencodes Jul 31, 2026
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
12 changes: 10 additions & 2 deletions collection-scripts/gather_service_logs_util
View file
Open in desktop
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}"
# In CI (OPENSHIFT_CI/ARTIFACTS_DIR) skip gzip for easier inspection of artifacts
if [[ -n "${OPENSHIFT_CI:-}" || -n "${ARTIFACTS_DIR:-}" ]]; then
LOG_SUFFIX=".log"

@ardaguclu ardaguclu Jul 30, 2026

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.

Instead of assigning in else statement, it is better to default to these values and only modify the compress_service_logs is true

LOG_PIPE="cat"
else
LOG_SUFFIX=".log.gz"

@Prashanth684 Prashanth684 Jul 21, 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.

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

@praveencodes praveencodes Jul 29, 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.

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 •
edited
Loading

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.

@ardaguclu ardaguclu Jul 30, 2026

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?

@coderabbitai coderabbitai Bot Jul 30, 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.

🧩 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
14 changes: 13 additions & 1 deletion collection-scripts/gather_windows_node_logs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,26 @@ if [ -z "$WIN_NODES" ]; then
fi

PIDS=()
# In CI (OPENSHIFT_CI/ARTIFACTS_DIR) skip gzip for easier inspection of artifacts
if [[ -n "${OPENSHIFT_CI:-}" || -n "${ARTIFACTS_DIR:-}" ]]; then
WIN_LOG_SUFFIX=""
WIN_LOG_PIPE="cat"
else
WIN_LOG_SUFFIX=".gz"
WIN_LOG_PIPE="gzip"
fi

WINDOWS_NODE_LOGS_START=$(date +%s)

@ardaguclu ardaguclu Jul 30, 2026

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.

This change should be separate and is not related to the motivation of this PR

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

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.

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" | ${WIN_LOG_PIPE} >"${LOG_FILE_DIR}/$(basename "$log")${WIN_LOG_SUFFIX}" &

@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

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.

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

PIDS+=($!)
done
wait "${PIDS[@]}"
WINDOWS_NODE_LOGS_END=$(date +%s)
echo "INFO: Windows node log collection completed in $((WINDOWS_NODE_LOGS_END - WINDOWS_NODE_LOGS_START)) seconds"

# force disk flush to ensure that all data gathered is accessible in the copy container
sync