fix(glances): InfluxDB export broken in Glances v4 - #639
Conversation
Glances v4 requires --export on the CLI but cannot combine it with -w (webserver mode). Run a dedicated quiet/standalone instance for exports while the webserver remains the supervised foreground process. Fixes: hassio-addons#621
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe startup script now launches Glances exporter and webserver processes separately when exporters are configured, with coordinated signal handling and failure monitoring. The Dockerfile also updates the pinned nginx package revision. ChangesMulti-process Glances startup with exporter isolation
Nginx package pin update
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@frenck — this is a focused bug-fix PR split off from #628. It contains a single change to the Context: Glances v4 broke InfluxDB export for all users who have it enabled (issue #621, 9 👍). The The best-practices improvements from the original PR are tracked separately in #628. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
glances/rootfs/etc/services.d/glances/run (1)
61-73: ⚖️ Poor tradeoffConsider adding shutdown timeout and SIGKILL escalation.
If either process hangs and doesn't respond to SIGTERM, the shutdown function will wait indefinitely. Best practice is to wait with a timeout and escalate to SIGKILL if needed.
♻️ Example with timeout and SIGKILL escalation
shutdown() { bashio::log.info "Stopping Glances..." bashio::log.debug "Sending SIGTERM to web server (PID: ${WEBSERVER_PID})..." kill "${WEBSERVER_PID}" 2>/dev/null bashio::log.debug "Sending SIGTERM to exporter (PID: ${EXPORTER_PID})..." kill "${EXPORTER_PID}" 2>/dev/null # Wait up to 10 seconds for graceful shutdown local timeout=10 while (( timeout > 0 )); do if ! kill -0 "${WEBSERVER_PID}" 2>/dev/null && ! kill -0 "${EXPORTER_PID}" 2>/dev/null; then break fi sleep 1 (( timeout-- )) done # Force kill if still running kill -9 "${WEBSERVER_PID}" 2>/dev/null kill -9 "${EXPORTER_PID}" 2>/dev/null wait "${WEBSERVER_PID}" 2>/dev/null wait "${EXPORTER_PID}" 2>/dev/null bashio::log.info "Glances stopped" exit 0 }🤖 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 `@glances/rootfs/etc/services.d/glances/run` around lines 61 - 73, The shutdown() function currently sends SIGTERM to WEBSERVER_PID and EXPORTER_PID and then waits indefinitely; modify shutdown() to implement a bounded graceful shutdown: after sending SIGTERM to WEBSERVER_PID and EXPORTER_PID, poll (e.g., with kill -0) in a short loop up to a configurable timeout (seconds) to detect process exit, logging progress, and if either PID is still alive after the timeout send SIGKILL (kill -9) to that PID, then perform wait on both PIDs and log the final stopped messages; keep references to the existing WEBSERVER_PID and EXPORTER_PID variables and preserve existing logging (bashio::log.*).
🤖 Prompt for all review comments with 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.
Inline comments:
In `@glances/rootfs/etc/services.d/glances/run`:
- Around line 43-45: After backgrounding Glances with glances
"${exporter_options[@]}" & and storing EXPORTER_PID, verify the process is
actually running by waiting a short moment (e.g., sleep 1) then testing the pid
(use kill -0 or check /proc/$EXPORTER_PID); if the pid is not alive, call wait
$EXPORTER_PID to capture the exit status and log an error via bashio::log.error
including the exit code or reason, then exit non‑zero; if the pid is alive, keep
the existing bashio::log.info "Glances exporter started (PID: ${EXPORTER_PID})".
- Around line 57-59: The web server is backgrounded with glances and
WEBSERVER_PID is set but there's no verification it actually started; after
launching glances (glances "${options[@]}" & and WEBSERVER_PID=$!) add a quick
liveness check (e.g., short sleep then kill -0 $WEBSERVER_PID or check
/proc/$WEBSERVER_PID) and if the check fails, log an error via bashio::log.error
including the PID and exit or handle failure the same way the exporter does so
wait -n won't be triggered unexpectedly; keep the existing bashio::log.info
"Glances web server started (PID: ${WEBSERVER_PID})" only after the liveness
check succeeds.
---
Nitpick comments:
In `@glances/rootfs/etc/services.d/glances/run`:
- Around line 61-73: The shutdown() function currently sends SIGTERM to
WEBSERVER_PID and EXPORTER_PID and then waits indefinitely; modify shutdown() to
implement a bounded graceful shutdown: after sending SIGTERM to WEBSERVER_PID
and EXPORTER_PID, poll (e.g., with kill -0) in a short loop up to a configurable
timeout (seconds) to detect process exit, logging progress, and if either PID is
still alive after the timeout send SIGKILL (kill -9) to that PID, then perform
wait on both PIDs and log the final stopped messages; keep references to the
existing WEBSERVER_PID and EXPORTER_PID variables and preserve existing logging
(bashio::log.*).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06616c29-ac94-4d8a-b85f-c745b0aae9e3
📒 Files selected for processing (1)
glances/rootfs/etc/services.d/glances/run
If either child process ignores SIGTERM, the previous shutdown() would wait indefinitely. Poll with kill -0 up to STOP_TIMEOUT seconds; force SIGKILL on any PID still alive after the deadline.
|
Thanks for the review @coderabbitai! Shutdown — bounded graceful shutdown with SIGKILL fallback ✅ Fixed in 434c9b8 Added a 30-second polling loop ( Startup verification (both findings) — Skipped intentionally The |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat glances/rootfs/etc/services.d/glances/runLength of output: 131
Shutdown (bounded SIGKILL fallback): The 30-second Startup verification: Your reasoning holds — The implementation looks well-reasoned overall. Good luck getting the maintainer review! 🐇 ✨ |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2ae94b1 to
434c9b8
Compare
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
github-actions
Bot
added
the
stale
Ah yes, the stale bot. Nothing says "thank you for your contributions" like threatening to close the PR that fixes a bug still breaking everyone's startup on 0.22.0. The bug isn't stale, it's very much alive and looping. Maybe point the 7-day timer at #621 instead? 😄 |
github-actions
Bot
removed
the
stale
|
@frenck what would it take to get this reviewed and merged? |
3 participants
Bug
Glances v4 changed the export mechanism: exporters must now be enabled via the
--exportCLI flag. However,--exportis incompatible with-w(webserver mode) — Glances enforces this restriction since nicolargo/glances#614. Running Glances with both flags results in:This causes the addon to crash on startup whenever any exporter (InfluxDB) is configured. Reported in issue #621 (9 👍).
Fix
Run two separate Glances processes when an exporter is configured:
glances -w …(serves the UI, same as before)glances --quiet --export <name> …(no webserver, data export only)Both run as background processes. A
traponSIGTERM/SIGINTforwards the signal to both children for clean shutdown when the addon stops.wait -nmonitors both; if either exits unexpectedly, the other is killed and the container exits with code 1 (S6-Overlay will restart it).When no exporter is configured, the script falls back to the original
exec glancescode path — no change in behaviour.Testing
Fixes #621
Summary by CodeRabbit