Skip to content

chore: fix the collect-logs.sh #355

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
zhiying-lin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: fix the collect-logs.sh #355

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
133 changes: 124 additions & 9 deletions test/e2e/collect-logs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,125 @@ echo -e "${GREEN}Starting log collection at ${TIMESTAMP}${NC}"
echo "Logs will be saved to: ${LOG_DIR}"
echo ""

# Function to collect logs from a pod
collect_pod_logs() {
# Function to get pod UID for log file lookup
get_pod_uid() {
local pod_name=$1
kubectl get pod "${pod_name}" -n "${NAMESPACE}" -o jsonpath='{.metadata.uid}' 2>/dev/null || echo ""
}

# Function to get Kind node name for cluster
get_kind_node_name() {
local cluster_name=$1
echo "${cluster_name}-control-plane"
}

# Function to collect logs directly from Kind node filesystem (much faster and complete)
collect_pod_logs_direct() {
local pod_name=$1
local cluster_name=$2
local log_file_prefix=$3

echo -e "${YELLOW}Collecting logs from pod ${pod_name} in cluster ${cluster_name}${NC}"


echo -e "${YELLOW}Collecting logs from pod ${pod_name} in cluster ${cluster_name} (direct access)${NC}"

# Get pod UID for log directory lookup
local pod_uid
pod_uid=$(get_pod_uid "${pod_name}")
if [ -z "$pod_uid" ]; then
echo -e "${RED}Could not get UID for pod ${pod_name}, falling back to kubectl logs${NC}"
collect_pod_logs_kubectl "$@"
return
fi

# Get all containers in the pod
local containers
containers=$(kubectl get pod "${pod_name}" -n "${NAMESPACE}" -o jsonpath='{.spec.containers[*].name}' 2>/dev/null || echo "")

if [ -z "$containers" ]; then
echo -e "${RED}No containers found in pod ${pod_name}${NC}"
return
fi


# Get Kind node name
local node_name
node_name=$(get_kind_node_name "${cluster_name}")

# Construct log directory path inside the Kind node
local log_dir="/var/log/pods/${NAMESPACE}_${pod_name}_${pod_uid}"

# Collect logs for each container
for container in $containers; do
echo " - Container ${container}:"

# Get all log files for this container from the Kind node
local container_log_dir="${log_dir}/${container}"
local log_files
log_files=$(docker exec "${node_name}" find "${container_log_dir}" -name "*.log" 2>/dev/null | sort -V || echo "")

if [ -z "$log_files" ]; then
echo -e " ${RED}No direct log files found, falling back to kubectl logs${NC}"
# Fallback to kubectl approach for this container
local log_file="${log_file_prefix}-${container}.log"
if kubectl logs "${pod_name}" -n "${NAMESPACE}" -c "${container}" > "${log_file}" 2>&1; then
echo " -> ${log_file} (via kubectl)"
else
echo " -> Failed to get logs via kubectl" > "${log_file}"
fi
continue
fi

# Copy individual log files for this container
local file_count=0
for log_file_path in $log_files; do
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The loop iterates over $log_files without quotes. If log file paths contain spaces, this will break the iteration. While unlikely in Kubernetes log paths, it's a potential issue. The variable should be properly quoted or handled as an array:

while IFS= read -r log_file_path; do
    file_count=$((file_count + 1))
    # ... rest of the loop
done <<< "$log_files"

This approach safely handles paths with spaces and special characters.

Copilot uses AI. Check for mistakes.
file_count=$((file_count + 1))
local base_name
base_name=$(basename "${log_file_path}")
local individual_log_file="${log_file_prefix}-${container}-${base_name}"

{
echo "# Log file metadata"
echo "# Timestamp: $(date -u '+%Y-%m-%d %H:%M:%S UTC')"
echo "# Source: ${log_file_path}"
echo "# Pod: ${pod_name}"
echo "# Container: ${container}"
echo "# Cluster: ${cluster_name}"
echo "# Namespace: ${NAMESPACE}"
echo "# Method: Direct file access from Kind node"
echo "# Node: ${node_name}"
echo "# Part: ${file_count} of $(echo "$log_files" | wc -l)"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The command echo "$log_files" | wc -l is executed on every iteration of the loop, which is inefficient. This should be calculated once before the loop starts:

# Before the loop (around line 103)
local total_files
total_files=$(echo "$log_files" | wc -l)

# Then in the loop (line 120)
echo "# Part: ${file_count} of ${total_files}"

Copilot uses AI. Check for mistakes.
echo "# =================================="
echo ""
docker exec "${node_name}" cat "${log_file_path}" 2>/dev/null || echo "Failed to read ${log_file_path}"
} > "${individual_log_file}"

echo " -> ${individual_log_file}"
done
done
}

# Function to collect logs using kubectl (fallback method)
collect_pod_logs_kubectl() {
local pod_name=$1
local cluster_name=$2
local log_file_prefix=$3

echo -e "${YELLOW}Collecting logs from pod ${pod_name} in cluster ${cluster_name} (kubectl fallback)${NC}"

# Get all containers in the pod
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The containers variable is not declared as local, which could lead to it being a global variable. This should be declared as local for consistency and to avoid potential conflicts, especially since it's used in multiple functions.

local containers
containers=$(kubectl get pod "${pod_name}" -n "${NAMESPACE}" -o jsonpath='{.spec.containers[*].name}' 2>/dev/null || echo "")
Suggested change
# Get all containers in the pod
# Get all containers in the pod
local containers

Copilot uses AI. Check for mistakes.
containers=$(kubectl get pod "${pod_name}" -n "${NAMESPACE}" -o jsonpath='{.spec.containers[*].name}' 2>/dev/null || echo "")

if [ -z "$containers" ]; then
echo -e "${RED}No containers found in pod ${pod_name}${NC}"
return
fi

# Collect logs for each container
for container in $containers; do
log_file="${log_file_prefix}-${container}.log"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The log_file variable is not declared as local, which could lead to it being a global variable and potentially cause naming conflicts with other parts of the script.

local log_file="${log_file_prefix}-${container}.log"
Suggested change
log_file="${log_file_prefix}-${container}.log"
local log_file="${log_file_prefix}-${container}.log"

Copilot uses AI. Check for mistakes.
echo " - Container ${container} -> ${log_file}"

# Get current logs
kubectl logs "${pod_name}" -n "${NAMESPACE}" -c "${container}" > "${log_file}" 2>&1 || \
echo "Failed to get logs for container ${container}" > "${log_file}"

# Try to get previous logs if pod was restarted
previous_log_file="${log_file_prefix}-${container}-previous.log"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The previous_log_file variable is not declared as local, which could lead to it being a global variable and potentially cause naming conflicts.

local previous_log_file="${log_file_prefix}-${container}-previous.log"
Suggested change
previous_log_file="${log_file_prefix}-${container}-previous.log"
local previous_log_file="${log_file_prefix}-${container}-previous.log"

Copilot uses AI. Check for mistakes.
if kubectl logs "${pod_name}" -n "${NAMESPACE}" -c "${container}" --previous > "${previous_log_file}" 2>&1; then
Expand All @@ -69,6 +163,27 @@ collect_pod_logs() {
done
}

# Function to collect logs from a pod (tries direct access first, falls back to kubectl)
collect_pod_logs() {
local pod_name=$1
local cluster_name=$2
local log_file_prefix=$3

# Get pod info for debugging
local pod_info_file="${log_file_prefix}-pod-info.txt"
echo " - Pod info -> ${pod_info_file}"
{
echo "=== Pod Description ==="
kubectl describe pod "${pod_name}" -n "${NAMESPACE}"
echo ""
echo "=== Pod YAML ==="
kubectl get pod "${pod_name}" -n "${NAMESPACE}" -o yaml
} > "${pod_info_file}" 2>&1

# Try direct access first (much better), fallback to kubectl if needed
collect_pod_logs_direct "$@"
}

# Collect hub cluster logs
echo -e "${GREEN}=== Collecting Hub Cluster Logs ===${NC}"
kind export kubeconfig --name "${HUB_CLUSTER}" 2>/dev/null || {
Expand Down
Loading