-
Notifications
You must be signed in to change notification settings - Fork 20
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.
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the log collection script for e2e tests by implementing direct filesystem access to container logs from Kind nodes instead of relying solely on kubectl logs, which only retrieves limited rotated log history. This ensures complete hub agent logs are captured.
Key Changes
- Implements direct log file access from Kind node filesystem via
docker execfor complete log history - Adds fallback mechanism to
kubectl logswhen direct access fails - Collects pod metadata (description and YAML) for debugging purposes
|
|
||
| echo -e "${YELLOW}Collecting logs from pod ${pod_name} in cluster ${cluster_name} (kubectl fallback)${NC}" | ||
|
|
||
| # Get all containers in the pod |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
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 "")| # Get all containers in the pod | |
| # Get all containers in the pod | |
| local containers |
|
|
||
| # Collect logs for each container | ||
| for container in $containers; do | ||
| log_file="${log_file_prefix}-${container}.log" |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
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"| log_file="${log_file_prefix}-${container}.log" | |
| local log_file="${log_file_prefix}-${container}.log" |
|
|
||
| # Try to get previous logs if pod was restarted | ||
| previous_log_file="${log_file_prefix}-${container}-previous.log" |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
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"| previous_log_file="${log_file_prefix}-${container}-previous.log" | |
| local previous_log_file="${log_file_prefix}-${container}-previous.log" |
|
|
||
| # Copy individual log files for this container | ||
| local file_count=0 | ||
| for log_file_path in $log_files; do |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
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.
| 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)" |
Copilot
AI
Dec 3, 2025
There was a problem hiding this comment.
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}"Signed-off-by: Zhiying Lin <[email protected]>
3dfae74 to
cf060c6
Compare
Description of your changes
The kubectl logs command retrieves container logs from files stored locally on the Kubernetes node by the kubelet agent, but it only accesses a limited, rotated portion of the log history by default.
hub agent logs are missing using kubectl logs command.
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer