Skip to content

feat: re-add daemonize to start.sh and loop waiting for ./readyz#391

Merged
canercidam merged 1 commit intomainfrom
fryd/buildernet-start-script
Mar 3, 2026
Merged

feat: re-add daemonize to start.sh and loop waiting for ./readyz#391
canercidam merged 1 commit intomainfrom
fryd/buildernet-start-script

Conversation

@fkondej
Copy link
Contributor

@fkondej fkondej commented Mar 2, 2026

No description provided.

@fkondej fkondej self-assigned this Mar 2, 2026
@fkondej fkondej requested a review from canercidam as a code owner March 2, 2026 16:26
@claude
Copy link
Contributor

claude bot commented Mar 2, 2026

Claude finished @fkondej's task in 1m 36s —— View job


Review of PR #391

  • Gather context and read changed files
  • Analyze code changes for issues
  • Post review feedback

Summary

This PR changes start.sh from using exec (foreground QEMU replacing the shell) to -daemonize (QEMU forks into background), and adds a readiness polling loop that waits for the HAProxy /readyz endpoint to return 200.

The overall approach is sound. Two minor issues flagged inline:

  1. Sleep-before-check pattern (lines 100-116) — the loop sleeps 5s before the first health check, adding unnecessary delay if the VM boots quickly. Suggest checking first, then sleeping.
  2. Missing cleanup hint on timeout (lines 118-120) — when readyz times out, the QEMU VM is still running but the error message doesn't mention stop.sh. Users may be left with an orphaned VM.

No bugs, security concerns, or correctness issues found.

Comment on lines +100 to +116
while (( SECONDS < DEADLINE )); do
sleep 5

# Fail fast if QEMU died (bad image, kernel panic, etc.)
if ! kill -0 "$(cat "${PIDFILE}")" 2>/dev/null; then
echo "Error: QEMU process died during boot"
echo " Check console log: tail -f ${CONSOLE_LOG}"
exit 1
fi

HTTP_CODE=$(curl -s -k -o /dev/null -w "%{http_code}" "${READYZ_URL}" 2>/dev/null || echo "000")
if [[ "${HTTP_CODE}" == "200" ]]; then
echo "VM ready after $(( READYZ_TIMEOUT - (DEADLINE - SECONDS) ))s (PID: $(cat "${PIDFILE}"))"
exit 0
fi
echo " waiting... $(( READYZ_TIMEOUT - (DEADLINE - SECONDS) ))s (HTTP ${HTTP_CODE})"
done
Copy link
Contributor

Choose a reason for hiding this comment

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

The loop sleeps 5s before the first health check. If the VM is already ready (or becomes ready quickly), this adds an unnecessary delay. Consider checking first, then sleeping:

Suggested change
while (( SECONDS < DEADLINE )); do
sleep 5
# Fail fast if QEMU died (bad image, kernel panic, etc.)
if ! kill -0 "$(cat "${PIDFILE}")" 2>/dev/null; then
echo "Error: QEMU process died during boot"
echo " Check console log: tail -f ${CONSOLE_LOG}"
exit 1
fi
HTTP_CODE=$(curl -s -k -o /dev/null -w "%{http_code}" "${READYZ_URL}" 2>/dev/null || echo "000")
if [[ "${HTTP_CODE}" == "200" ]]; then
echo "VM ready after $(( READYZ_TIMEOUT - (DEADLINE - SECONDS) ))s (PID: $(cat "${PIDFILE}"))"
exit 0
fi
echo " waiting... $(( READYZ_TIMEOUT - (DEADLINE - SECONDS) ))s (HTTP ${HTTP_CODE})"
done
while (( SECONDS < DEADLINE )); do
# Fail fast if QEMU died (bad image, kernel panic, etc.)
if ! kill -0 "$(cat "${PIDFILE}")" 2>/dev/null; then
echo "Error: QEMU process died during boot"
echo " Check console log: tail -f ${CONSOLE_LOG}"
exit 1
fi
HTTP_CODE=$(curl -s -k -o /dev/null -w "%{http_code}" "${READYZ_URL}" 2>/dev/null || echo "000")
if [[ "${HTTP_CODE}" == "200" ]]; then
echo "VM ready after $(( READYZ_TIMEOUT - (DEADLINE - SECONDS) ))s (PID: $(cat "${PIDFILE}"))"
exit 0
fi
echo " waiting... $(( READYZ_TIMEOUT - (DEADLINE - SECONDS) ))s (HTTP ${HTTP_CODE})"
sleep 5

Comment on lines +118 to +120
echo "Error: VM did not become ready within ${READYZ_TIMEOUT}s"
echo " Check console log: tail -f ${CONSOLE_LOG}"
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

On timeout, the QEMU VM is still running in the background but the user isn't told. Consider adding a hint to run stop.sh:

Suggested change
echo "Error: VM did not become ready within ${READYZ_TIMEOUT}s"
echo " Check console log: tail -f ${CONSOLE_LOG}"
exit 1
echo "Error: VM did not become ready within ${READYZ_TIMEOUT}s"
echo " Check console log: tail -f ${CONSOLE_LOG}"
echo " Stop the VM: ./scripts/stop.sh"
exit 1

@canercidam canercidam merged commit 9d1baaf into main Mar 3, 2026
13 checks passed
@canercidam canercidam deleted the fryd/buildernet-start-script branch March 3, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants