Repository navigation
fix(utils): don't wait for IP addresses when the host agent is not running - #3368
s-celles-bot wants to merge 1 commit into
Conversation
|
|
Thanks — the report in #3366 is accurate and this reproduces exactly as described on our hardware. I reviewed the change and ran it on the QA rig. You left "end-to-end test for Raspberry Pi devices" unchecked, so I covered that for you:
In every case the cached IP addresses were still returned correctly and the misleading I also diffed the behaviour state-by-state against Three things to fix: 1. Stale documentation elsewhere in the tree. You updated the
They all say "waits up to ~80s (60s 2. The 3. Worth noting the limit of the fix in the docstring. Minor: this conflicts with your #3367 — both append to the end of None of these are blockers on the fix itself. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3368 +/- ##
=========================================
Coverage ? 90.75%
=========================================
Files ? 85
Lines ? 10475
Branches ? 1163
=========================================
Hits ? 9507
Misses ? 708
Partials ? 260 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
One more thing, and apologies that this isn't written down anywhere public yet — it's a house rule rather than something you could have inferred from All commits need to be signed with an SSH key. This commit currently shows as unsigned, so it'll need a signed replacement before it can go in. If you haven't set up SSH signing before: git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub
git config --global commit.gpgsign trueThen add that same public key to your GitHub account a second time, under Settings → SSH and GPG keys → New SSH key, with the key type set to Signing Key — an authentication key alone won't make commits show as Verified. Since there's only one commit here, re-signing it is: git commit --amend --no-edit
git push --force-with-leaseYou'll be force-pushing anyway, so it's a good moment to fold in the review points from my earlier comment in the same push if you want to. Worth saying clearly: this is a process box to tick, not a reflection on the change. The fix itself tested clean on our hardware. |
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Correct fix and nicely contained. Skipping the publish along with the wait is the right call and not just an optimisation — Redis pub/sub has no persistence, so a hostcmd sent to an absent agent is dropped on the floor rather than queued, and a late-starting agent would never have seen it anyway. The log messages are a real improvement too; "Internet connection is not available." was describing the one thing that definitely wasn't wrong.
Two things.
The ~80s arithmetic is now wrong in two more places. You updated the docstring in utils.py, but _resolve_node_ip repeats the same figure, and it's the docstring whose entire job is explaining why it doesn't call get_node_ip() — src/anthias_server/api/views/v2.py:152:
``anthias_common.utils.get_node_ip()`` ... publishes
``set_ip_addresses`` to host_agent and waits up to ~80s
(60s host_agent_ready + 20s ip_addresses_ready) for the result.
After this change the no-agent path is 60s, not 80s. Worth correcting there so the two don't drift — the reasoning still holds either way, the number just isn't right.
The 60s is still paid on every call. I'm not asking you to widen the PR — InfoViewV2.get_ip_addresses (v2.py:1029) already says out loud that blocking there is a deliberate trade, and the polling endpoint has _safe_ip_addresses(), so the dangerous path is covered. But on a board with no host agent, each /api/v2/info request now burns a full minute of a worker, and the answer is known after the first one. The precedent for fixing that is directly above _resolve_node_ip:
_IP_REFRESH_PENDING_KEY = 'splash:ip_refresh_pending'
_IP_REFRESH_DEBOUNCE_S = 12A short-TTL negative cache in the same style — write a key when the 60 retries expire, return early on a hit — would turn repeat calls into a cache read while still letting a host agent that starts later be picked up when the TTL lapses. Follow-up, not a blocker, but it's the half of #3366 that an operator would actually notice.
Tests are good: asserting 'ip_addresses_ready' not in keys pins the skip at the level that matters rather than timing it, and checking 'Internet connection' not in caplog.text keeps the old message from creeping back. Patching utils.sleep in one test and tenacity.nap.time.sleep in the other is correct — they're genuinely different sleeps.
Happy to approve with the v2.py:152 docstring corrected.
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Approving. The fix is correct and the log messages are a real improvement — "Internet connection is not available" for a missing host agent has probably sent more than one person looking at the wrong thing.
I traced the new flag through every exit from the first loop, since a wrongly-True host_agent_ready would reintroduce the 20s wait silently:
environment in ['development', 'test']→ breaks withTrue, which is what you want: the second loop also short-circuits on the same check, so dev/test behaviour is unchanged.json.loads(is_ready)truthy → breaks withTrue. Correct, the agent is up.retries >= max_retries→ the only path that setsFalse.
There's no other way out of that while True, so the flag can't be stale. And falling back on r.get('ip_addresses') regardless of which branch ran means a device whose agent died after publishing still reports its last known addresses instead of 'Unable to retrieve IP.' — worth the explicit comment you gave it.
The tests pin the two things that would regress: that ip_addresses_ready is never read (via call_args_list, rather than just asserting on elapsed time, which would be flaky), and that the old "Internet connection" wording is gone. Checking the absence of the misleading string and the presence of the accurate one is the right pair of assertions.
One thing worth a follow-up
Your own measurement is the interesting part: 79s → 60.3s. The 20s you removed was the smaller half — GET /api/v2/info still blocks for a full 60 seconds on a device with no host agent, inside a request/response cycle. That's untouched here and I don't think it should hold this PR up, but it's the cost that actually hurts, and it's now the only cost.
Two directions, if you want to open an issue for it: return the cached ip_addresses immediately and let the refresh happen out of band, or shrink max_retries on the grounds that an agent which isn't up within a few seconds of the first poll is not coming up during this request. The first wait exists to cover agent startup racing the first API call, which argues for a much smaller budget than 60s once the unreachable case no longer pays the extra 20.
Minor: the docstring's "waits up to ~80s" headline now describes only the agent-present-but-silent case. Still technically the maximum, so fine to leave, but the new sentence you added is doing the real explaining.
CI is green and the branch is 10 commits behind master — same as #3367, worth a rebase before merge.
…nning get_node_ip() waited 60 s for the host agent, then 20 s more for the IP addresses that only the host agent publishes, and logged "Internet connection is not available." while waiting. Without host agent the second wait cannot succeed: it is now skipped, the last cached addresses are still returned, and the log says that the host agent did not publish the IP addresses. GET /api/v2/info without host agent now answers after 60 s instead of 80 s. The set_ip_addresses publish to hostcmd is skipped too: Redis pub/sub does not queue messages, so no host agent would receive it. Fixes Screenly#3366 Assisted-by: AI
2662678 to
9ed8339
Compare
|
Issues Fixed
Description
get_node_ip()waited up to 60 s forhost_agent_ready, then up to 20 s forip_addresses_ready, which only the host agent sets, and logged "Internet connection is not available." during the second wait.Checked with the
screenly/anthias-server:0115908-x86image and thisutils.pymounted over the original, without host agent:GET /api/v2/infoanswers after 60.3 s instead of 79 s, and the log readshost_agent_service is not ready after 60 retries: the host agent is not running, so the IP addresses cannot be refreshed.Checklist
tests/test_utils.py, the non-integration suite withTZ=UTC, ruff and mypy; CI not run yet.)Assisted-by: AI