Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
22 changes: 21 additions & 1 deletion .github/workflows/router-e2e-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,15 @@ jobs:
--timeout 20
timeout-minutes: 20

- name: Cleanup k8s discovery resources
if: always()
run: |
if helm status vllm >/dev/null 2>&1; then
helm uninstall vllm --wait
else
echo "No vllm Helm release found; skipping cleanup."
fi

- name: Archive k8s discovery routing test results
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
if: always()
Expand Down Expand Up @@ -180,13 +189,24 @@ jobs:
echo "🚀 Starting vLLM serve backend"
mkdir -p "$LOG_DIR"
CUDA_VISIBLE_DEVICES=0 vllm serve facebook/opt-125m --port 8001 --gpu-memory-utilization 0.7 --chat-template tests/assets/template-chatml.jinja > "$LOG_DIR/backend1.log" 2>&1 &
backend_1_pid=$!
echo "STATIC_BACKEND_1_PID=$backend_1_pid" >> "$GITHUB_ENV"
CUDA_VISIBLE_DEVICES=1 vllm serve facebook/opt-125m --port 8002 --gpu-memory-utilization 0.7 --chat-template tests/assets/template-chatml.jinja > "$LOG_DIR/backend2.log" 2>&1 &
backend_2_pid=$!
echo "STATIC_BACKEND_2_PID=$backend_2_pid" >> "$GITHUB_ENV"

- name: Wait for backends to be ready
run: |
echo "⏳ Waiting for backends to be ready"
chmod +x tests/e2e/wait-for-backends.sh
./tests/e2e/wait-for-backends.sh 180 "http://localhost:8001" "http://localhost:8002"
./tests/e2e/wait-for-backends.sh \
180 \
"http://localhost:8001" \
"http://localhost:8002" \
"$STATIC_BACKEND_1_PID" \
"$STATIC_BACKEND_2_PID" \
"$LOG_DIR/backend1.log" \
"$LOG_DIR/backend2.log"

- name: Run All Static Discovery Routing Tests
run: |
Expand Down
32 changes: 32 additions & 0 deletions tests/e2e/wait-for-backends.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,46 @@ echo "⏳ Waiting for backends to be ready"
timeout=${1:-120}
backend1=${2:-"http://localhost:8001"}
backend2=${3:-"http://localhost:8002"}
backend1_pid=${4:-}
backend2_pid=${5:-}
backend1_log=${6:-}
backend2_log=${7:-}

print_backend_log() {
local backend_name=$1
local log_file=$2

if [ -n "$log_file" ] && [ -f "$log_file" ]; then
echo "Last 50 lines from ${backend_name} log (${log_file}):"
tail -n 50 "$log_file"
fi
}
Comment on lines +14 to +23

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since print_backend_log is a diagnostic helper, any failure within it (e.g., if tail fails due to permission issues or if the file is removed) should not cause the entire script to exit under set -e.

To prevent this, we should ensure the function always returns 0 by appending || true to the tail command and adding an explicit return 0 at the end of the function.

Suggested change
print_backend_log() {
local backend_name=$1
local log_file=$2
if [ -n "$log_file" ] && [ -f "$log_file" ]; then
echo "Last 50 lines from ${backend_name} log (${log_file}):"
tail -n 50 "$log_file"
fi
}
print_backend_log() {
local backend_name=$1
local log_file=$2
if [ -n "$log_file" ] && [ -f "$log_file" ]; then
echo "Last 50 lines from ${backend_name} log (${log_file}):"
tail -n 50 "$log_file" || true
fi
return 0
}


check_backend_process() {
local backend_name=$1
local backend_pid=$2
local log_file=$3

if [ -n "$backend_pid" ] && ! kill -0 "$backend_pid" 2>/dev/null; then
echo "❌ ${backend_name} process exited before becoming reachable"
print_backend_log "$backend_name" "$log_file"
return 1
fi
}
Comment on lines +25 to +36

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In Bash, if a function finishes without an explicit return, its exit status is that of the last executed command. If the if condition is false, the if statement returns 0, but relying on this implicit behavior can be fragile and less readable.

Adding an explicit return 0 at the end of check_backend_process makes the success path clear and robust.

Suggested change
check_backend_process() {
local backend_name=$1
local backend_pid=$2
local log_file=$3
if [ -n "$backend_pid" ] && ! kill -0 "$backend_pid" 2>/dev/null; then
echo "❌ ${backend_name} process exited before becoming reachable"
print_backend_log "$backend_name" "$log_file"
return 1
fi
}
check_backend_process() {
local backend_name=$1
local backend_pid=$2
local log_file=$3
if [ -n "$backend_pid" ] && ! kill -0 "$backend_pid" 2>/dev/null; then
echo "❌ ${backend_name} process exited before becoming reachable"
print_backend_log "$backend_name" "$log_file"
return 1
fi
return 0
}


start_time=$(date +%s)
echo "⏳ Waiting for backends to become reachable..."
while true; do
current_time=$(date +%s)
elapsed=$((current_time - start_time))

check_backend_process "Backend 1" "$backend1_pid" "$backend1_log" || exit 1
check_backend_process "Backend 2" "$backend2_pid" "$backend2_log" || exit 1

if [ $elapsed -ge "$timeout" ]; then
echo "❌ Backends failed to become reachable after ${timeout} seconds"
print_backend_log "Backend 1" "$backend1_log"
print_backend_log "Backend 2" "$backend2_log"
exit 1
fi

Expand Down
Loading