Raise the update hotspot, and always put the network back #54
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/networkmanager-hotspot"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
updater/radio.pydrives the host's NetworkManager throughnmcliover thesystem bus compose already mounts, so nodes can join a
campervan-otaaccesspoint to pull firmware. With Ethernet up, or with nothing up, the hotspot goes
alongside whatever is there; only when
wlan0is the uplink does it come downfirst, and Home Assistant is then unreachable from the network until the update
finishes.
The failure that matters is a Pi stranded as an access point, so all four
guards are here: a stale
campervan-otais deleted at startup, a restoredeadline is armed when the hotspot goes up, the uplink profile keeps its
autoconnectso a power cut recovers without the updater, and SIGTERM restoreswithin the
stop_grace_periodcompose sets. The address is read back fromNetworkManager rather than assumed to be 10.42.0.1, since a later ticket puts
it on the bus.
The hotspot passphrase is generated per run rather than configured — a fixed
one in the repository would be a shared secret, and the node learns this one
the same way it learns the address.
Tested through a fake
nmcliat the seam: which case we are in, the startuptear-down, the deadline firing, and restore on every failure path.
nmcliitself is not exercised, and the image now installs it.
CCS-UHA-2.
Three of these break the guarantee the ticket is built around — a restore that half-fails leaves no record of what to put back, and a crashed container leaves
wlan0down for good. Detail inline.@ -0,0 +270,4 @@async def test_a_line_with_fewer_fields_than_asked_for_is_skipped() -> None:"""nmcli prints the odd warning of its own, and it is not a device."""CI fails here: ruff D403 (pinned 0.16.6) wants the docstring's first word capitalised.
@ -0,0 +66,4 @@passphrase: strasync def run_nmcli(*arguments: str) -> str:No timeout. nmcli defaults to 90 s for
connection upanddevice wifi hotspot(nmcli(1)), so a restore on SIGTERM can outrunstop_grace_period: 30sand get SIGKILLed mid-way, and a slow_address()means the deadline at line 190 is never armed while the hotspot is already up. Pass--waitbounded under the grace period.@ -0,0 +136,4 @@)wireless: Uplink | None = Nonefor line in listing.splitlines():fields = _fields(line)Nothing checks the wireless uplink is on
self._device, so an uplink on a second radio would be taken down for a hotspot that never touches it.@ -0,0 +166,4 @@# Never `connection modify ... autoconnect no` to go with this: the# profile's autoconnect is what recovers the Pi after a power cut,# with the updater out of the picture entirely.await self._nmcli("connection", "down", uplink.connection)nmcli connection downinternally blocks that profile from autoconnecting "until reboot or until the user performs an action that unblocks autoconnect" (nmcli(1),connection down). So if the container is killed rather than stopped, the next start'sclear_stale_hotspotfreeswlan0but NetworkManager will not reactivate the uplink, and_suspendeddied with the process. Leavingconnection.autoconnectset only covers the power-cut case. Have the startup path runnmcli device connect <device>after clearing a stale hotspot, and drop the README claim (line 170) that a crash cannot leave it stranded.@ -0,0 +184,4 @@passphrase,)hotspot = Hotspot(await self._address(), HOTSPOT_SSID, passphrase)except RadioError:except RadioErrormissesOSError/FileNotFoundErrorfromcreate_subprocess_exec— nmcli absent from the image, say — which leaves the uplink down with no local restore. CatchExceptionand re-raise.@ -0,0 +199,4 @@service stopping, and more than one of them can arrive."""self._disarm()await self._remove_hotspot()If
_remove_hotspot()raises, the uplink is never brought back up — theconnection upbelow is unreachable. Put the teardown in atry/finallyso the uplink comes back regardless of what the delete did.@ -0,0 +200,4 @@"""self._disarm()await self._remove_hotspot()suspended, self._suspended = self._suspended, None_suspendedis cleared before theconnection uphas succeeded, so a transient failure loses the only record of what to put back. Trace it: wireless uplink suspended, deadline fires,connection deletesucceeds,connection up Van WiFiraises. The error propagates out of_restore_on_deadlineinto a task nobody awaits,_suspendedis nowNone, and the SIGTERM restore later does nothing. Pi left with no hotspot and no uplink — the exact failure the ticket names. Clear_suspendedonly after theupreturns.@ -33,3 +35,4 @@"""loop = asyncio.get_running_loop()stop = asyncio.Event()radio = Radio() if radio is None else radiotests/test_updater.py::test_a_termination_signal_stops_the_servicepasses no radio, so it builds a realRadioand shells out to the host'snmcli— I confirmed twonmcli -g NAME connection showsubprocesses against this machine's NetworkManager. On a host that has acampervan-otaprofile the suite would delete it; on a host withoutnmclithe test fails. Pass a fake radio there.@ -39,2 +42,4 @@await radio.clear_stale_hotspot()await serve(settings, stop)finally:await radio.restore()If
clear_stale_hotspot()fails — no/run/dbus, NetworkManager not running, AppArmor denying the bus — thisrestore()fails the same way, replaces the original exception and skipsremove_signal_handler. The container then crash-loops on a secondary traceback that does not say what actually went wrong. Catch the startup failure, log it and carry on.Last round's findings are all addressed, but holding
_suspendedacross theconnection upintroduced a crash when two restores overlap — reproduced, detail inline. Therun_nmclikill path is clean: I ran a hanging nmcli against it and the child is reaped with no zombie, no fd leak and no ResourceWarning.@ -0,0 +33,4 @@RESTORE_AFTER = timedelta(minutes=15)"""How long the access point may stand before the uplink comes back regardless."""NMCLI_TIMEOUT = timedelta(seconds=20)The 20 s is per call, but a restore makes three:
connection show,connection delete, thenconnection up. Worst case is 60 s againststop_grace_period: 30s, so the docstring's claim that this fits inside the grace period does not hold. Budget the whole restore rather than each call, or raise the grace period to cover 3 × the timeout.@ -0,0 +36,4 @@NMCLI_TIMEOUT = timedelta(seconds=20)"""How long one nmcli may take.Its own default wait is 90 seconds, which outlasts the `stop_grace_period` aWorth saying here that this bounds nmcli, not NetworkManager: the D-Bus request has already gone, so a timed-out
connection upusually still completes and aRadioErrorfrom a timeout does not mean the operation did not happen. (The hotspot case is fine — nmcli creates the profile before activation finishes, so_remove_hotspotstill finds it.)@ -0,0 +153,4 @@return_LOGGER.warning("Cleared a %s left by an earlier run", HOTSPOT_CONNECTION)try:await self._nmcli("device", "connect", self._device)This runs whenever a stale profile was deleted, including the Ethernet case where the previous run never took
wlan0down and nothing is blocked. Two consequences from nmcli(1)device connect: it "will also consider connections that are not set to auto connect", so it can activate a wifi profile the user deliberately setautoconnect noon — host policy the updater never took away; and "if no compatible connection exists, a new profile with default settings will be created and activated", which I have not verified for a wifi device with no saved profile and did not want to run against a live network. Worth checking once on the Pi that a straywlan0profile is not left behind on every restart.The precise version is to write the suspended uplink's name into the firmware-cache volume when you suspend it and bring back exactly that at startup. Then the Ethernet case does nothing at all.
@ -0,0 +238,4 @@finally:if self._suspended is not None:await self._nmcli("connection", "up", self._suspended.connection)_LOGGER.info("Uplink %s is back", self._suspended.connection)Two overlapping restores crash here. The check on line 239, the
await, and the clear on line 242 are no longer one step, so both callers pass the check, both runconnection up, the first clears_suspended, and the second dereferencesNone—AttributeError: 'NoneType' object has no attribute 'connection'. It is not aRadioError, so service.py's handler does not catch it and the shutdown path reports a failure it did not have.Reachable today:
_restore_on_deadlinenulls_deadlinebefore callingrestore(), so a SIGTERM arriving while the deadline is mid-restore cannot disarm it and both run. I reproduced it with aconnection upthat takes 50 ms; the call log also showsconnection delete campervan-otatwice, and the second would fail against real nmcli.Take
suspended = self._suspendedinto a local before the await, or put anasyncio.Lockaround the body ofrestore()— the lock also fixes the double delete.@ -36,3 +39,4 @@for number in STOP_SIGNALS:loop.add_signal_handler(number, stop.set)try:await radio.clear_stale_hotspot()stopis set by the signal handler but not read untilserve(), so a SIGTERM arriving duringclear_stale_hotspotis not honoured until it finishes — up to 60 s at the current per-call timeout, withdevice connecttaking a full 20 s to fail when there is nothing to reconnect to.docker compose downshortly after a start gets SIGKILLed rather than shutting down cleanly.Second round taken in
c45dc24.Fixed: the overlapping-restore regression, by an
asyncio.Lockaroundrestore()plus a local for the suspended uplink, so a second caller finds the work done rather than aNone. Test covers two restores racing with a slowconnection up.device connectis gone, and with it the risk you flagged of nmcli inventing a profile. The uplink taken down for an update is now written to/var/lib/campervan-updater/state/suspended-uplinkon a new compose volume, beforeconnection downruns, andrecover()at startup brings back exactly that profile. A note that cannot be written stops the uplink coming down at all, which is the way round that keeps the Pi reachable. The Ethernet case is now a genuine no-op.NMCLI_TIMEOUTis 9 seconds, so a restore's three calls fit inside the 30 secondstop_grace_periodrather than overrunning it by double.Not changed, deliberately: a SIGTERM arriving during
recover()is still not acted on untilserve(), so a slow startup recovery can be SIGKILLed part way. That leaves either the stale hotspot or the noted uplink exactly as it found them, and the next start heals both — the note is what makes that true now. Plumbingstopthrough the recovery would buy a faster exit in a case that already self-heals.The lock is right:
restore()is its only holder, never re-enters, and_restore_on_deadlinenulls_deadlinebefore calling it, so_disarmcannot cancel a task queued on the lock and the second acquirer correctly finds nothing to do. No deadlock, no skipped restore. The note's write-before-down half is right too — a crash between the two costs a redundantconnection up, nothing more.recover()doing both jobs is the right shape: both are "undo the last run", the order matters, and there is one caller.The window that still loses the uplink is the in-memory half of the same ordering. Two findings inline.
@ -0,0 +217,4 @@# profile's autoconnect is what recovers the Pi after a power cut,# with the updater out of the picture entirely.await self._nmcli("connection", "down", uplink.connection)self._suspended = uplink_suspendedis set after theconnection down, so a failed or timed-out down leaves the note on disk and nothing in memory. The uplink is down with autoconnect blocked, no hotspot went up, no deadline was armed, andrestore()on SIGTERM does nothing — I ran it: the only call it makes afterwards is-g NAME connection show. The note only helps at the next start, and there will not be one, because the caller just catches the exception and the service keeps running.Nine seconds is a short budget for a deactivation that normally takes well under one. Move this line above the
connection down, the same way the note already is — aconnection upon a profile that never went down is harmless, and then in-memory and on-disk state agree.@ -0,0 +170,4 @@# Dropped rather than retried next start: a profile that will not# come up now will not come up then either, and the note would# outlive the reason for it._LOGGER.exception("Could not bring %s back", noted)Dropping the note when the
upfails is wrong for something that moves. The comment's premise — "a profile that will not come up now will not come up then either" — holds for a fixed installation, but a van starts where the AP is out of range. The block on that profile's autoconnect is still in place, which is the whole reason the note exists, so once the note is gone the Pi will not rejoin that network until a reboot.Clear the note only after a successful
up. It self-clears on the first start where the network is there, at the cost of one failing call per start until then.test_a_noted_uplink_that_will_not_come_up_is_not_owed_foreverinverts with it.@ -0,0 +212,4 @@# Noted before it goes anywhere: if the note cannot be written, the# uplink stays up and the update does not happen, which is the way# round that leaves the Pi reachable.self._remember(uplink)Minor:
raise_hotspotwrites_suspended, the note and_deadlinewithout the lockrestore()now takes. Nothing can trigger it today, but once CCS-UHA-3 calls this, a deadline restore in flight can clear_suspendedand_forget()the note a new raise has just written. Either holdself._restoringhere too, or say in the docstring that raising and restoring are never concurrent.Third round taken in
1e56a7f._suspendedis now set beforeconnection down, and the down moved inside thetry, so a failed or timed-out deactivation restores immediately and a SIGTERM afterwards still knows what it owes. New test: a refusedconnection downends withconnection up Van WiFiand the note cleared.recover()keeps the note when theconnection upfails — your reading is right, a van parked out of range is the ordinary case, and the blocked autoconnect means nothing else would ever bring it back. The test inverted with it.Not changed:
raise_hotspotstill writes_suspended, the note and the deadline outside the restore lock. Agreed it wants fixing, but nothing raises a hotspot yet, so there is no second caller to race — the right time is when CCS-UHA-3 or the OTA ticket wires up a caller and the shape of the critical section is actually known. Guarding only part of the raise now would read as safe without being it.Both findings are fixed. I ran the three failure points — a refused
connection down, hotspot and address read — and each one now bringsVan WiFiback up and clears the note, with a laterrestore()correctly finding nothing left to do. Declining the lock onraise_hotspotis fair: a partial critical section reading as safe is the worse outcome.One test-isolation nit inline; nothing else.
@ -36,3 +39,4 @@for number in STOP_SIGNALS:loop.add_signal_handler(number, stop.set)try:await radio.recover()Not this file, but reached from here:
tests/test_updater.pybuildsRadio(no_connections)with nonote=, and thenotefixture that redirectsSUSPENDED_UPLINKis autouse only intests/test_updater_radio.py. Sorecover()there reads the real/var/lib/campervan-updater/state/suspended-uplink, and if one existed — on the Pi — the stubbedconnection upwould "succeed" and_forget()would unlink it. Pass anote=tmp_path / ..., or move the fixture intoconftest.py.