Add HEALTHCHECK support #421
Conversation
WalkthroughThe pull request introduces several modifications to the Tailscale project. A health check command has been added to the Dockerfile, enabling monitoring of the container's health. Additionally, the script managing the Tailscale service's control flow has been updated to recognize an additional state, "Stopped." Furthermore, a new healthcheck script has been implemented to evaluate the operational state of the Tailscale service, logging health status and managing timestamps for service activity. Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lmagyar
force-pushed
the
pr-healthcheck
branch
from
d124525 to
5a3ea4d
Compare
|
This behaves as expected in live environments and no issues reported by real users. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
tailscale/rootfs/usr/bin/healthcheck (1)
25-25: Useprintfwith format strings to prevent unintended behaviorIn lines 25, 36, 49, and 57, you're using
printfwithout specifying a format string. This can lead to unexpected behavior if the variables contain format specifiers or special characters. It's safer to provide a format string.Apply this diff to fix the issue:
-printf "${STARTED_TIMESTAMP}" > /var/run/s6/container_environment/STARTED_TIMESTAMP +printf '%s\n' "${STARTED_TIMESTAMP}" > /var/run/s6/container_environment/STARTED_TIMESTAMP -printf "${LAST_ONLINE_TIMESTAMP}" > /var/run/s6/container_environment/LAST_ONLINE_TIMESTAMP +printf '%s\n' "${LAST_ONLINE_TIMESTAMP}" > /var/run/s6/container_environment/LAST_ONLINE_TIMESTAMP -printf "UNHEALTHY" > /var/run/s6/container_environment/LAST_REPORTED_HEALTH_STATE +printf '%s\n' "UNHEALTHY" > /var/run/s6/container_environment/LAST_REPORTED_HEALTH_STATE -printf "HEALTHY" > /var/run/s6/container_environment/LAST_REPORTED_HEALTH_STATE +printf '%s\n' "HEALTHY" > /var/run/s6/container_environment/LAST_REPORTED_HEALTH_STATEAlso applies to: 36-36, 49-49, 57-57
tailscale/translations/en.yaml (1)
47-55: Minor grammatical improvements in descriptionsConsider the following minor changes to enhance clarity:
- Line 47: Add "the" before "timeout": "This option allows you to set the timeout in seconds..."
- Line 54: Change "ie." to "i.e.": "The minimum value is 900, i.e., 15 minutes."
- Line 55: Rephrase for clarity: "You may set it to several minutes or even hours."
Apply this diff to implement the changes:
- This option allows you to set timeout in seconds for Tailscale to be offline + This option allows you to set the timeout in seconds for Tailscale to be offline - after (re)start. The minimum value is 900, ie. 15 minutes. Set it to several + after (re)start. The minimum value is 900, i.e., 15 minutes. You may set it to severaltailscale/DOCS.md (4)
73-74: Consider documenting the rationale for example timeout valuesThe example configuration shows specific timeout values (110s for offline and 3600s for restart) without explaining why these particular values were chosen. Consider adding a brief comment to help users understand these choices.
healthcheck_offline_timeout: 110 # Time to wait before restarting after going offline healthcheck_restart_timeout: 3600 # Time to wait for recovery after restart (1 hour)
194-215: Fix abbreviation formatting and enhance clarityThe documentation for
healthcheck_offline_timeoutis comprehensive but needs some grammatical improvements:
- Fix abbreviation formatting:
-Tailscale was once online, ie. it won't have any effect when +Tailscale was once online, i.e., it won't have any effect when -Tailscale's status is eg. Starting +Tailscale's status is e.g., Starting -Assistant in each 30s, ie. the effective +Assistant in each 30s, i.e., the effective
- Consider adding an example scenario to make it more concrete:
+For example, if set to 300 seconds (5 minutes), the add-on will restart if Tailscale goes offline and doesn't recover within 5 minutes of detecting the offline state.🧰 Tools
🪛 LanguageTool
[uncategorized] ~204-~204: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...s only after Tailscale was once online, ie. it won't have any effect when Tailscale...(I_E)
[uncategorized] ~205-~205: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...e any effect when Tailscale's status is eg. Starting, NeedsLogin or NeedsMachineAut...(E_G)
[uncategorized] ~213-~213: The abbreviation “i.e.” (= that is) requires two periods.
Context: ... checked by Home Assistant in each 30s, ie. the effective resolution of this option...(I_E)
216-241: Fix abbreviation formatting and clarify timeout behaviorThe documentation for
healthcheck_restart_timeoutneeds grammatical improvements:Fix abbreviation formatting:
-The minimum value is 900, ie. 15 minutes +The minimum value is 900, i.e., 15 minutes -Tailscale is starting, ie. it won't have +Tailscale is starting, i.e., it won't have -Assistant in each 30s, ie. the effective +Assistant in each 30s, i.e., the effective🧰 Tools
🪛 LanguageTool
[uncategorized] ~221-~221: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...r (re)start. The minimum value is 900, ie. 15 minutes, to always give you enough t...(I_E)
[uncategorized] ~229-~229: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...appens only when Tailscale is starting, ie. it won't have any effect after Tailscal...(I_E)
[uncategorized] ~239-~239: The abbreviation “i.e.” (= that is) requires two periods.
Context: ... checked by Home Assistant in each 30s, ie. the effective resolution of this option...(I_E)
194-241: Consider adding a visual timeline diagramThe documentation thoroughly explains the health check mechanisms, but a visual timeline diagram could help users better understand how
healthcheck_offline_timeoutandhealthcheck_restart_timeoutinteract with different Tailscale states.Example diagram structure:
Timeline: [Starting] -> [Online] -> [Offline] -> [Restarting] -> [Online] | | | | | | | | restart_timeout | offline_timeout restart_timeout🧰 Tools
🪛 LanguageTool
[uncategorized] ~204-~204: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...s only after Tailscale was once online, ie. it won't have any effect when Tailscale...(I_E)
[uncategorized] ~205-~205: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...e any effect when Tailscale's status is eg. Starting, NeedsLogin or NeedsMachineAut...(E_G)
[uncategorized] ~213-~213: The abbreviation “i.e.” (= that is) requires two periods.
Context: ... checked by Home Assistant in each 30s, ie. the effective resolution of this option...(I_E)
[uncategorized] ~221-~221: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...r (re)start. The minimum value is 900, ie. 15 minutes, to always give you enough t...(I_E)
[uncategorized] ~229-~229: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...appens only when Tailscale is starting, ie. it won't have any effect after Tailscal...(I_E)
[uncategorized] ~239-~239: The abbreviation “i.e.” (= that is) requires two periods.
Context: ... checked by Home Assistant in each 30s, ie. the effective resolution of this option...(I_E)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
tailscale/DOCS.md(2 hunks)tailscale/Dockerfile(1 hunks)tailscale/config.yaml(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run(1 hunks)tailscale/rootfs/usr/bin/healthcheck(1 hunks)tailscale/translations/en.yaml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
tailscale/DOCS.md
[uncategorized] ~204-~204: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...s only after Tailscale was once online, ie. it won't have any effect when Tailscale...
(I_E)
[uncategorized] ~205-~205: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...e any effect when Tailscale's status is eg. Starting, NeedsLogin or NeedsMachineAut...
(E_G)
[uncategorized] ~213-~213: The abbreviation “i.e.” (= that is) requires two periods.
Context: ... checked by Home Assistant in each 30s, ie. the effective resolution of this option...
(I_E)
[uncategorized] ~221-~221: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...r (re)start. The minimum value is 900, ie. 15 minutes, to always give you enough t...
(I_E)
[uncategorized] ~229-~229: The abbreviation “i.e.” (= that is) requires two periods.
Context: ...appens only when Tailscale is starting, ie. it won't have any effect after Tailscal...
(I_E)
[uncategorized] ~239-~239: The abbreviation “i.e.” (= that is) requires two periods.
Context: ... checked by Home Assistant in each 30s, ie. the effective resolution of this option...
(I_E)
🔇 Additional comments (3)
tailscale/config.yaml (1)
39-39: Verify the schema definition for healthcheck_restart_timeout
The schema defines healthcheck_restart_timeout as int(900,)?. Please ensure that this syntax correctly enforces a minimum value of 900 as intended. If the schema requires a different format to enforce a minimum value, consider updating it accordingly.
tailscale/Dockerfile (1)
38-40: HEALTHCHECK command added correctly
The addition of the HEALTHCHECK command appears to be correct and properly implemented.
tailscale/rootfs/etc/s6-overlay/s6-rc.d/post-tailscaled/run (1)
93-93: Verify inclusion of "Stopped" in backend state condition
Including .BackendState == "Stopped" in the condition may affect the script's logic. Please verify that considering the "Stopped" state here is intentional and ensures correct behavior during service startup and monitoring.
tailscale/DOCS.md
Outdated
| healthcheck_offline_timeout: 110 | ||
| healthcheck_restart_timeout: 3600 |
There was a problem hiding this comment.
I really don't want to keep add options like this.
People shouldn't have to worry about this stuff.
../Frenck
There was a problem hiding this comment.
Hardwire 5 minutes and 1 hour? Based on my logs, 2-3 minutes long dropouts occure approx. daily.
It also means that healthcheck will be live after update. I'm just a bit afraid that a "regular" restart can ruin something. Maybe DNS. I was never able to configure HA to use TS's DNS, so I can't test it, it's just a wild guess.
Proposed Changes
This PR treats the following situations unhealthy:
In my case a plain WAN switch-over to backup WAN on the router was able to permanently make Tailscale offline. And it's not only me: tailscale/tailscale#12021 It seems to be an issue when TS is in a container. And similar bugs can happen in the future.
And after testing it in real life circumstances, TS was able to not go online even after a restart, if the transient in the WAN state was long enough. But after (re)start, we should restart the add-on again much slower, eg. to give time to authenticate on the very first time.
Note: I was able to put TS into Stopped state only manually, but maybe this can be helpful once, at least won't harm.
Note: This schema (below) would look nicer, but until a bug in the Supervisor/UI is not fixed (see #2640), we can't do this.
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation