Add pending-dataset TLV write support - #269
Conversation
|
Kindly requesting CI workflow approval to see if anything pops up. cc @puddly |
There was a problem hiding this comment.
Pull request overview
Adds raw pending-dataset TLV write support needed for coordinated Thread network migration.
Changes:
- Adds
set_pending_dataset_tlvs(). - Adds typed timestamp and delay-timer construction/parsing.
- Improves malformed-TLV errors and credential-safe logging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
python_otbr_api/__init__.py |
Adds the pending-dataset TLV endpoint. |
python_otbr_api/tlv_parser.py |
Adds TLV builders, typed decoding, and safer errors/logging. |
tests/test_init_legacy.py |
Tests pending-dataset requests and errors. |
tests/test_tlv_parser.py |
Tests new parser and builder behavior. |
pyproject.toml |
Bumps the release to 2.11.0. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
758881b to
85ec955
Compare
|
Can we try this PR with any TBR? |
|
The way I've tested and am using this is with an OpenWrt-based TBR + Matter Network Infrastructure Manager, i.e. the OpenWrt device is added to the Matter fabric and its TBR can then be managed through HA via Matter. See also project-chip/matter-openwrt#53 Edit: Forgot to mention, this also enables Thread cred rotation on the HA managed OTBR. |
agners
left a comment
agners
left a comment
There was a problem hiding this comment.
Besides the pending dataset overwrite protection, I wonder if we should move some of the logic of home-assistant/core#178291 into the library.
Or maybe even one step further: Move it into the OTBR backend (REST API). But since we generally support older BRs, we'd probably need validation here still 🤔 .
85ec955 to
7a245c9
Compare
7a245c9 to
52c8086
Compare
|
I believe this should be good to go now |
| mesh silently ignores a pending dataset that is not newer than the one | ||
| it already holds, while the router accepts the write. A caller that | ||
| deliberately supersedes the in-flight dataset -- having stamped the new | ||
| one above it -- passes allow_replace=True. |
There was a problem hiding this comment.
Why is allow_replace needed actually? The driving feature here is a feature which allows to merge Thread networks (home-assistant/core#178291), but do we need to be able to merge one while another pending dataset is in flight? Similar to what we discussed in #272 (review), this seems unnecessary risk to me... Or maybe I miss something?
I get that we added support to make writing pending dataset safer in openthread/ot-br-posix#3552 (not accidentally overwriting something we did not intend to). But from how I understood it was mostly about supporting If-None-Match: *, allowing to safely prevent overwriting a pending dataset if one is present. And that is a nice addition to the OTBR REST API.
But from what I can tell, the use cases we want to support should not require pending dataset overwrite.
There was a problem hiding this comment.
The use case is aborting a migration, not merging a dataset during an ongoing one.
Once a pending dataset has propagated, every device is counting down; delete_pending_dataset()
only drops the local copy, so the sole way to stop the switch is a newer pending dataset.
Otherwise a mistaken migration is irreversible until it completes.
test_in_flight_migration_away_is_superseded in home-assistant/core#178291
covers that.
I agree that this isn't ideal yet, but the "replace the dataset I just read" case needs #274 on
openthread/ot-br-posix#3553, which I'd like to defer for now. So this flag stays the only
mechanism for the time being, and as-is home-assistant/core#178291 depends on it.
There was a problem hiding this comment.
Otherwise a mistaken migration is irreversible until it completes.
Yes, that is the consequence which I would accept.
As stated in #272 (review), the problem of sending a new pending dataset is that it's not certain that all devices receive it in time. So you may end up some devices migrated, and some not. Now you can make the argument so what, it is a "best effort" cancellation. But the problem I see is that with silently incrementing timestamp and sending another dataset doesn't make the user aware that there was already one pending, and the possible consequences.
Also, this potentially reverts a pending set channel operation without the user knowing. And as we add more features, how can we make sure those don't interfere. Rejecting any changes while a pending dataset is present is also a cheap way to avoid any such concurrency issue.
IMHO, home-assistant/core#178291 should just handle the upstream rejection and tell the user. But maybe we should discuss the Core PR in general first.
162e5b3 to
08fffc2
Compare
Add set_pending_dataset_tlvs(), the pending-endpoint twin of set_active_dataset_tlvs(), so a controller can hand a border router a new operational dataset as a raw TLV pending set. The TLV parser gains the write-side counterparts callers need to build such a dataset: Timestamp.from_values(), with range checks for the 48-bit seconds and 15-bit ticks fields, and DelayTimer.from_milliseconds(). PENDINGTIMESTAMP and DELAYTIMER now decode to typed items, matching ACTIVETIMESTAMP. Also raise TLVError instead of leaking struct.error on a malformed timestamp or delay timer TLV, and log unknown TLVs by type and length only -- the value may hold network credentials. Like the channel change, the write is refused while a pending dataset is already in place. A not-newer one would be silently ignored by the mesh, and superseding is never safe to do implicitly: the replacement races the delay timer on every device that already holds the old dataset, so a late replacement can split the mesh, and it would also silently undo whatever the in-flight dataset was doing. A caller that finds the write refused should surface that, not retry. The check runs locally first, and again on the border router: unless allow_replace is set the request carries "If-None-Match: *", which a border router with openthread/ot-br-posix#3552 evaluates atomically with the write, closing the race the local check leaves open. Older border routers ignore the header and keep relying on the local check. Both refusals raise PendingDatasetConflictError rather than a plain OTBRError, so a caller can tell them apart from a transport or protocol failure without matching on the message: nothing was written, and the state that made the write wrong is there to be read back. Assisted-By: Claude Fable 5
08fffc2 to
ea41b20
Compare
Add
set_pending_dataset_tlvs(), the pending-endpoint twin ofset_active_dataset_tlvs(), so a controller can hand a border router a new operational dataset as a raw TLV pending set.The TLV parser gains the write-side counterparts callers need to build such a dataset:
Timestamp.from_values(), with range checks for the 48-bit seconds and 15-bit ticks fields, andDelayTimer.from_milliseconds().PENDINGTIMESTAMPandDELAYTIMERnow decode to typed items, matchingACTIVETIMESTAMP.Also raise TLVError instead of leaking struct.error on a malformed timestamp TLV, and log unknown TLVs by type and length only -- the value may hold network credentials.
The write is refused while a pending dataset is already in place. Superseding one is never safe to do implicitly: the replacement races the delay timer on every device that already holds the old dataset, so a late replacement can split the mesh, and it would silently undo whatever the in-flight dataset was doing. Both refusal paths raise
PendingDatasetConflictError, anOTBRErrorsubclass, so callers can tell the refusal apart without matching messages.The check runs locally first, and again on the border router: the request always carries
If-None-Match: *, which a border router with openthread/ot-br-posix#3552 evaluates atomically with the write, closing the race the local GET-then-PUT check leaves open. Older border routers ignore the header and keep relying on the local check. This PR is the client-side counterpart of that one.Required by: home-assistant/core#178291
Counterpart to: openthread/ot-br-posix#3552
AI use: Written with AI assistance -- the commit carries an
Assisted-Bytrailer. I've reviewed and understand all of it, and will be answering questions myself.