fix(edge-host): make the bootstrap self-contained and the runbook followable #474
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: konairius/homelab#474
Reference in New Issue
Block a user
Prompted by preparing a second Wyse 3040 as an NTP peer — this is the procedure that would have been followed to build it, and it has three defects. Two are standing house rules; one simply does not work.
1. It required a DHCP reservation
A reservation, as a documented precondition. The UDM registers DHCP leases in DNS on its own, so the record appears without one — and the host is identified by name everywhere (inventory, plays, README), with its address never recorded.
2. It required
--limit, because the plays were split wrongThe
--limitwas load-bearing, not sloppiness:common.ymlishosts: all:!…, so an unrestricted run while bootstrapping one new box would reconfigure the entire fleet. The split left no other option.But
--limitnarrows the target list rather than staging anything, and leaves every other host outside convergence with no report that they were skipped.Fixed structurally rather than by deleting the flag.
install-edge-host.ymlnow carriescertificatesandcommonitself — the parts ofcommon.ymlthat apply to an edge host — so the bootstrap is one unrestricted run over theedge_hostgroup. No second command, no limit.Not carried over:
qemu-guest-agent— these are bare-metal thin clients, not VMsmonitoring-agent— Debian-oriented; edge hosts ship logs viaedge-alloyinsteadcommon.yml'sloki_endpoint/prometheus_remote_write_endpointvars belong tomonitoring-agent, so they aren't needed — verified thecommonrole doesn't reference them.3. It named a host that does not exist
The README said
edge-1.srv.renner.uno, in both the base-install steps and the Z-Wave cutover checklist. The host isedge-1.iot.renner.uno, and.srvdoes not resolve — so anyone following the runbook hits a dead name at step 3.Verification
Idempotence against the live host, before committing — whole
edge_hostgroup, no limit:All five changes are inside the
edge-hostrole (open3e venv, E3onCAN checkout/venv) — already in this play before the change, and pre-existing check-mode behaviour of git/pip installs.certificatesandcommoncontributed no changes at all.Also confirmed the
commonrole branches only onDebian/RedHatos_family, so its OS-specific tasks skip cleanly on Arch.Follow-up, not in this PR
The second Wyse should not simply join
[edge_host]— it would inherit edge-1's USB device paths ({{ edge_zwave_device }}:/dev/zwaveet al.) and try to start containers for hardware it does not have. It needs its own group feedingntp_cluster:children, plus an entry in thentp_serverspriority map. That change is deferred until the box exists and has a name.🤖 Generated with Claude Code
The documented base-install procedure had three defects, two of which are standing house rules and one of which simply does not work. 1. It required a DHCP reservation. "Add the DHCP reservation/DNS entry for <host> on the UDM" was step 3 -- a reservation as a documented precondition. Reservations are off the table here. The UDM registers DHCP leases in DNS on its own, so the record appears without one, and the host is identified by name everywhere: inventory, plays, README. Its address is never recorded. 2. It required --limit, because the plays were split wrong. Step 4 was: ansible-playbook plays/common.yml -l edge_host ansible-playbook plays/install-edge-host.yml The --limit was load-bearing, not sloppiness: common.yml is `hosts: all:!...`, so running it unrestricted while bootstrapping a single new box would reconfigure the entire fleet. The split left no other option. But --limit narrows the target list rather than staging anything, and leaves every other host outside convergence with no report that they were skipped. Fixed structurally rather than by deleting the flag: install-edge-host.yml now carries `certificates` and `common` itself -- the parts of common.yml that apply to an edge host -- so the bootstrap is one unrestricted run over the edge_host group. No second command, no limit. Not carried over: qemu-guest-agent (these are bare metal, not VMs) and monitoring-agent (Debian-oriented; edge hosts ship logs via edge-alloy). common.yml's loki/prometheus vars go with monitoring-agent and are not needed -- verified that the common role does not reference them. 3. It named a host that does not exist. The README said `edge-1.srv.renner.uno` in both the base-install steps and the Z-Wave cutover checklist. The host is `edge-1.iot.renner.uno`; the .srv name does not resolve, so anyone following the runbook hits a dead name at step 3. Verified idempotent against the live host before committing: `ansible-playbook plays/install-edge-host.yml --check --diff` over the whole edge_host group -> ok=53 changed=5 failed=0, with all five changes inside the edge-host role (open3e venv, E3onCAN checkout/venv), which was already in this play. certificates and common contributed no changes at all. Also checked the common role branches only on Debian/RedHat os_family, so its OS-specific tasks skip cleanly on Arch. Prompted by preparing a second Wyse 3040 as an NTP peer -- this is the procedure that would have been followed to build it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>🤖 Automated review — model
CLI default(complexity: complex), commite288ebe39d1f3e991033b81cac5d11016fd28346.Summary
This PR folds the
certificatesandcommonroles intoinstall-edge-host.ymlso bootstrapping an edge host is one unrestricted play instead of acommon.yml -l edge_host+install-edge-host.ymlpair, and fixes a staleedge-1.srv.renner.unohostname (which doesn't resolve — the real host isedge-1.iot.renner.uno) plus a bogus DHCP-reservation precondition inansible/roles/edge-host/README.md. The ansible change itself checks out — role composition, exclusion ofqemu-guest-agent/monitoring-agent, and the unusedloki_endpoint/prometheus_remote_write_endpointvars all match what's actually inroles/common/tasks/main.yml, and the PR body documents a live--check --diffrun confirmingcertificates/commonproduce zero changes. However, the same stale hostname and the same superseded two-command bootstrap sequence this PR sets out to fix still exist in two other files that are part of the same procedure and weren't touched.🔴 Blocking
images/edge-host-iso/airootfs/usr/local/bin/edge-install:102-105— the installer script (baked into the ISO, run on the box, its own output is what the operator reads at the console immediately after imaging) still prints the old, now-superseded two-command sequence:This is exactly the pattern the PR's own description calls out as broken (
--limitnarrows the target list and silently skips the rest of the fleet). Sinceinstall-edge-host.ymlnow carriescertificates/commonitself, the first line here is redundant, and following it reintroduces the exact problem being fixed — a reader has no way to know the printed instructions are stale.images/edge-host-iso/README.md:3— still names the hostedge-1.srv.renner.uno, the same non-resolving hostname the PR fixes everywhere else (ansible/roles/edge-host/README.md). This file is the target of the link in the updated README's own step 2 ("Build and boot the dedicated installer image — seeimages/edge-host-iso/README.md"), so anyone following the full procedure end-to-end lands right back on the defect this PR claims to have closed.images/edge-host-iso/README.md:41-46— same stale two-command bootstrap (plays/common.yml -l edge_hostfollowed byplays/install-edge-host.yml) as inedge-installabove, for the same reason.Given the PR's stated goal is specifically "make the bootstrap self-contained and the runbook followable," these three spots leave the actual end-to-end procedure (ISO build → install → bootstrap) inconsistent with the fixed
ansible/roles/edge-host/README.md, and reintroduce both defects (#2 and #3 from the PR description) through a linked/printed document that wasn't updated.🟡 Suggestions
ansible/plays/install-edge-host.yml:30—become: trueis new; previously the play ran without it (nobecomeanywhere indocker-install/edge-hosttasks).inventory/pve_hosts.yml:45connects asansible_user=root, sobecome: truesudos from root to root — consistent with the pattern already used inplays/common.yml, and the PR body's--check --diffrun reportsok=53 changed=5 failed=0, so it evidently works against the live host. Worth a one-line comment noting why it's needed even though the connection is already root (e.g. if any future task narrows privileges), since it's non-obvious from the diff alone why a root-connection play needsbecome.ansible/plays/install-edge-host.yml:28-35— the merged play omitsignore_unreachable: true, whichplays/common.yml's base-hardening play sets. Harmless today since[edge_host]has exactly one member, but the PR's own follow-up section anticipates a second edge host soon; if that box (or edge-1) is briefly unreachable during a routine convergence run, plain Ansible behavior already skips just that host and continues for others, so this is low-value — flagging only because the field was intentionally present in the play this replaces.🟢 Nits
None beyond the above.
Tests
This is an Ansible/documentation change with no test suite in the repo for playbooks; the PR substitutes a
--check --diffrun against the liveedge-1.iot.renner.unohost, which is the right verification method here and is reported in the PR body (ok=53 changed=5 unreachable=0 failed=0, changes confined to the pre-existingedge-hostrole tasks). That's adequate for the ansible change. It does not, and can't, catch the stale references left in the ISO-side files flagged above — those need a manual grep sweep (which the PR partially did —edge-1.srv.renner.unoand "DHCP reservation" are gone fromansible/, just not fromimages/edge-host-iso/).Recommendation
Request changes — 3 blocking issue(s) (stale hostname and superseded two-command bootstrap left in
images/edge-host-iso/README.mdandimages/edge-host-iso/airootfs/usr/local/bin/edge-install).🤖 Automated review — model
CLI default(complexity: complex), commit07faf0ae69627cecff137287315a16049dc955d9.Summary
This PR folds
certificatesandcommonintoplays/install-edge-host.ymlso edge-host bootstrap is a single unrestricted run over theedge_hostgroup, and fixes the runbook's deadedge-1.srv.renner.unoreferences (should be.iot.renner.uno) and the DHCP-reservation precondition across three files (ansible/plays/install-edge-host.yml,ansible/roles/edge-host/README.md,images/edge-host-iso/README.md,images/edge-host-iso/airootfs/usr/local/bin/edge-install). I checked every factual claim against the checkout:common's tasks only branch onDebian/RedHatos_family(ansible/roles/common/tasks/main.yml:27,37) so they no-op on Arch;certificatesis an intentionally empty role (ansible/roles/certificates/tasks/main.yml);loki_endpoint/prometheus_remote_write_endpointare indeed unused bycommon(only consumed bymonitoring-agent, which isn't pulled in);edge_host's inventory entry already usesansible_user=root(ansible/inventory/pve_hosts.yml:45), making the newbecome: truea no-op as the added comment states; and the corrected hostnameedge-1.iot.renner.unomatches the actual inventory entry. All of this holds up — no blocking issues found.🟡 Suggestions
ansible/plays/install-edge-host.yml:9-27: the new header comment is quite long (19 lines) for what is now a 6-line play. Not wrong, but consider trimming to the load-bearing facts (why--limitwas structurally forced before, what's intentionally excluded) and letting the PR description carry the rest — future edits to this play risk the comment drifting out of sync with the role list.images/edge-host-iso/airootfs/usr/local/bin/edge-install:10,102-111:HOSTis still hardcoded toedge-1(pre-existing, untouched by this diff), so the new post-install instructions ("confirmssh root@${HOST}works", "add${HOST}to[edge_host]") will printedge-1even when this ISO is later used to image the second Wyse box. The PR body explicitly scopes that second host out as a follow-up, so this isn't a defect introduced here — just flagging that whoever picks up the follow-up will need to parameterizeHOST(e.g. prompt for it) before the printed instructions are correct for a second box.Tests
No automated tests exist for these Ansible plays/docs in this repo (consistent with the rest of the codebase), and none would be idiomatic here. The PR body documents manual verification instead — a
--check --diffrun of the new unrestricted play against the liveedge_hostgroup showing only pre-existing check-mode noise from theedge-hostrole, withcertificates/commoncontributing zero changes. I can't independently re-run that against the live host, but the claim is consistent with what's on disk (emptycertificatesrole,commontasks that don't match Arch'sos_family), so it's credible rather than just asserted.Recommendation
Looks reasonable — no blocking issues; comments above are optional.
🤖 Automated review — model
CLI default(complexity: complex), commitad550ecb0413f091300561484b2002520744ed5b.Summary
This PR splits the monolithic
edge-hostAnsible role into eleven single-concern roles, foldscertificates/commonintoinstall-edge-host.ymlso the bootstrap is one unrestricted run over theedge_hostgroup, and fixes the staleedge-1.srv.renner.unohostname in the README/ISO docs. The refactor itself is clean and the variable/handler split is mostly correct, but the new role order introduces a real migration hazard for the liveedge-1host, and one handler reference was left dangling by the split — both would have been invisible to the PR's--check --diffverification becausecommand/shelltasks don't actually execute under Ansible check mode.🔴 Blocking
ansible/plays/install-edge-host.yml:44-45—zwave-gatewayruns before the legacy-project teardown thatpower-meter-smlperforms, so a real run against the existingedge-1host will fail with a Docker container-name conflict. The oldedge-hostrole deployed one shared compose project at/opt/edge-host/docker-compose.ymlcontaining serviceszwave-jsandsml2mqtt(both with fixedcontainer_name:). This PR'spower-meter-sml/tasks/main.yml:17-28tears that legacy project down (docker compose -f {{ edge_legacy_compose }} down+ delete the file) — but that task lives inpower-meter-sml, which the play runs last.zwave-gateway/tasks/main.yml:78-88("Start zwave-js") runs before it and doesdocker compose up -din the new project directory (zwave_base_dir), trying to create a container also namedzwave-js. Docker enforces globally-unique container names regardless of compose project, so while the oldzwave-jscontainer (owned by the still-untorn-down legacy project) exists, thisup -dfails with a "Conflict: container name … already in use" error on stderr — which does not match the'error gathering device information'string the task'sfailed_whentolerates, so the task is marked failed and the play aborts for that host, never reachingpower-meter-sml's cleanup. The role's own default comment (power-meter-sml/defaults/main.yml:11-12) acknowledges "the fixed container_name is released for the new projects" as a prerequisite — but the retirement step needs to run before bothzwave-gatewayandpower-meter-sml, not just before the latter.The PR's stated verification (
--check --diff, "ok=53 changed=5") cannot have exercised this:ansible.builtin.command/shelltasks are not check-mode aware and are skipped (not simulated) under--check, so thedocker compose down/upsequence never actually ran during that dry run.Fix: move the legacy-project retirement (and file removal) to run before
zwave-gateway— e.g. as its own step earlier in the play, or as the first task ofzwave-gatewayitself.ansible/roles/zwave-gateway/tasks/main.yml:29— dangling handler notify. The "Extract musl libeatmydata.so" task doesnotify: Restart edge compose services, but no handler by that name exists anywhere in the repository (verified via repo-wide search) — it's a leftover from the deletedroles/edge-host/handlers/main.yml, which defined that handler.zwave-gateway/handlers/main.ymlonly definesRestart zwave-js. Today this is masked because the task is guarded bycreates: "{{ zwave_base_dir }}/libeatmydata.so"and that file already exists onedge-1(confirmed by the PR body: the extraction wasn't among the 5 reported changes), so the task reportsok, notchanged, and the notify never fires. But per the role's own comment ("Refresh by deleting the .so if the base ever bumps its Alpine major version"), the very next time that file is deleted/refreshed, this task will reportchanged, fire the notify, and Ansible will fatally error with "the requested handler 'Restart edge compose services' was not found", aborting the play.Fix:
notify: Restart zwave-jsto match the handler that actually exists in this role.🟡 Suggestions
kubernetes/bootstrap.sh:28still referencesansible/roles/edge-host, which this PR deletes. Not part of the diff, so easy to miss, but now stale alongside the other doc updates this PR made (README, ISO docs) — worth a follow-up fix toansible/docs/edge-host.md.🟢 Nits
None beyond the above.
Tests
No test suite exists for this Ansible content (consistent with the rest of the repo); the PR's own idempotence check (
--check --diffagainst the live host) is the established verification method here, but as noted above it structurally cannot catch failures incommand/shelltasks (the exact place both blocking issues live), since those don't execute under--check. A real (non-check) idempotent re-run againstedge-1, or at minimum a--checkrun withcheck_mode: falseforced on the twodocker composecommand tasks, would have caught the first issue; nothing short of an actualchangedrun of the libeatmydata extraction task would catch the second.Recommendation
Request changes — 2 blocking issues.
ad550ecb04to2d9c3ba213🤖 Automated review — model
CLI default(complexity: complex), commit2d9c3ba213b37cedf32cb0a8601a66f98d70a53c.Summary
The diff is much larger than the PR description suggests: the description
(and title) talk only about three doc/runbook defects — dropping the DHCP
reservation requirement, folding
common/certificatesintoinstall-edge-host.ymlso no--limitis needed, and fixing theedge-1.srv.renner.uno→edge-1.iot.renner.unotypo. The actual (and only)commit in the PR,
2d9c3ba, is titledrefactor(edge): split the edge-host role into one role per concernand does exactly that: it breaks the oldmonolithic
edge-hostrole into nine new roles (edge-flash-wear,edge-log-shipper,mqtt-bridge,edge-vlan,socketcan,heating-can,zwave-gateway,power-meter-sml, plus additions to the shareddocker-installrole). The decomposition itself is clean and well-commented,and the described doc/runbook fixes are all present and correct. But the
undisclosed scope is where the real risk is: two concrete bugs below would
either break the very production host (
edge-1) this PR is meant to migrate,or silently change Docker's config on several unrelated hosts.
🔴 Blocking
ansible/plays/install-edge-host.yml:44-45—zwave-gatewayruns beforepower-meter-smlretires the legacy compose project, so the live-migrationrun this PR is meant to perform will fail on a container-name conflict.
The old
edge-hostrole deployed one compose project at/opt/edge-host/docker-compose.ymlwith two fixed container names,zwave-jsandsml2mqtt(deleted in this diff, e.g. the oldansible/roles/edge-host/templates/docker-compose.yml.j2). The only taskthat stops/removes that legacy project is "Retire the legacy shared compose
project" in
ansible/roles/power-meter-sml/tasks/main.yml:17-23. But therole order in
install-edge-host.ymlrunszwave-gatewaybeforepower-meter-sml(socketcan,heating-can,zwave-gateway,power-meter-sml).zwave-gateway's "Start zwave-js" task(
ansible/roles/zwave-gateway/tasks/main.yml:78-88) runsdocker compose up -din a brand-new project directory whose compose file still declarescontainer_name: zwave-js(ansible/roles/zwave-gateway/templates/docker-compose.yml.j2:20)— the same name already held by the still-running legacy container, since
nothing has stopped it yet. Docker refuses to create a second container with
a name already in use,
docker compose up -dexits non-zero with a"Conflict… container name … already in use" message that does not
contain
'error gathering device information', so thefailed_whenguard(lines 83-85) does not swallow it — the task fails and the play stops before
power-meter-sml(which contains the only retirement logic) ever runs. On aretry the same failure repeats, because nothing changed. This is exactly the
scenario the PR's own verification target (edge-1, already provisioned
under the old role) will hit on the first real run.
The PR body's
--check --diffverification against the live host would nothave caught this:
ansible.builtin.command/shelltasks withoutcreates/removesare not executed in check mode, so none of thedocker compose up -d/docker compose downtasks actually ran during thatvalidation — consistent with the reported
changed=5covering only thegit-checkout/pip-install tasks.
Fix: run the "retire legacy compose project" step before any role tries to
claim
zwave-js/sml2mqttas container names (e.g. as its own step aheadof
zwave-gateway, or duplicated intozwave-gatewaysince it startsfirst).
ansible/roles/docker-install/tasks/main.yml:74-79— the new "Configuredocker daemon log driver" task is unconditional in a shared role, and
directly fights
netbird-proxy's owndaemon.jsonmanagement.docker-installis not edge-host-specific — it's included byplays/ install-netbird-proxy.yml,install-netbird-server.yml,install-music-assistant.yml,install-grist.yml,install-ical-proxy.yml,install-gatus.yml,install-owncloud.yml,install-authentik-ha.yml, andinstall-n8n.yml, none of which are mentioned in the PR description. Thenew task writes
/etc/docker/daemon.jsonwith{"log-driver": "journald"}(ansible/roles/docker-install/templates/docker-daemon.json.j2)on every one of those hosts and restarts docker via the
Restart dockerhandler — undisclosed scope creep on its own, but for
netbird-proxyit'san active conflict:
ansible/roles/netbird-proxy/tasks/main.yml:21-26already deploys its own full-replacement
/etc/docker/daemon.json(IPv6config,
daemon.json.j2→{"ipv6": true, "fixed-cidr-v6": …, "ip6tables": true}, nolog-driverkey) later in the same play(
plays/install-netbird-proxy.yml:26-27:docker-installthennetbird-proxy), and also defines a handler namedRestart docker(
ansible/roles/netbird-proxy/handlers/main.yml). Both tasks do a fullansible.builtin.templateoverwrite of the same file with mutuallyexclusive content, so every single run of
install-netbird-proxy.ymlleaves the two tasks alternately clobbering each other's version of
daemon.jsonand both reporting "changed" — this breaks the idempotencethis repo otherwise verifies for (per this PR's own commit-message
standard), and the IPv6 config the play depends on ends up racing with the
new log-driver config with no clear winner between runs, each hop also
restarting docker (and hence at least momentarily the netbird-proxy/relay
containers).
Fix: scope the log-driver task to edge hosts only (e.g. move it into
edge-flash-wearor gate it behind a role variable that defaults off), anddon't add unconditional host-wide file ownership to a role six other plays
already depend on without checking those consumers.
🟡 Suggestions
ansible/roles/heating-can/tasks/main.yml:117-118(blank line style aside)— no functional issue, just noting the two heating-can tasks read/checked
fine: same systemd unit names as before, so re-applying against the
already-running open3e/e3oncan services on edge-1 is a normal idempotent
restart, unlike the compose-project case above.
install-edge-host.ymlnext to thezwave-gateway/power-meter-smlentries calling out the one-time legacy-composedependency, the way the header comment already calls out the
mqtt-bridge/edge-flash-wear/socketcanordering constraints — it's theone ordering rule that isn't documented there and it's the one that's wrong.
🟢 Nits
kubernetes/bootstrap.sh:28still referencesansible/roles/edge-hostina comment; that path no longer exists after this PR (moved to
ansible/docs/edge-host.md).Tests
No automated tests cover Ansible playbooks/roles in this repo; verification
here is manual
--check --diffagainst the live host, as described in thePR body. That method is exactly why both blocking issues above weren't
caught:
command/shelltasks withoutcreates/removesdon't executeunder
--check, so none of thedocker compose up/downinvocations — whereboth bugs live — actually ran during the author's own validation. A real
(non-check) dry run against a disposable Arch VM seeded with the old
edge-hostrole's compose project (to reproduce the container-namecollision) and against one of the other
docker-installconsumers (toreproduce the
daemon.jsonfight) would have caught both.Recommendation
Request changes — 2 blocking issues.
🤖 Automated review — model
CLI default(complexity: complex), commit9cc20251fdddf2a51a21584c1fc887689dd7170b.Summary
This PR restructures the monolithic
edge-hostansible role into ten focused roles (edge-flash-wear,edge-log-shipper,mqtt-bridge,edge-vlan,socketcan,heating-can,zwave-gateway,power-meter-sml, pluscertificates/commonfolded into the play), fixes the bootstrap runbook (drops the DHCP-reservation precondition, removes the--limittwo-command dance, corrects the deadedge-1.srv.renner.unohostname), and renamesroles/edge-host/README.mdtoansible/docs/edge-host.md. I traced the role split, the new play'spre_taskslegacy-compose migration, and cross-checked the claims in the PR body (common role's OS branching,loki_endpoint/prometheus_remote_write_endpointnot being needed, no leftoveredge-1.srvorroles/edge-hostreferences) — all check out. There is one blocking issue: a task added to the shareddocker-installrole has fleet-wide blast radius that the PR doesn't disclose.🔴 Blocking
ansible/roles/docker-install/tasks/main.yml:71-79— the new "Configure docker daemon log driver" task was added directly to the shareddocker-installrole, not scoped toedge_host. This role is also used byplays/install-netbird-server.yml,install-n8n.yml,install-authentik-ha.yml,install-owncloud.yml,install-gatus.yml,install-ical-proxy.yml,install-grist.yml,install-music-assistant.yml, andinstall-netbird-proxy.yml. None of those hosts currently have/etc/docker/daemon.json(the template anddocker_log_driverdefault are both new files introduced by this PR), so the next time any of those plays is run, this task will create the file, reportchanged, and fire theRestart dockerhandler (ansible/roles/docker-install/handlers/main.yml) — restarting the Docker daemon and every container on that host (authentik, n8n, owncloud, grist, netbird, gatus, music-assistant, ical-proxy). That's an unannounced availability blip on unrelated production services, triggered by a PR framed entirely around edge-host bootstrapping.The task's own comment — "the services that use this host run with
network_mode: host" — is true for the edge-host services but false for most of the other consumers of this role, which is a good sign the change was written with only edge-1 in mind and landed in the wrong file.Fix: move this task (and the
docker_log_driverdefault /docker-daemon.json.j2template) into a role that's actually edge-host-scoped (e.g.edge-flash-wear, which is already edge-host-only and already owns the flash-wear/journald rationale), or gate the task withwhen: "'edge_host' in group_names"if it must stay indocker-install.🟡 Suggestions
ansible/plays/install-edge-host.ymlplay-levelvars.edge_base_dirandedge_legacy_composeduplicate defaults independently re-declared inheating-can,power-meter-sml, andzwave-gatewayrole defaults (all/opt/edge-host). Harmless today since play vars win over role defaults, but worth a comment noting the play-level copy is intentionally kept in sync forpre_tasks, in case a future edit changes one without the other.🟢 Nits
None beyond the above.
Tests
This is an ansible-only infra repo with no automated test suite; verification is via
--check --diffagainst the live host, which the PR body documents (idempotent, only pre-existing check-mode-visible changes inside the roles that were already there before the split). That's consistent with how this repo verifies other plays. The one thing that verification pass could not have caught is thedocker-installblast-radius issue above, since it only exercisedinstall-edge-host.yml— a--check --diffrun of one of the other nine plays that consumedocker-install(e.g.install-gatus.yml) would have surfaced the new daemon.json task and its handler firing.Recommendation
Request changes — 1 blocking issue.
🤖 Automated review — model
CLI default(complexity: complex), commit6cfa63f6f3e294cb14d3969f84d49bd8ff84fc45.Summary
This PR does two things bundled together: it splits the monolithic
edge-hostrole into eight focused roles (
edge-flash-wear,edge-log-shipper,edge-vlan,heating-can,mqtt-bridge,power-meter-sml,socketcan,zwave-gateway), and it makesinstall-edge-host.ymlself-contained(carrying
certificates+commonitself) so the bootstrap runbook no longerneeds a separate
common.yml -l edge_hoststep, plus a hostname/doc fix(
edge-1.srv.renner.uno→edge-1.iot.renner.uno). The PR body onlydescribes the latter, smaller set of changes, but the diff is overwhelmingly
the role split — that's fine (it's a legitimate mechanical decomposition and
mostly renames), just worth noting the description undersells the diff's
actual size. The decomposition itself is careful: dependency order in the
play is correct (mqtt-bridge → its clients, socketcan → heating-can,
edge-flash-wear → docker-install), and the migration off the old shared
compose project (
pre_tasksininstall-edge-host.yml) correctly removesboth the legacy
docker-compose.ymland the orphanedzwavejs.envat the oldpath (
{{ edge_base_dir }}/zwavejs.env, now served fromzwave-js/zwavejs.env). No blocking issues found in the mechanics of thesplit itself. One design gap is worth fixing before/soon after merge (see
below), plus a couple of typos.
🟡 Suggestions
ansible/roles/monitoring-agentis not actually excluded fromedge_host, contradicting the PR's stated design. The PR body says monitoring-agent is "Debian-oriented" and intentionally not carried intoinstall-edge-host.ymlbecause edge hosts ship logs viaedge-alloyinstead. Butplays/common.yml's second play (ansible/plays/common.yml:8,hosts: all:!pve:!pbs:!proxmox_noansible:!proxmox_nodes:!pve_cluster) still includesedge_hostin scope, andansible/roles/monitoring-agent/tasks/main.ymlis gated only bymonitoring_agent_enabled(defaulttrueinansible/roles/monitoring-agent/defaults/main.yml:5), not byos_family— its Alloy-binary-download path has no Debian/RedHat guard.ansible/inventory/group_vars/edge_host/vars.ymlsets nomonitoring_agent_enabled: falseoverride. So an unrestricted future run ofplays/common.ymlwould still deploy a second Grafana Alloy onto edge-1: same self-metrics port (alloy_http_listen_port: 12345in monitoring-agent vs.edge_alloy_http_port: 12345inansible/roles/edge-log-shipper/defaults/main.yml) and a push target (loki.srv.renner.uno:3100) that the PR body itself says is firewalled off the IoT VLAN. This predates this PR (edge_host was already in that host pattern), and the trend elsewhere in the repo (authentik, netbird, etc. all carrycommonvia their own dedicated plays rather than relying oncommon.ymlunrestricted) makes it less likely to actually fire — but since this PR is explicitly the place that decided "not carried over," it's the natural place to also make that durable, e.g.monitoring_agent_enabled: falseandqemu_guest_agent-equivalent inansible/inventory/group_vars/edge_host/vars.yml, or excludingedge_hostfromplays/common.yml's host pattern.ansible/plays/install-edge-host.yml:123-127— the legacy-compose teardown (docker compose -f "{{ edge_legacy_compose }}" down) usesfailed_when: false, which blanket-swallows any failure, not just "already torn down." Since the task only runs whenedge_legacy_composeexists (guarded byremoves:), a genuine failure here (e.g. daemon hiccup) would be silently ignored, the very next task still deletes the compose file, and the old fixed-name containers (zwave-js/sml2mqtt) could be left running — causing the laterdocker compose up -dinzwave-gateway/power-meter-smlto fail with a "container name already in use" error that doesn't match the'error gathering device information'string those tasks tolerate, so the play would abort with a confusing error whose actual cause (the swalloweddownfailure) is invisible. The PR body already acknowledges--checkcan't exercise this path; worth at least narrowingfailed_whento the "already down" case rather than everything.🟢 Nits
ansible/roles/edge-log-shipper/tasks/main.yml:5-6andansible/roles/heating-can/tasks/main.yml:7— missing apostrophes: "docker s journald driver", "Alloy s storage/WAL", "heat pump s own energy management".ansible/roles/heating-can/tasks/main.ymlhas a stray double blank line between the "Enable open3e" and "E3onCAN" sections (around line 55).Tests
This repo has no automated test harness for Ansible roles (confirmed — no molecule/pytest setup under
ansible/); the established convention here isansible-playbook ... --check --diffagainst the live host, which the PR does document (idempotent,ok=53 changed=5, changes isolated to pre-existingheating-canvenv/git tasks). That's consistent with how this repo verifies infrastructure changes and is adequate for this kind of change. The one thing--checkstructurally cannot exercise — the legacy-compose teardown inpre_tasks— is called out by the PR itself as unverifiable in check mode; that's an honest and accurate limitation, not a gap in what was checked.Recommendation
Looks reasonable — no blocking issues; comments above are optional but the monitoring-agent gap is worth a quick follow-up to make the "not carried over" decision actually durable.