openthread_border_router: Fix start with containerd.io to 1.7.24-1 #3864
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @nodamnway
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
📝 WalkthroughWalkthroughThe pull request updates the OpenThread Border Router (OTBR) addon configuration and changelog. The version has been bumped from 2.12.3 to 2.12.4, addressing an issue with the containerd.io update to version 1.7.24-1. The changelog details improvements in radio timeout recovery, mesh header fragmentation, and compile-time configurations. A new device entry for Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
![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 (1)
openthread_border_router/CHANGELOG.md (1)
3-6: Enhance changelog entry with implementation detailsWhile the changelog entry correctly identifies the issue, it would be helpful to include the actual fix implemented (addition of
/dev/net/tundevice access). This helps users and maintainers understand the solution.Consider updating to:
## 2.12.3 -Fix OTBR addon does not start after updating containerd.io to 1.7.24-1 +Fix OTBR addon not starting after containerd.io 1.7.24-1 update by adding /dev/net/tun device access
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openthread_border_router/CHANGELOG.md(1 hunks)openthread_border_router/config.yaml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
openthread_border_router/CHANGELOG.md (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
openthread_border_router/config.yaml (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🔇 Additional comments (2)
openthread_border_router/config.yaml (2)
2-2: LGTM: Version bump follows semantic versioning
Appropriate patch version increment for a configuration fix.
23-24: LGTM: Device access configuration addresses the root cause
The addition of /dev/net/tun device access is the correct fix for the OTBR startup issue with containerd.io 1.7.24-1. This aligns with the containerd GitHub issue recommendation.
Let's verify if this device is required by other addons for similar functionality:
✅ Verification successful
Device access configuration is unique to OpenThread Border Router
The verification confirms that /dev/net/tun device access is only configured in the OpenThread Border Router addon and not used by other addons in the repository, making this a targeted fix specific to OTBR's networking requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other addons that might use tun device
rg -l "/dev/net/tun" --type yaml
Length of output: 69
agners
force-pushed
the
platformConfigureTunDevice_operation_not_permitted
branch
from
23977a6 to
641ce2a
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
openthread_border_router/Dockerfile (1)
Line range hint
1-108: Critical: Container fails to start due to missing device configuration.The Dockerfile changes don't address the reported issue with
/dev/net/tunaccess that prevents the container from starting after the containerd.io update. According to the PR objectives, this device needs to be enabled in the configuration.The container needs access to
/dev/net/tunfor network interface configuration. This should be configured in the addon'sconfig.yamlas mentioned in the PR objectives:devices: - /dev/net/tunPlease ensure this configuration is added to resolve the startup failure.
🧹 Nitpick comments (6)
tellstick/README.md (1)
5-6: Add comma for improved readabilityAdd a comma before "and" to separate independent clauses.
-years ago and it cannot be built on Alpine versions above 3.15. +years ago, and it cannot be built on Alpine versions above 3.15.🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ned. Its last activity was 5 > years ago and it cannot be built on Alpine versions a...(COMMA_COMPOUND_SENTENCE)
openthread_border_router/translations/en.yaml (1)
17-18: Enhance the warning message format.The warning message effectively directs users to the documentation. However, consider making it more prominent by using an exclamation mark at the start.
- over above configuration). Not recommended! See documentation for more + over above configuration). ⚠️ Not recommended! See documentation for moreopenthread_border_router/DOCS.md (1)
70-83: Fix grammar in the warning message.The warning message effectively explains the risks of using TCP/IP for RCP connections. However, there's a grammar issue that needs to be fixed.
- If the TCP/IP connection fails, the OTBR will not shutdown cleanly and leave + If the TCP/IP connection fails, the OTBR will not shut down cleanly and leave🧰 Tools
🪛 Markdownlint (0.37.0)
83-83: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
🪛 LanguageTool
[grammar] ~73-~73: The word “shutdown” is a noun. The verb is spelled with a space.
Context: .../IP connection fails, the OTBR will not shutdown cleanly and leave > stale routes in you...(NOUN_VERB_CONFUSION)
openthread_border_router/Dockerfile (1)
93-94: Review the restoration settings and document their impact.The new build flags introduce important changes:
-DOT_PROJECT_CONFIGpoints to the new header file-DOT_RCP_RESTORATION_MAX_COUNT=2sets a limit on restoration attemptsThese settings should be documented to explain their purpose and impact on system behavior.
mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/run (1)
8-13: LGTM! Consider adding validation for the arguments.The implementation correctly reads and applies custom MariaDB server arguments.
Consider adding validation to ensure the arguments are safe and well-formed before applying them.
mariadb/DOCS.md (1)
77-82: Enhance documentation with more examples and warnings.While the documentation is clear, it could benefit from:
- More example configurations for different scenarios
- Warnings about potential impacts of incorrect parameters
- Guidelines for choosing appropriate values
🛑 Comments failed to post (1)
mariadb/rootfs/etc/s6-overlay/s6-rc.d/mariadb-core/run (1)
18-22: 🛠️ Refactor suggestion
Add quotes around array expansion.
The array expansion needs proper quoting to handle arguments containing spaces.
Apply this diff:
- exec mysqld --datadir="${MARIADB_DATA}" --user=root "${extra_args[@]}" < /dev/null + exec mysqld --datadir="${MARIADB_DATA}" --user=root "${extra_args[@]+"${extra_args[@]}"}" < /dev/null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if [ -z "${extra_args+x}" ] || [ ${#extra_args[@]} -eq 0 ]; then exec mysqld --datadir="${MARIADB_DATA}" --user=root < /dev/null else exec mysqld --datadir="${MARIADB_DATA}" --user=root "${extra_args[@]+"${extra_args[@]}"}" < /dev/null fi
|
This actually got resolved with the containerd.io release 1.7.25 and the Debian package 1.7.25-1 for it. However, I think it still make sense to explicitly add permission for the tun device, just in case Docker plans to indeed go through with removing access to it by default. |
agners
changed the title
Issue
OTBR Addon fails to load with
platformConfigureTunDevice() at netif.cpp:2022: Operation not permittedI upgraded containerd.io to
1.7.24-1on Debian 12 and this started to happen preventing the container from starting.Logs are available here:
home-assistant/core#132124
#3826 (comment)
As a temporary solution, I downgraded to
containerd.io=1.7.23-1, which worked for me and some other users.Reason and solution
containerd/containerd#11078 (comment)
so the recommended solution is to enable the
/dev/net/tundevice:Testing
I only did local testing on my Home Assistant Supervised machine, such as:
/usr/share/hassio/addons/core/openthread_border_router/config.yamlby adding the/dev/net/tunto devices (and didn't bump the version, as OTBR addon won't install then)Add-on StoreNot sure if this is the right way to test, but it was the fastest for me and it solved my issue.
Summary by CodeRabbit
Bug Fixes
Chores
/dev/net/tun