build(heating-can): image for the CAN readers, replacing pip-into-venv #476

Merged

k.renner merged 7 commits from heating-can-image into master 2026-08-10 19:04:50 +00:00

Conversation 6 Commits 7 Files Changed 11

+369 -184

claude-bot commented 2026-08-09 06:53:18 +00:00

Member

Copy Link

Copy Source

One image containing open3e (Vitocal heat pump, Vitocharge VX3) and E3onCAN (E380 grid energy meter), both pinned in the Containerfile.

Why

Both are pip-installed into venvs on edge-1, open3e straight from an unpinned git URL. pip cannot compare an installed VCS dist against a requested one without fetching it first, so pip install re-clones and rebuilds on every Ansible converge — reporting changed, firing the restart handler, and putting a gap in heating telemetry each time.

Pinning the SHA does not fix that. Verified directly on the host:

pip install --dry-run "git+...@6553c104"
  -> Cloning ... Resolved ... to commit 6553c104
     Installing build dependencies: started

A tagged image does: docker compose up -d on an unchanged tag is a genuine no-op.

Auditability

This is the stronger reason. The weekly update-audit currently has to be told about two special cases in prose — open3e publishes no tags at all, and E3onCAN is pinned to an untagged commit between releases. A date-tagged image in the Gitea registry is a normal artifact the existing tooling already understands, same shape as the ci-* images.

One image, two services

They read the same physical installation over the same bus and are versioned together, so they're one artifact. But they'll run as two containers from it, so either can crash and restart independently without a supervisor as PID 1.

Incidental fix

e3oncan.service sets PYTHONUNBUFFERED=1; open3e.service does not. That's why open3e's stdout was block-buffered and effectively never reached the journal or Loki — a long-standing blind spot. The image sets it for both.

Runtime notes (for the follow-up, not this PR)

Needs network_mode: host: SocketCAN interfaces live in the host network namespace, so can0 is only reachable that way — and it's also how the local broker on 127.0.0.1:1883 is reached. CAN_RAW needs no capabilities, so both run unprivileged. This is the same shape zwave-js and sml2mqtt already use on this host.

Deliberately artifact-only

The heating-can role still deploys venvs and systemd units. Moving it to a compose project is a separate change, once this image exists and has been pulled and run once.

Rewriting the role that drives the heat pump against an image that has never been built would be the wrong order — and the cutover has real hazards worth handling on their own: the old units must be stopped and masked in the same run, or two open3e instances would issue UDS requests on one CAN bus.

Depends on

#475 registers both services with the update audit. Once this image lands and the role moves to it, that audit entry should be rewritten to track the image tag and the Containerfile pins instead of the Ansible variables.

🤖 Generated with Claude Code

One image containing **open3e** (Vitocal heat pump, Vitocharge VX3) and **E3onCAN** (E380 grid energy meter), both pinned in the Containerfile. ## Why Both are pip-installed into venvs on edge-1, open3e straight from an **unpinned** git URL. pip cannot compare an installed VCS dist against a requested one without fetching it first, so `pip install` re-clones and rebuilds on **every Ansible converge** — reporting `changed`, firing the restart handler, and putting a gap in heating telemetry each time. Pinning the SHA does **not** fix that. Verified directly on the host: ``` pip install --dry-run "git+...@6553c104" -> Cloning ... Resolved ... to commit 6553c104 Installing build dependencies: started ``` A tagged image does: `docker compose up -d` on an unchanged tag is a genuine no-op. ## Auditability This is the stronger reason. The weekly `update-audit` currently has to be told about two special cases in prose — open3e publishes **no tags at all**, and E3onCAN is pinned to an **untagged commit between releases**. A date-tagged image in the Gitea registry is a normal artifact the existing tooling already understands, same shape as the `ci-*` images. ## One image, two services They read the same physical installation over the same bus and are versioned together, so they're one artifact. But they'll run as **two containers** from it, so either can crash and restart independently without a supervisor as PID 1. ## Incidental fix `e3oncan.service` sets `PYTHONUNBUFFERED=1`; `open3e.service` does not. That's why open3e's stdout was block-buffered and effectively never reached the journal or Loki — a long-standing blind spot. The image sets it for both. ## Runtime notes (for the follow-up, not this PR) Needs `network_mode: host`: SocketCAN interfaces live in the host network namespace, so `can0` is only reachable that way — and it's also how the local broker on `127.0.0.1:1883` is reached. CAN_RAW needs no capabilities, so both run unprivileged. This is the same shape `zwave-js` and `sml2mqtt` already use on this host. ## Deliberately artifact-only The `heating-can` role still deploys venvs and systemd units. Moving it to a compose project is a **separate change**, once this image exists and has been pulled and run once. Rewriting the role that drives the heat pump against an image that has never been built would be the wrong order — and the cutover has real hazards worth handling on their own: the old units must be stopped and masked in the same run, or two open3e instances would issue UDS requests on one CAN bus. ## Depends on #475 registers both services with the update audit. Once this image lands and the role moves to it, that audit entry should be rewritten to track the image tag and the Containerfile pins instead of the Ansible variables. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-08-09 06:53:18 +00:00

build(heating-can): image for the CAN readers, replacing pip-into-venv ... 0e4d516722

Builds one image containing open3e (Vitocal heat pump, Vitocharge VX3) and
E3onCAN (E380 grid energy meter), both pinned in the Containerfile.

Why: both were pip-installed into venvs on edge-1, open3e straight from an
unpinned git URL. pip cannot compare an installed VCS dist against a
requested one without fetching it, so `pip install` re-cloned and rebuilt on
every Ansible converge -- reporting changed, firing the restart handler, and
putting a gap in heating telemetry each time. Pinning the SHA does not fix
that; verified directly on the host that pip still clones and rebuilds a
pinned VCS requirement. A tagged image does: `docker compose up -d` on an
unchanged tag is a genuine no-op.

It also gives the weekly update-audit a normal artifact instead of two
awkward special cases. open3e publishes no tags at all, and E3onCAN is
pinned to an untagged commit sitting between its newest tag and HEAD -- both
of which the audit currently has to be told about in prose.

One image, two services. They read the same physical installation over the
same bus and are versioned together, so they are one artifact; but they will
run as two containers from it, so either can crash and restart on its own
without a supervisor as PID 1.

Fixes a long-standing blind spot in passing: e3oncan.service set
PYTHONUNBUFFERED=1 and open3e.service did not, so open3e's stdout was
block-buffered and effectively never reached the journal or Loki. The image
sets it for both.

Build follows the existing kaniko pattern exactly -- path-scoped `when:` so
only this image rebuilds, date tag plus latest, pushed to the Gitea registry
alongside the ci-* images.

Deliberately artifact-only. The heating-can role still deploys venvs and
systemd units; moving it to a compose project is a separate change, once
this image exists and has been pulled and run once. Rewriting the role that
drives the heat pump against an image that has never been built would be the
wrong order.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot reviewed 2026-08-09 06:57:09 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 0e4d5167221cb371dc674164a2b0b587a75ab365.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

Adds a new build-heating-can kaniko step (mirroring every other step in .woodpecker/build-ci-images.yml exactly) and a new ci/images/heating-can/Containerfile that bundles open3e and E3onCAN, both pinned to full 40-char commit SHAs, into one date-tagged image. The pipeline wiring is a faithful copy of the existing pattern and looks correct. The Containerfile is well-reasoned and well-commented, but one of its own comments describes a build step that isn't actually implemented — see below. No blocking issues.

🟡 Suggestions

• ci/images/heating-can/Containerfile:87-91 (comment) vs :69-114 (implementation): the comment says "git is a build-time need only … it is dropped from the final image via the venv copy below." This is a single-stage build (FROM python:3.13-slim once, no second FROM, no COPY --from=) — there is no "venv copy" that strips git out. ci/images/cni-plugins/Containerfile in this same repo already uses a real multi-stage COPY --from=fetch pattern, so the tooling for this exists; it just wasn't applied here. As shipped, git, ca-certificates, and their apt dependency chain (perl, etc.) remain installed in the final runtime image that's pulled to edge-1 — unnecessary size and attack surface for a bug-fix PR whose stated goal is a lean, auditable artifact. Either implement the multi-stage copy the comment describes, or correct the comment to match reality.
• ci/images/heating-can/Containerfile:112-114: WORKDIR /opt and /opt/e3oncan (E3onCAN's checkout, where it presumably runs from) are owned by root since they're created before USER open3e is set — the open3e user can read/execute but not write there. If either service writes any local state (cache, lock file, etc.) next to its source rather than to stdout/MQTT, it will fail with a permission error at runtime. I couldn't check this against E3onCAN's or open3e's actual source (not vendored, no network access from this review), so treat this as unconfirmed — worth a quick smoke test (docker run --rm <image> <command>) before wiring it into the compose file in the follow-up PR.

🟢 Nits

• ci/images/heating-can/Containerfile:112: the shared system user for both services is named open3e, even though it also runs E3onCAN. A neutral name (e.g. heating-can) would read less confusingly, though this is purely cosmetic since it's a private service account.

Tests

No automated tests apply here — this is infrastructure-as-code (a Containerfile + CI pipeline step), consistent with how every other ci/images/* build is handled in this repo (no test step, image-scans run separately via the scheduled security-scan.yml). Note the new image won't be picked up by security-scan.yml's image-scans step until it's referenced from an ansible/*.yml or compose template (that step greps image: out of those files) — expected and fine given this PR is deliberately artifact-only, but worth remembering for the follow-up.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `0e4d5167221cb371dc674164a2b0b587a75ab365`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Adds a new `build-heating-can` kaniko step (mirroring every other step in `.woodpecker/build-ci-images.yml` exactly) and a new `ci/images/heating-can/Containerfile` that bundles open3e and E3onCAN, both pinned to full 40-char commit SHAs, into one date-tagged image. The pipeline wiring is a faithful copy of the existing pattern and looks correct. The Containerfile is well-reasoned and well-commented, but one of its own comments describes a build step that isn't actually implemented — see below. No blocking issues. ## 🟡 Suggestions - `ci/images/heating-can/Containerfile:87-91` (comment) vs `:69-114` (implementation): the comment says "git is a build-time need only … it is dropped from the final image via the venv copy below." This is a single-stage build (`FROM python:3.13-slim` once, no second `FROM`, no `COPY --from=`) — there is no "venv copy" that strips git out. `ci/images/cni-plugins/Containerfile` in this same repo already uses a real multi-stage `COPY --from=fetch` pattern, so the tooling for this exists; it just wasn't applied here. As shipped, `git`, `ca-certificates`, and their apt dependency chain (perl, etc.) remain installed in the final runtime image that's pulled to edge-1 — unnecessary size and attack surface for a bug-fix PR whose stated goal is a lean, auditable artifact. Either implement the multi-stage copy the comment describes, or correct the comment to match reality. - `ci/images/heating-can/Containerfile:112-114`: `WORKDIR /opt` and `/opt/e3oncan` (E3onCAN's checkout, where it presumably runs from) are owned by `root` since they're created before `USER open3e` is set — the `open3e` user can read/execute but not write there. If either service writes any local state (cache, lock file, etc.) next to its source rather than to stdout/MQTT, it will fail with a permission error at runtime. I couldn't check this against E3onCAN's or open3e's actual source (not vendored, no network access from this review), so treat this as unconfirmed — worth a quick smoke test (`docker run --rm <image> <command>`) before wiring it into the compose file in the follow-up PR. ## 🟢 Nits - `ci/images/heating-can/Containerfile:112`: the shared system user for both services is named `open3e`, even though it also runs E3onCAN. A neutral name (e.g. `heating-can`) would read less confusingly, though this is purely cosmetic since it's a private service account. ## Tests No automated tests apply here — this is infrastructure-as-code (a Containerfile + CI pipeline step), consistent with how every other `ci/images/*` build is handled in this repo (no test step, image-scans run separately via the scheduled `security-scan.yml`). Note the new image won't be picked up by `security-scan.yml`'s `image-scans` step until it's referenced from an `ansible/*.yml` or compose template (that step greps `image:` out of those files) — expected and fine given this PR is deliberately artifact-only, but worth remembering for the follow-up. ## Recommendation Looks reasonable — no blocking issues; comments above are optional. <!-- pr-review:sha=0e4d5167221cb371dc674164a2b0b587a75ab365 -->

claude-bot added 1 commit 2026-08-09 07:54:21 +00:00

build(heating-can): make both services first-class commands in the image ... 4df1018e05

The image had no ENTRYPOINT and no CMD, with a comment claiming that was
deliberate because the commands are rendered by Ansible. That was
rationalising a defect.

An image with nothing runnable is not a self-contained artifact. The
knowledge of how to START each service leaked into Ansible, and the two were
not even symmetric: open3e installs a console script via pip, while E3onCAN
is an unpackaged script in a checkout, so the compose file would have had to
carry `python /opt/e3oncan/src/E3onCANcollect.py` -- an implementation detail
of this image's internal layout, in a file that has no business knowing it.
Move the checkout and every caller breaks.

E3onCAN now gets a wrapper on PATH, so both services are invoked the same
way and `docker run <image> e3oncan --help` works.

CMD prints what the image provides and exits 64 (EX_USAGE) rather than
silently doing nothing, so `docker run <image>` is self-describing. Which
service to run, and its arguments, stays the caller's choice -- the compose
file renders those, because the datapoint list and broker settings live
there.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot requested changes 2026-08-09 07:59:27 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model claude-sonnet-5 (complexity: normal), commit 4df1018e05763710c4939f3eb2d9eb38cd2a1ea9.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

This PR adds a new heating-can Containerfile (open3e + E3onCAN, pinned to specific commits) and wires it into the existing build-ci-images.yml kaniko pipeline, following the exact same step pattern as the other images in that file. The CI wiring itself is solid and consistent with the rest of the file. There is one concrete blocking issue in the Containerfile: the final USER/WORKDIR combination leaves the process running as a non-root user in a directory it cannot write to, which — per this repo's own documentation of the same library's behavior — is where it needs to write a log file.

🔴 Blocking

• ci/images/heating-can/Containerfile:124-126 — WORKDIR /opt is set after USER open3e, and nothing in the Containerfile chowns /opt to the open3e user first. /opt is created/touched only by earlier RUN steps that execute as root (python -m venv /opt/venv, the git clone into /opt/e3oncan), so it keeps the standard root:root, 0755 ownership from the base Debian image — not writable by a non-root user. This matters because ansible/roles/heating-can/tasks/main.yml:33-34 explicitly documents, for the same open3e/udsoncan dependency this image installs, that "udsoncan writes its log file into the working directory," which is exactly why the existing venv-based deploy gives open3e a directory it owns (ansible/roles/heating-can/tasks/main.yml:35-41). In this image, open3e will try to create that log file in /opt as the unprivileged open3e user and get a permission error. Depending on how udsoncan handles that failure, this either silently loses logging again (the exact blind spot this PR says it's fixing) or crashes the process at startup. Fix: RUN mkdir -p /opt/open3e && chown open3e:open3e /opt/open3e (or similar) before USER open3e, and WORKDIR into that directory instead of the shared, root-owned /opt.

🟡 Suggestions

• ci/images/heating-can/Containerfile:87-88 vs. :26 — the comment above the apt-get install git ... step says git "is dropped from the final image via the venv copy below," but this is a single-stage build (FROM python:3.13-slim only, no COPY --from=). Nothing removes git/ca-certificates from the final image — they ship in the runtime artifact along with their transitive dependencies. Either implement the multi-stage copy the comment describes (smaller, git-free runtime image, matching the stated intent) or correct the comment so it doesn't claim behavior the Containerfile doesn't have.

🟢 Nits

• ci/images/heating-can/Containerfile:100 clones the full E3onCAN repository before checking out a single pinned commit (git clone with full history, then git checkout <sha>). A shallow fetch of just that commit would be cheaper at build time; purely a build-time efficiency nit, no functional impact.

Tests

No automated build/lint check exists for this Containerfile (no hadolint or docker build smoke test in this repo's CI, for this image or any of the other ci/images/* ones), so this is consistent with repo convention rather than a gap introduced by this PR. That said, a minimal smoke test (docker build … && docker run --rm --user open3e <image> sh -c 'touch test'-style check against the runtime WORKDIR) would have caught the permission issue above before merge.

Recommendation

Request changes — 1 blocking issue.

🤖 **Automated review** — model `claude-sonnet-5` (complexity: normal), commit `4df1018e05763710c4939f3eb2d9eb38cd2a1ea9`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR adds a new `heating-can` Containerfile (open3e + E3onCAN, pinned to specific commits) and wires it into the existing `build-ci-images.yml` kaniko pipeline, following the exact same step pattern as the other images in that file. The CI wiring itself is solid and consistent with the rest of the file. There is one concrete blocking issue in the Containerfile: the final `USER`/`WORKDIR` combination leaves the process running as a non-root user in a directory it cannot write to, which — per this repo's own documentation of the same library's behavior — is where it needs to write a log file. ## 🔴 Blocking - **`ci/images/heating-can/Containerfile:124-126`** — `WORKDIR /opt` is set *after* `USER open3e`, and nothing in the Containerfile `chown`s `/opt` to the `open3e` user first. `/opt` is created/touched only by earlier `RUN` steps that execute as root (`python -m venv /opt/venv`, the `git clone` into `/opt/e3oncan`), so it keeps the standard root:root, `0755` ownership from the base Debian image — not writable by a non-root user. This matters because `ansible/roles/heating-can/tasks/main.yml:33-34` explicitly documents, for the *same* open3e/udsoncan dependency this image installs, that "udsoncan writes its log file into the working directory," which is exactly why the existing venv-based deploy gives `open3e` a directory it owns (`ansible/roles/heating-can/tasks/main.yml:35-41`). In this image, `open3e` will try to create that log file in `/opt` as the unprivileged `open3e` user and get a permission error. Depending on how udsoncan handles that failure, this either silently loses logging again (the exact blind spot this PR says it's fixing) or crashes the process at startup. Fix: `RUN mkdir -p /opt/open3e && chown open3e:open3e /opt/open3e` (or similar) before `USER open3e`, and `WORKDIR` into that directory instead of the shared, root-owned `/opt`. ## 🟡 Suggestions - **`ci/images/heating-can/Containerfile:87-88` vs. `:26`** — the comment above the `apt-get install git ...` step says git "is dropped from the final image via the venv copy below," but this is a single-stage build (`FROM python:3.13-slim` only, no `COPY --from=`). Nothing removes `git`/`ca-certificates` from the final image — they ship in the runtime artifact along with their transitive dependencies. Either implement the multi-stage copy the comment describes (smaller, git-free runtime image, matching the stated intent) or correct the comment so it doesn't claim behavior the Containerfile doesn't have. ## 🟢 Nits - `ci/images/heating-can/Containerfile:100` clones the full E3onCAN repository before checking out a single pinned commit (`git clone` with full history, then `git checkout <sha>`). A shallow fetch of just that commit would be cheaper at build time; purely a build-time efficiency nit, no functional impact. ## Tests No automated build/lint check exists for this Containerfile (no hadolint or `docker build` smoke test in this repo's CI, for this image or any of the other `ci/images/*` ones), so this is consistent with repo convention rather than a gap introduced by this PR. That said, a minimal smoke test (`docker build … && docker run --rm --user open3e <image> sh -c 'touch test'`-style check against the runtime `WORKDIR`) would have caught the permission issue above before merge. ## Recommendation Request changes — 1 blocking issue. <!-- pr-review:sha=4df1018e05763710c4939f3eb2d9eb38cd2a1ea9 -->

claude-bot added 1 commit 2026-08-09 07:59:47 +00:00

refactor(heating-can): run the image, install nothing on the node ... 56a9859e50

Nothing is installed on the host any more. The image is built in CI with
both sources pinned, published to the registry, and edge-1 pulls and runs
it. That is the whole point of building it -- installation happens at build
time, once, not on every converge.

Removed from the node: both venvs, the E3onCAN checkout, and the two systemd
units. Kept and still rendered: devices.json, the vx3 datapoint overlay and
the broker credentials, now bind-mounted into the containers rather than
read from a venv working directory.

Two services from one image, as before: open3e drives the Vitocal/VX3 over
UDS, e3oncan listens passively for the E380 meter. Separate containers so
either can crash and restart without a supervisor as PID 1.

The migration stops AND masks the old units before the containers start.
Masked rather than disabled because two open3e instances issuing UDS
requests on one CAN bus is the failure mode to avoid, and a stray
`systemctl start` or leftover dependency should not be able to bring the
native one back alongside the container.

No missing-device tolerance here, unlike the zwave and meter roles: can0 is
this host's purpose. If it is absent the heating telemetry is down and the
run should say so rather than pass quietly.

heating_can_image replaces edge_open3e_pkg / edge_e3oncan_repo /
edge_e3oncan_version. The source pins move into the Containerfile, so what
is deployed here is a tag -- and `docker compose up -d` on an unchanged tag
is a genuine no-op, which is what removes the restart-per-converge that
pinning the pip URL could not.

Verification is limited and worth stating: plays/install-heating-can.yml
lives in #475 and is not on this branch, so only the master-side play
syntax-checks here. A --check run cannot exercise this at all -- command
tasks are skipped under check mode, so neither the pull nor the compose up
would run. It cannot be meaningfully verified until the image has been built
and #475 is merged.

Depends on #475 (the capability-split playbooks, and the MQTT contract
moving to group_vars). This branch is cut from master, so heating-can's
defaults will conflict there; #475 should merge first and this be rebased
onto it.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot requested changes 2026-08-09 08:06:00 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 56a9859e501e8ccf09ea408268518f07b0e62ecf.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

This PR replaces the pip-into-venv install of open3e + E3onCAN on edge-1 with a single CI-built image (ci/images/heating-can/Containerfile) run as two docker compose services, and rewrites the heating-can Ansible role to stop/mask the native systemd units and deploy the compose project instead. The migration story (stop-then-remove native units, keep config bind-mounted, tagged image for a sane update-audit story) is well thought out and well documented. However, the compose rewrite introduces two concrete defects that would break or weaken the deployment as written.

🔴 Blocking

• ansible/roles/heating-can/defaults/main.yml:39 vs templates/docker-compose.yml.j2:342-343 — vx3 overlay path doesn't exist inside the container. edge_open3e_devices.vx3.dpList is still /opt/edge-host/open3e/vx3plus.py (untouched by this diff — it's host-native path from the old venv layout). That value is serialized verbatim into devices.json (tasks/main.yml:72-77, "Deploy open3e devices.json"), which is bind-mounted into the container at /config/devices.json. But the overlay file itself is now mounted at /config/vx3plus.py (docker-compose.yml.j2:342-343), and nothing in the image or compose file creates /opt/edge-host/open3e/. open3e will look for the dpList override at a path that doesn't exist in the container, so the battery-module DID overlay (the entire point of files/open3e-vx3plus.py and the "Deploy open3e vx3 datapoint overlay" task) silently fails to load. dpList needs to read /config/vx3plus.py to match the new mount.

• ansible/roles/heating-can/templates/docker-compose.yml.j2:352-353 and :383-386 — MQTT broker credential is baked in plaintext into a world-readable file. The open3e command passes -muser "{{ edge_open3e_mqtt_user }}:{{ edge_open3e_mqtt_password }}", and the e3oncan command passes -muser/-mpass with the password literal. These are rendered directly by Jinja into docker-compose.yml, which is deployed with mode: "0644" (tasks/main.yml:94-99) — world-readable, and persists on disk. This is a real regression from the design the role already ships: templates/open3e.env.j2 still exists, is deployed at mode: "0600", and its own comment says the whole point of OPEN3E_MUSER is "so the open3e unit can pass it as a single -muser argument without exposing it in the unit file." The compose command no longer references ${OPEN3E_MUSER}/${OPEN3E_MQTT_PASSWORD} at all — heating-can.env is deployed but unused (it's listed under env_file: but nothing in either command: reads from the environment), and the secret ends up in the 0644 file instead of only the 0600 one. It's also then visible in docker inspect/docker compose ps and ps aux for anyone who can reach the docker socket or the host process list — the same argv-exposure the old systemd design specifically avoided at the unit-file level. Fix: have the commands read the password from the environment (e.g. sh -c wrapper reading $OPEN3E_MUSER, or open3e/E3onCAN's own env-var support if any) instead of interpolating it via Jinja into the compose file.

🟡 Suggestions

• Likely permission failure writing udsoncan's log file (ci/images/heating-can/Containerfile:571-574). The old tasks/main.yml (removed by this diff) had the comment "Owned by the service user: udsoncan writes its log file into the working directory," and correspondingly created {{ edge_base_dir }}/open3e with owner: open3e, group: open3e. The new image sets WORKDIR /opt after USER open3e, but /opt is created by the base image as root-owned (default python:3.13-slim permissions), and nothing chowns it or bind-mounts a writable volume there. If udsoncan really does write a log into its CWD as the old comment states, the open3e container will hit a permission error on startup under the unprivileged open3e user. I could not run the container to confirm this — worth checking directly (docker run --rm -u open3e <image> open3e ... or grepping the udsoncan source for where it opens its log file) before this ships, since if true it'll break the primary open3e service, not just the overlay.

• tasks/main.yml:37-56 — masking is undone by the very next task. "Stop and mask the native CAN services" masks open3e.service/e3oncan.service (which creates a symlink to /dev/null at /etc/systemd/system/<name>.service), but "Remove the native unit files" immediately deletes that exact path. The end state (no unit file at all) still prevents systemctl start, so this isn't functionally broken, but the mask step's stated purpose in the comment ("masked so a stray systemctl start ... cannot bring them back") doesn't actually persist — it's overwritten one task later. Either drop the masked: true (removal already achieves the goal) or reorder so removal doesn't clobber the mask.

• tasks/main.yml:106-118 — redundant recreate on first run. "Start heating-can services" runs docker compose up -d unconditionally after the compose file/env/config tasks, and any of those tasks notifies the Restart heating-can handler (docker compose up -d --force-recreate) which fires at end-of-play. On a config change (including the very first run) containers get started once by the task and then immediately force-recreated by the handler. Not incorrect, just a wasted extra container churn each time config changes.

🟢 Nits

• ci/images/heating-can/Containerfile:538-539 repeats the OPEN3E_REF/E3ONCAN_REF ARG declarations in the final stage purely to populate the LABELs — correct (ARGs don't cross FROM boundaries) but easy to let drift from the build-stage values if someone edits one and not the other; a comment noting they must stay in sync would help.

Tests

No automated coverage exists for this role (none existed before this PR either, and there's no ansible-lint/molecule/syntax-check step in .woodpecker/), so this is consistent with repo convention. Given that, the two blocking issues above are exactly the kind of thing that would only surface at deploy time on real hardware — worth a manual docker compose config / dry run against the rendered templates, and ideally a one-off manual test on edge-1 (or a scratch host) before merging, particularly to check the vx3 dpList path and whether open3e actually starts cleanly as the unprivileged user.

Recommendation

Request changes — 2 blocking issues.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `56a9859e501e8ccf09ea408268518f07b0e62ecf`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR replaces the pip-into-venv install of open3e + E3onCAN on edge-1 with a single CI-built image (`ci/images/heating-can/Containerfile`) run as two `docker compose` services, and rewrites the `heating-can` Ansible role to stop/mask the native systemd units and deploy the compose project instead. The migration story (stop-then-remove native units, keep config bind-mounted, tagged image for a sane update-audit story) is well thought out and well documented. However, the compose rewrite introduces two concrete defects that would break or weaken the deployment as written. ## 🔴 Blocking - **`ansible/roles/heating-can/defaults/main.yml:39` vs `templates/docker-compose.yml.j2:342-343` — vx3 overlay path doesn't exist inside the container.** `edge_open3e_devices.vx3.dpList` is still `/opt/edge-host/open3e/vx3plus.py` (untouched by this diff — it's host-native path from the old venv layout). That value is serialized verbatim into `devices.json` (`tasks/main.yml:72-77`, "Deploy open3e devices.json"), which is bind-mounted into the container at `/config/devices.json`. But the overlay file itself is now mounted at `/config/vx3plus.py` (`docker-compose.yml.j2:342-343`), and nothing in the image or compose file creates `/opt/edge-host/open3e/`. open3e will look for the dpList override at a path that doesn't exist in the container, so the battery-module DID overlay (the entire point of `files/open3e-vx3plus.py` and the "Deploy open3e vx3 datapoint overlay" task) silently fails to load. `dpList` needs to read `/config/vx3plus.py` to match the new mount. - **`ansible/roles/heating-can/templates/docker-compose.yml.j2:352-353` and `:383-386` — MQTT broker credential is baked in plaintext into a world-readable file.** The open3e command passes `-muser "{{ edge_open3e_mqtt_user }}:{{ edge_open3e_mqtt_password }}"`, and the e3oncan command passes `-muser`/`-mpass` with the password literal. These are rendered directly by Jinja into `docker-compose.yml`, which is deployed with `mode: "0644"` (`tasks/main.yml:94-99`) — world-readable, and persists on disk. This is a real regression from the design the role already ships: `templates/open3e.env.j2` still exists, is deployed at `mode: "0600"`, and its own comment says the whole point of `OPEN3E_MUSER` is "so the open3e unit can pass it as a single -muser argument **without exposing it in the unit file**." The compose command no longer references `${OPEN3E_MUSER}`/`${OPEN3E_MQTT_PASSWORD}` at all — `heating-can.env` is deployed but unused (it's listed under `env_file:` but nothing in either `command:` reads from the environment), and the secret ends up in the 0644 file instead of only the 0600 one. It's also then visible in `docker inspect`/`docker compose ps` and `ps aux` for anyone who can reach the docker socket or the host process list — the same argv-exposure the old systemd design specifically avoided at the unit-file level. Fix: have the commands read the password from the environment (e.g. `sh -c` wrapper reading `$OPEN3E_MUSER`, or open3e/E3onCAN's own env-var support if any) instead of interpolating it via Jinja into the compose file. ## 🟡 Suggestions - **Likely permission failure writing udsoncan's log file (`ci/images/heating-can/Containerfile:571-574`).** The old `tasks/main.yml` (removed by this diff) had the comment "Owned by the service user: udsoncan writes its log file into the working directory," and correspondingly created `{{ edge_base_dir }}/open3e` with `owner: open3e, group: open3e`. The new image sets `WORKDIR /opt` *after* `USER open3e`, but `/opt` is created by the base image as root-owned (default `python:3.13-slim` permissions), and nothing `chown`s it or bind-mounts a writable volume there. If udsoncan really does write a log into its CWD as the old comment states, the open3e container will hit a permission error on startup under the unprivileged `open3e` user. I could not run the container to confirm this — worth checking directly (`docker run --rm -u open3e <image> open3e ...` or grepping the udsoncan source for where it opens its log file) before this ships, since if true it'll break the primary open3e service, not just the overlay. - **`tasks/main.yml:37-56` — masking is undone by the very next task.** "Stop and mask the native CAN services" masks `open3e.service`/`e3oncan.service` (which creates a symlink to `/dev/null` at `/etc/systemd/system/<name>.service`), but "Remove the native unit files" immediately deletes that exact path. The end state (no unit file at all) still prevents `systemctl start`, so this isn't functionally broken, but the mask step's stated purpose in the comment ("masked so a stray `systemctl start` ... cannot bring them back") doesn't actually persist — it's overwritten one task later. Either drop the `masked: true` (removal already achieves the goal) or reorder so removal doesn't clobber the mask. - **`tasks/main.yml:106-118` — redundant recreate on first run.** "Start heating-can services" runs `docker compose up -d` unconditionally after the compose file/env/config tasks, and any of those tasks notifies the `Restart heating-can` handler (`docker compose up -d --force-recreate`) which fires at end-of-play. On a config change (including the very first run) containers get started once by the task and then immediately force-recreated by the handler. Not incorrect, just a wasted extra container churn each time config changes. ## 🟢 Nits - `ci/images/heating-can/Containerfile:538-539` repeats the `OPEN3E_REF`/`E3ONCAN_REF` `ARG` declarations in the final stage purely to populate the `LABEL`s — correct (ARGs don't cross `FROM` boundaries) but easy to let drift from the build-stage values if someone edits one and not the other; a comment noting they must stay in sync would help. ## Tests No automated coverage exists for this role (none existed before this PR either, and there's no ansible-lint/molecule/syntax-check step in `.woodpecker/`), so this is consistent with repo convention. Given that, the two blocking issues above are exactly the kind of thing that would only surface at deploy time on real hardware — worth a manual `docker compose config` / dry run against the rendered templates, and ideally a one-off manual test on edge-1 (or a scratch host) before merging, particularly to check the vx3 dpList path and whether open3e actually starts cleanly as the unprivileged user. ## Recommendation Request changes — 2 blocking issues. <!-- pr-review:sha=56a9859e501e8ccf09ea408268518f07b0e62ecf -->

claude-bot added 1 commit 2026-08-09 08:06:44 +00:00

build(heating-can): one container, both readers supervised inside it ... 96fcb426aa

Reverts my own reinterpretation. The instruction was to put both in one
container because they belong together; I argued against it on crash-isolation
grounds, then read the correction "two services, one image" as agreement with
my version rather than as a restatement of the original. It was not.

A container runs one PID 1. With two processes that PID 1 is a supervisor,
which is how a container handles multiple services. supervisord now runs both
readers: it restarts either on its own if it dies, reaps children and forwards
shutdown -- none of which a bare process as PID 1 can do for a sibling, which
was the actual answer to the concern I raised.

Also gone: the dispatch entrypoint and the argv in the compose file. Both were
the same mistake in different places -- the invocation living outside the
artifact. open3e's and E3onCAN's command lines are an implementation detail of
this image now; supervisord builds them from environment variables it expands
itself. Nothing outside the image knows either CLI, so an upstream flag rename
cannot break the compose file or an Ansible template.

The compose file supplies configuration and nothing else: one service, one
container, environment only. The credential env file is dropped -- the
credentials are environment already.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude-bot added 1 commit 2026-08-09 08:12:05 +00:00

fix(heating-can): correct the e3oncan path, and smoke-test the image in CI ... 6fb760edae

The image shipped a path that does not exist. supervisord ran
`python /opt/e3oncan/src/E3onCANcollect.py`, but that file is at the repo
ROOT -- there is no src/ directory. The `src` came from the old Ansible task,
which cloned into `{{ edge_base_dir }}/e3oncan/src`; that was the clone
DESTINATION, not part of the upstream layout, and I carried it into the
Containerfile without checking. Confirmed against the upstream tree at the
pinned commit: E3onCANcollect.py sits at the root alongside requirements.txt,
which is why the build succeeded while the runtime path was wrong.

So the image built green and would have started open3e and failed e3oncan on
every restart -- the E380 meter silently absent, with supervisord retrying
forever.

Nothing caught that, because nothing ran the image. Now CI does: a smoke-test
step uses the freshly built image as its own step image and asserts both
services execute and supervisord's config carries both programs.

It cannot exercise the CAN bus or the broker, neither of which exists on a
runner, so it verifies exactly what is verifiable without hardware. That is
still the difference between "built" and "works" -- and it belongs on a
throwaway runner, not on the production edge host, which is where I wrongly
went looking for this the first time.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot requested changes 2026-08-09 08:13:55 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit 96fcb426aaf8c495e760670e21815bc873298222.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

This PR replaces the pip-into-venv install of open3e + E3onCAN on edge-1 with a single CI-built image (ci/images/heating-can), supervised by supervisord, deployed via a new docker-compose-based heating-can role. The rationale (avoiding VCS-requirement re-clones on every converge, giving the update-audit a normal tagged artifact) is sound and well explained. However, I found three concrete defects that would break the migration as written, and a few smaller issues worth fixing.

🔴 Blocking

• vx3.dpList still points at the old native host path, which doesn't exist in the container. ansible/roles/heating-can/defaults/main.yml:39 (unchanged by this PR) sets dpList: "/opt/edge-host/open3e/vx3plus.py". This value is serialized verbatim into devices.json by the "Deploy open3e devices.json" task (ansible/roles/heating-can/tasks/main.yml, around line 213) and mounted read-only at /config/devices.json inside the container. The overlay file itself is now deployed to {{ heating_can_base_dir }}/vx3plus.py and bind-mounted at /config/vx3plus.py (templates/docker-compose.yml.j2:333-335) — but devices.json's dpList field still says /opt/edge-host/open3e/vx3plus.py, a path that does not exist anywhere inside the container's filesystem (WORKDIR is /opt, and nothing creates /opt/edge-host). open3e will fail to load the vx3 battery-module DID overlay after this migration — exactly the functionality the PR body says this role depends on. Fix: either update dpList to /config/vx3plus.py in defaults/main.yml, or template devices.json to substitute the runtime path.

• supervisord.conf sets user=open3e while the container has already dropped to that user — this is a documented supervisord failure mode. ci/images/heating-can/Containerfile:544-545 runs USER open3e before the ENTRYPOINT, so supervisord starts as a non-root process. ci/images/heating-can/supervisord.conf:569 then sets user=open3e in the [supervisord] section. Supervisor's own privilege-drop logic refuses to switch users unless it is currently running as root, and exits with Can't drop privilege as nonroot user when it isn't — this exact combination (container already non-root via USER, plus user= in the config) is called out in supervisor's own FAQ as something to avoid. If that's right, the container fails at startup on every deploy. I could not execute supervisord in this sandbox to reproduce it directly (no network/python available), so please verify by building the image and running it once (docker run --rm <image> and check whether supervisord starts) — but given the severity (total service outage) this needs confirming before merge. If confirmed, the fix is simply to drop user=open3e from supervisord.conf since the Containerfile already sets the runtime user.

• The MQTT broker password is now written in plaintext into a world-readable file, a regression from the pattern this repo already uses elsewhere. ansible/roles/heating-can/templates/docker-compose.yml.j2:342 inlines MQTT_PASSWORD: "{{ edge_open3e_mqtt_password }}" directly into the compose file's environment: block, and the task that renders it (tasks/main.yml, "Deploy heating-can compose project") deploys it with mode: "0644" into a 0755 directory with no owner restriction — world-readable. The file this replaced (open3e.env.j2, deleted by this PR) held the same secret at mode: "0600", specifically to keep it off other local accounts. Compare with ansible/roles/zwave-gateway/templates/docker-compose.yml.j2, which keeps its compose file secret-free and instead uses a separate env_file: zwavejs.env deployed at mode: "0600" (ansible/roles/zwave-gateway/tasks/main.yml:58-60) — that's the established convention in this repo for exactly this situation. As written, any local account on edge-1 can read the MQTT password by cat-ing the compose file. Fix: move the secret into a separate env_file (or Docker secret) deployed at 0600, referenced from the compose file rather than inlined.

🟡 Suggestions

• Masking is undone in the same play run. tasks/main.yml: "Stop and mask the native CAN services" masks open3e/e3oncan (which, per ansible.builtin.systemd, replaces /etc/systemd/system/<name>.service with a symlink to /dev/null), but the very next task, "Remove the native unit files", deletes that same path (state: absent). The net effect after this play is "no unit file, not masked" rather than "masked" — the specific protection the comment describes ("masked so a stray systemctl start or a leftover dependency cannot bring them back") no longer holds, since a future unit file dropped at that path would need no unmask step to become startable again. Either drop the explicit unit-file-removal task (masking already replaces the file) or reorder so the two don't target the same path.
• changed_when on "Start heating-can services" parses human-readable CLI text ('Started' in ... or 'Created' in ... or 'Creating' in ...) which is brittle across docker compose versions/locales and could silently misreport idempotency. Consider docker compose up -d --wait with a simpler success check, or accept that this task's changed-state is approximate.
• Stale files left behind from the native install. "Remove the native venvs" (tasks/main.yml) removes the venv and e3oncan/src directories but leaves {{ edge_base_dir }}/open3e/devices.json, vx3plus.py, and open3e.env (the latter still containing the old plaintext MQTT credential, at 0600) orphaned on disk. Low risk given the permissions, but worth cleaning up along with the rest of the migration.
• edge_e3oncan_mqtt_format is now dead. defaults/main.yml still defines it, but docker-compose.yml.j2 only sets a single shared MQTT_FORMAT (from edge_open3e_mqtt_format) that both supervisord.conf programs consume via %(ENV_MQTT_FORMAT)s. The two variables currently have the same default value so there's no behavioral change today, but the separate e3oncan variable no longer does anything — worth removing or wiring back up if per-service formatting is still intended.

🟢 Nits

• tasks/main.yml: register: heating_can_native_stopped is never used afterward.

Tests

No automated tests accompany this (consistent with how other roles/images in this repo are handled — no molecule or CI test harness found for Ansible roles). Given the three correctness issues above, at minimum a manual smoke test on edge-1 (build the image, deploy, confirm the container starts and stays up, confirm vx3 battery DIDs still appear on MQTT) is necessary before merge; the PR body already scopes this as artifact-only and defers the cutover, but the image and role should work when exercised together.

Recommendation

Request changes — 3 blocking issues.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `96fcb426aaf8c495e760670e21815bc873298222`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR replaces the pip-into-venv install of open3e + E3onCAN on edge-1 with a single CI-built image (`ci/images/heating-can`), supervised by `supervisord`, deployed via a new docker-compose-based `heating-can` role. The rationale (avoiding VCS-requirement re-clones on every converge, giving the update-audit a normal tagged artifact) is sound and well explained. However, I found three concrete defects that would break the migration as written, and a few smaller issues worth fixing. ## 🔴 Blocking - **`vx3.dpList` still points at the old native host path, which doesn't exist in the container.** `ansible/roles/heating-can/defaults/main.yml:39` (unchanged by this PR) sets `dpList: "/opt/edge-host/open3e/vx3plus.py"`. This value is serialized verbatim into `devices.json` by the "Deploy open3e devices.json" task (`ansible/roles/heating-can/tasks/main.yml`, around line 213) and mounted read-only at `/config/devices.json` inside the container. The overlay file itself is now deployed to `{{ heating_can_base_dir }}/vx3plus.py` and bind-mounted at `/config/vx3plus.py` (`templates/docker-compose.yml.j2:333-335`) — but `devices.json`'s `dpList` field still says `/opt/edge-host/open3e/vx3plus.py`, a path that does not exist anywhere inside the container's filesystem (`WORKDIR` is `/opt`, and nothing creates `/opt/edge-host`). open3e will fail to load the vx3 battery-module DID overlay after this migration — exactly the functionality the PR body says this role depends on. Fix: either update `dpList` to `/config/vx3plus.py` in `defaults/main.yml`, or template `devices.json` to substitute the runtime path. - **`supervisord.conf` sets `user=open3e` while the container has already dropped to that user — this is a documented supervisord failure mode.** `ci/images/heating-can/Containerfile:544-545` runs `USER open3e` before the `ENTRYPOINT`, so `supervisord` starts as a non-root process. `ci/images/heating-can/supervisord.conf:569` then sets `user=open3e` in the `[supervisord]` section. Supervisor's own privilege-drop logic refuses to switch users unless it is currently running as root, and exits with `Can't drop privilege as nonroot user` when it isn't — this exact combination (container already non-root via `USER`, plus `user=` in the config) is called out in supervisor's own FAQ as something to avoid. If that's right, the container fails at startup on every deploy. I could not execute supervisord in this sandbox to reproduce it directly (no network/python available), so please verify by building the image and running it once (`docker run --rm <image>` and check whether supervisord starts) — but given the severity (total service outage) this needs confirming before merge. If confirmed, the fix is simply to drop `user=open3e` from `supervisord.conf` since the Containerfile already sets the runtime user. - **The MQTT broker password is now written in plaintext into a world-readable file, a regression from the pattern this repo already uses elsewhere.** `ansible/roles/heating-can/templates/docker-compose.yml.j2:342` inlines `MQTT_PASSWORD: "{{ edge_open3e_mqtt_password }}"` directly into the compose file's `environment:` block, and the task that renders it (`tasks/main.yml`, "Deploy heating-can compose project") deploys it with `mode: "0644"` into a `0755` directory with no owner restriction — world-readable. The file this replaced (`open3e.env.j2`, deleted by this PR) held the same secret at `mode: "0600"`, specifically to keep it off other local accounts. Compare with `ansible/roles/zwave-gateway/templates/docker-compose.yml.j2`, which keeps its compose file secret-free and instead uses a separate `env_file: zwavejs.env` deployed at `mode: "0600"` (`ansible/roles/zwave-gateway/tasks/main.yml:58-60`) — that's the established convention in this repo for exactly this situation. As written, any local account on edge-1 can read the MQTT password by `cat`-ing the compose file. Fix: move the secret into a separate `env_file` (or Docker secret) deployed at `0600`, referenced from the compose file rather than inlined. ## 🟡 Suggestions - **Masking is undone in the same play run.** `tasks/main.yml`: "Stop and mask the native CAN services" masks `open3e`/`e3oncan` (which, per `ansible.builtin.systemd`, replaces `/etc/systemd/system/<name>.service` with a symlink to `/dev/null`), but the very next task, "Remove the native unit files", deletes that same path (`state: absent`). The net effect after this play is "no unit file, not masked" rather than "masked" — the specific protection the comment describes ("masked so a stray systemctl start or a leftover dependency cannot bring them back") no longer holds, since a future unit file dropped at that path would need no `unmask` step to become startable again. Either drop the explicit unit-file-removal task (masking already replaces the file) or reorder so the two don't target the same path. - **`changed_when` on "Start heating-can services" parses human-readable CLI text** (`'Started' in ... or 'Created' in ... or 'Creating' in ...`) which is brittle across `docker compose` versions/locales and could silently misreport idempotency. Consider `docker compose up -d --wait` with a simpler success check, or accept that this task's changed-state is approximate. - **Stale files left behind from the native install.** "Remove the native venvs" (`tasks/main.yml`) removes the `venv` and `e3oncan/src` directories but leaves `{{ edge_base_dir }}/open3e/devices.json`, `vx3plus.py`, and `open3e.env` (the latter still containing the old plaintext MQTT credential, at `0600`) orphaned on disk. Low risk given the permissions, but worth cleaning up along with the rest of the migration. - **`edge_e3oncan_mqtt_format` is now dead.** `defaults/main.yml` still defines it, but `docker-compose.yml.j2` only sets a single shared `MQTT_FORMAT` (from `edge_open3e_mqtt_format`) that both `supervisord.conf` programs consume via `%(ENV_MQTT_FORMAT)s`. The two variables currently have the same default value so there's no behavioral change today, but the separate e3oncan variable no longer does anything — worth removing or wiring back up if per-service formatting is still intended. ## 🟢 Nits - `tasks/main.yml`: `register: heating_can_native_stopped` is never used afterward. ## Tests No automated tests accompany this (consistent with how other roles/images in this repo are handled — no molecule or CI test harness found for Ansible roles). Given the three correctness issues above, at minimum a manual smoke test on edge-1 (build the image, deploy, confirm the container starts and stays up, confirm vx3 battery DIDs still appear on MQTT) is necessary before merge; the PR body already scopes this as artifact-only and defers the cutover, but the image and role should work when exercised together. ## Recommendation Request changes — 3 blocking issues. <!-- pr-review:sha=96fcb426aaf8c495e760670e21815bc873298222 -->

claude-bot added 1 commit 2026-08-09 08:14:00 +00:00

fix(heating-can): self-test at build time instead of a CI step ... fac46aa7ac

The smoke-test step I added errored the whole pipeline before any step ran.
It used the freshly built image as its own step image, but the Gitea registry
is private: the build steps authenticate kaniko for PUSH, nothing gives the
runner credentials to PULL. So the step could never have worked.

Moved into the Containerfile as a build-time RUN, which is better anyway --
it needs no registry credentials, and a broken image is never published
rather than published and then flagged.

It asserts what the previous revision got wrong: both services actually
execute, and supervisord's config carries both programs. That is exactly the
failure that shipped green -- supervisord pointed at
/opt/e3oncan/src/E3onCANcollect.py, which does not exist (the file is at the
repo root), so the image would have run open3e while e3oncan crash-looped
and the E380 meter went silently missing.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot requested changes 2026-08-09 08:20:17 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit fac46aa7ac0e379972399b9bb22006419b780778.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

Replaces the pip-into-venv install of open3e + E3onCAN on edge-1 with a single CI-built image (ci/images/heating-can) run as two supervisord-managed processes, deployed via a docker-compose project instead of two systemd units. The CI wiring, Containerfile structure, and migration-off-native-install tasks are solid and well explained. There is one concrete config regression that will break the vx3 battery-module datapoint overlay, plus a couple of things worth confirming before this goes to edge-1.

🔴 Blocking

• ansible/roles/heating-can/defaults/main.yml:39 — edge_open3e_devices.vx3.dpList is still the old host path /opt/edge-host/open3e/vx3plus.py, unchanged by this PR. The new compose file (templates/docker-compose.yml.j2:19) bind-mounts the overlay to /config/vx3plus.py inside the container, and devices.json is rendered from this same dict (tasks/main.yml:72-77) and mounted at /config/devices.json. Nothing mounts anything at /opt/edge-host/open3e/ inside the container, so open3e will look for the vx3 overlay at a path that doesn't exist there. This silently drops the battery-module DID overlay documented in defaults/main.yml:36-38 (vx3.1577-1582 etc. would fall back to the stock, filtered vx3 list). Fix: change the dpList value to /config/vx3plus.py.

🟡 Suggestions

• ci/images/heating-can/Containerfile:109-111 — WORKDIR /opt is set after USER open3e, but /opt is created by the earlier COPY --from=build layers as root:root with no chown, so it isn't writable by the non-root open3e user. The code this PR deletes explicitly called this out: ansible/roles/heating-can/tasks/main.yml (old) had "Owned by the service user: udsoncan writes its log file into the working directory," and gave open3e's directory owner: open3e. If udsoncan still writes a log file into its CWD on start, that write will now fail. I can't confirm from the checkout whether that failure is fatal to open3e (upstream source isn't vendored here) — worth confirming by actually starting the built image and checking for a permission error, or just pre-empt it with a chown open3e /opt (or a dedicated writable WORKDIR) before USER open3e.
• ansible/roles/heating-can/templates/docker-compose.yml.j2:26 — MQTT_PASSWORD is embedded in plaintext in the compose file, which is deployed with mode: "0644" (tasks/main.yml:87-92), i.e. world-readable. This role's own previous file with the same secret (open3e.env.j2, now deleted) was mode: "0600", and sibling roles on the same host keep secrets in a separate 0600 file rather than the 0644 compose file (e.g. power-meter-sml/templates/sml2mqtt-config.yml.j2 is 0600 while its compose file is 0644). n8n's compose file does the same inline-secret-at-0644 thing this PR does, so it's not unprecedented in this repo, but it is a regression from what this specific role used to do. Consider rendering the password via an env_file at 0600 instead, matching power-meter-sml/zwave-gateway.
• ansible/roles/heating-can/defaults/main.yml:43 — edge_e3oncan_mqtt_format is now dead: docker-compose.yml.j2:27 sets MQTT_FORMAT from edge_open3e_mqtt_format for both services (the old e3oncan unit used edge_e3oncan_mqtt_format via its own -mfstr). Since the two vars currently default to the same string, behavior is unchanged today, but overriding edge_e3oncan_mqtt_format no longer does anything. Either wire it through to a distinct env var the image respects for e3oncan, or drop the now-unused default.

🟢 Nits

• tasks/main.yml:109-111 — the changed_when check for docker compose up -d mixes 'Created'/'Started' in stderr with 'Creating'/'Started' in stdout; only affects Ansible's reported "changed" status, not actual behavior.
• tasks/main.yml:47 — heating_can_native_stopped is registered but never read afterward.
• tasks/main.yml:62-69 removes the venvs/src but leaves the now-empty {{ edge_base_dir }}/open3e and {{ edge_base_dir }}/e3oncan directories behind.

Tests

No automated test framework for Ansible roles exists in this repo (no molecule/ansible-lint CI step), consistent with how other roles here are handled, so that's not a gap specific to this PR. The Containerfile's build-time self-test (RUN open3e --help ... && python .../E3onCANcollect.py --help ...) is a good addition and already caught one real path bug per its own comment, but it only checks that the binaries execute — it wouldn't catch the dpList path regression above, since that only manifests once devices.json is actually parsed against the mounted files at runtime. Worth a manual docker compose up + log check on a real edge host (or at least docker run with the rendered devices.json/vx3plus.py) before rolling this out, given the migration also stops and masks the native units in the same run.

Recommendation

Request changes — 1 blocking issue.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `fac46aa7ac0e379972399b9bb22006419b780778`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Replaces the pip-into-venv install of open3e + E3onCAN on edge-1 with a single CI-built image (`ci/images/heating-can`) run as two supervisord-managed processes, deployed via a docker-compose project instead of two systemd units. The CI wiring, Containerfile structure, and migration-off-native-install tasks are solid and well explained. There is one concrete config regression that will break the vx3 battery-module datapoint overlay, plus a couple of things worth confirming before this goes to edge-1. ## 🔴 Blocking - **`ansible/roles/heating-can/defaults/main.yml:39`** — `edge_open3e_devices.vx3.dpList` is still the old host path `/opt/edge-host/open3e/vx3plus.py`, unchanged by this PR. The new compose file (`templates/docker-compose.yml.j2:19`) bind-mounts the overlay to `/config/vx3plus.py` inside the container, and `devices.json` is rendered from this same dict (`tasks/main.yml:72-77`) and mounted at `/config/devices.json`. Nothing mounts anything at `/opt/edge-host/open3e/` inside the container, so open3e will look for the vx3 overlay at a path that doesn't exist there. This silently drops the battery-module DID overlay documented in `defaults/main.yml:36-38` (vx3.1577-1582 etc. would fall back to the stock, filtered vx3 list). Fix: change the `dpList` value to `/config/vx3plus.py`. ## 🟡 Suggestions - **`ci/images/heating-can/Containerfile:109-111`** — `WORKDIR /opt` is set after `USER open3e`, but `/opt` is created by the earlier `COPY --from=build` layers as `root:root` with no `chown`, so it isn't writable by the non-root `open3e` user. The code this PR deletes explicitly called this out: `ansible/roles/heating-can/tasks/main.yml` (old) had "Owned by the service user: udsoncan writes its log file into the working directory," and gave open3e's directory `owner: open3e`. If udsoncan still writes a log file into its CWD on start, that write will now fail. I can't confirm from the checkout whether that failure is fatal to open3e (upstream source isn't vendored here) — worth confirming by actually starting the built image and checking for a permission error, or just pre-empt it with a `chown open3e /opt` (or a dedicated writable `WORKDIR`) before `USER open3e`. - **`ansible/roles/heating-can/templates/docker-compose.yml.j2:26`** — `MQTT_PASSWORD` is embedded in plaintext in the compose file, which is deployed with `mode: "0644"` (`tasks/main.yml:87-92`), i.e. world-readable. This role's own previous file with the same secret (`open3e.env.j2`, now deleted) was `mode: "0600"`, and sibling roles on the same host keep secrets in a separate 0600 file rather than the 0644 compose file (e.g. `power-meter-sml/templates/sml2mqtt-config.yml.j2` is 0600 while its compose file is 0644). `n8n`'s compose file does the same inline-secret-at-0644 thing this PR does, so it's not unprecedented in this repo, but it is a regression from what this specific role used to do. Consider rendering the password via an env_file at 0600 instead, matching `power-meter-sml`/`zwave-gateway`. - **`ansible/roles/heating-can/defaults/main.yml:43`** — `edge_e3oncan_mqtt_format` is now dead: `docker-compose.yml.j2:27` sets `MQTT_FORMAT` from `edge_open3e_mqtt_format` for both services (the old e3oncan unit used `edge_e3oncan_mqtt_format` via its own `-mfstr`). Since the two vars currently default to the same string, behavior is unchanged today, but overriding `edge_e3oncan_mqtt_format` no longer does anything. Either wire it through to a distinct env var the image respects for e3oncan, or drop the now-unused default. ## 🟢 Nits - `tasks/main.yml:109-111` — the `changed_when` check for `docker compose up -d` mixes `'Created'`/`'Started'` in stderr with `'Creating'`/`'Started'` in stdout; only affects Ansible's reported "changed" status, not actual behavior. - `tasks/main.yml:47` — `heating_can_native_stopped` is registered but never read afterward. - `tasks/main.yml:62-69` removes the venvs/src but leaves the now-empty `{{ edge_base_dir }}/open3e` and `{{ edge_base_dir }}/e3oncan` directories behind. ## Tests No automated test framework for Ansible roles exists in this repo (no molecule/ansible-lint CI step), consistent with how other roles here are handled, so that's not a gap specific to this PR. The Containerfile's build-time self-test (`RUN open3e --help ... && python .../E3onCANcollect.py --help ...`) is a good addition and already caught one real path bug per its own comment, but it only checks that the binaries execute — it wouldn't catch the `dpList` path regression above, since that only manifests once `devices.json` is actually parsed against the mounted files at runtime. Worth a manual `docker compose up` + log check on a real edge host (or at least `docker run` with the rendered `devices.json`/`vx3plus.py`) before rolling this out, given the migration also stops and masks the native units in the same run. ## Recommendation Request changes — 1 blocking issue. <!-- pr-review:sha=fac46aa7ac0e379972399b9bb22006419b780778 -->

claude-bot referenced this issue from a commit 2026-08-10 18:58:12 +00:00

fix(heating-can): correct the overlay path, secret mode and writable CWD

claude-bot added 1 commit 2026-08-10 18:58:12 +00:00

fix(heating-can): correct the overlay path, secret mode and writable CWD ... f9848d2bf5

Review on #476 found a real regression plus three things worth fixing before
this reaches edge-1.

Blocking: edge_open3e_devices.vx3.dpList still pointed at the old host path
/opt/edge-host/open3e/vx3plus.py. The compose file mounts that overlay at
/config/vx3plus.py and nothing mounts anything under /opt/edge-host inside
the container, so open3e would have looked for it at a path that does not
exist -- silently falling back to the stock vx3 datapoint list and dropping
the battery-module DIDs (vx3.1577-1582 and friends) the overlay exists to
add. Now /config/vx3plus.py.

Worth noting the build-time self-test could not have caught this: it proves
the binaries execute, not that devices.json resolves against the mounted
files at runtime.

Writable working directory. WORKDIR was /opt, which the COPY layers create
root-owned, while the process runs as open3e. udsoncan writes its log file
into the CWD -- the native install gave open3e's directory `owner: open3e`
for exactly that reason, and I dropped that when containerising. Now a
dedicated /var/lib/open3e, owned by the service user.

Secret mode. MQTT_PASSWORD was inline in docker-compose.yml, which is
rendered 0644. The env file this replaced was 0600, and the sibling roles on
this host keep secrets in a 0600 file beside a world-readable compose file.
Restored: credentials move to heating-can.env at 0600, referenced via
env_file.

edge_e3oncan_mqtt_format had become dead -- both services were fed
MQTT_FORMAT from edge_open3e_mqtt_format. The two default to the same string
so behaviour was unchanged, but overriding it did nothing. Wired through as
E3ONCAN_MQTT_FORMAT, which supervisord passes to e3oncan only.

Also from review: dropped the registered-but-unread heating_can_native_stopped,
and the migration now removes the emptied {{ edge_base_dir }}/open3e and
/e3oncan directories rather than leaving them behind.

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

review-bot reviewed 2026-08-10 19:03:09 +00:00

review-bot left a comment

Member

Copy Link

Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit f9848d2bf53d437e094fbb54ca5e2d3f86a92478.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

---

Summary

This PR replaces the pip-into-venv + systemd deployment of open3e and E3onCAN on edge-1 with a single date-tagged image (built via a new kaniko CI step) run as one two-process container behind supervisord, driven by a rewritten heating-can Ansible role that migrates off the native install. The change is well-scoped, internally consistent, and closely follows the conventions already established by the power-meter-sml and zwave-gateway roles on the same host (compose layout, docker compose pull/up -d idempotency pattern, 0600 credential env file kept separate from the 0644 compose file, restart: Restart <role> handler doing --force-recreate). I traced the MQTT credential and CLI-argument wiring from the old systemd units through to the new supervisord.conf and confirmed both open3e's and E3onCAN's command lines (including the -l {{ topic }}/cmnd write path and the combined user:pass -muser argument) are preserved. No blocking issues found.

🟡 Suggestions

• ansible/docs/edge-host.md is now stale and gives commands that will fail post-merge. This PR doesn't touch that file, but it directly contradicts it:

  • Line 10: the service table still lists open3e | venv + systemd.
  • Line 109: the cutover checklist says systemctl start open3e — the unit is now stopped, masked, and its file removed by this very role (ansible/roles/heating-can/tasks/main.yml:37-55).
  • Line 125: "the pip task tracks the git default branch (pin edge_open3e_pkg to a tag if that ever bites)" — edge_open3e_pkg was deleted in this PR (ansible/roles/heating-can/defaults/main.yml:9 diff), and there's no more pip task at all.
  • Line 126: "Ad-hoc CAN scans: systemctl stop open3e first" — also no longer valid; the equivalent action is now docker compose stop in heating_can_base_dir, and it's worth noting explicitly that stopping the container for an ad-hoc scan now also stops e3oncan (previously independent), since they share one PID 1.
    Worth a follow-up doc pass, ideally before someone runs this runbook by hand.

• ansible/roles/heating-can/tasks/main.yml:37-55 ("Stop and mask the native CAN services" / "Remove the native unit files") will run on every converge indefinitely, not just during the cutover. It's idempotent so it's harmless, but once the migration has actually happened on edge-1 this becomes permanent dead weight in every future run. Not urgent — just flagging for a later cleanup pass, consistent with the PR's own "deliberately artifact-only, migration is a separate concern" framing.

Tests

No automated tests exist for this role or Containerfile (consistent with the rest of the Ansible/CI tree, which has none). The Containerfile's build-time self-test (ci/images/heating-can/Containerfile:577-582) is a good substitute for the two things that would otherwise only fail at runtime: it invokes both entrypoints and greps supervisord.conf for both [program:...] sections, and the PR description notes it's specifically designed to catch the kind of silent breakage (wrong script path) that happened before. That's reasonable coverage for an artifact like this; no further test is expected here.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `f9848d2bf53d437e094fbb54ca5e2d3f86a92478`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR replaces the pip-into-venv + systemd deployment of open3e and E3onCAN on edge-1 with a single date-tagged image (built via a new kaniko CI step) run as one two-process container behind supervisord, driven by a rewritten `heating-can` Ansible role that migrates off the native install. The change is well-scoped, internally consistent, and closely follows the conventions already established by the `power-meter-sml` and `zwave-gateway` roles on the same host (compose layout, `docker compose pull`/`up -d` idempotency pattern, 0600 credential env file kept separate from the 0644 compose file, `restart: Restart <role>` handler doing `--force-recreate`). I traced the MQTT credential and CLI-argument wiring from the old systemd units through to the new `supervisord.conf` and confirmed both open3e's and E3onCAN's command lines (including the `-l {{ topic }}/cmnd` write path and the combined `user:pass` `-muser` argument) are preserved. No blocking issues found. ## 🟡 Suggestions - **`ansible/docs/edge-host.md` is now stale and gives commands that will fail post-merge.** This PR doesn't touch that file, but it directly contradicts it: - Line 10: the service table still lists `open3e | venv + systemd`. - Line 109: the cutover checklist says `systemctl start open3e` — the unit is now stopped, masked, and its file removed by this very role (`ansible/roles/heating-can/tasks/main.yml:37-55`). - Line 125: "the pip task tracks the git default branch (pin `edge_open3e_pkg` to a tag if that ever bites)" — `edge_open3e_pkg` was deleted in this PR (`ansible/roles/heating-can/defaults/main.yml:9` diff), and there's no more pip task at all. - Line 126: "Ad-hoc CAN scans: `systemctl stop open3e` first" — also no longer valid; the equivalent action is now `docker compose stop` in `heating_can_base_dir`, and it's worth noting explicitly that stopping the container for an ad-hoc scan now also stops e3oncan (previously independent), since they share one PID 1. Worth a follow-up doc pass, ideally before someone runs this runbook by hand. - **`ansible/roles/heating-can/tasks/main.yml:37-55`** ("Stop and mask the native CAN services" / "Remove the native unit files") will run on every converge indefinitely, not just during the cutover. It's idempotent so it's harmless, but once the migration has actually happened on edge-1 this becomes permanent dead weight in every future run. Not urgent — just flagging for a later cleanup pass, consistent with the PR's own "deliberately artifact-only, migration is a separate concern" framing. ## Tests No automated tests exist for this role or Containerfile (consistent with the rest of the Ansible/CI tree, which has none). The Containerfile's build-time self-test (`ci/images/heating-can/Containerfile:577-582`) is a good substitute for the two things that would otherwise only fail at runtime: it invokes both entrypoints and greps `supervisord.conf` for both `[program:...]` sections, and the PR description notes it's specifically designed to catch the kind of silent breakage (wrong script path) that happened before. That's reasonable coverage for an artifact like this; no further test is expected here. ## Recommendation Looks reasonable — no blocking issues; comments above are optional. <!-- pr-review:sha=f9848d2bf53d437e094fbb54ca5e2d3f86a92478 -->

k.renner merged commit 685905b1df into master 2026-08-10 19:04:50 +00:00

k.renner referenced this issue from a commit 2026-08-10 19:04:51 +00:00

Merge pull request 'build(heating-can): image for the CAN readers, replacing pip-into-venv' (#476) from heating-can-image into master

claude-bot referenced this issue from a commit 2026-08-10 19:17:43 +00:00

fix(audit): track the heating-can image pins, not the deleted Ansible vars

claude-bot referenced this pull request 2026-08-10 19:46:05 +00:00

fix(heating-can): stop the native services before removing their units #477

Sign in to join this conversation.

Reviewers

No Reviewers

review-bot

Labels

Clear labels

apollo

Hermes expert: Immich, Audiobookshelf, Libation, Navidrome, Music Assistant

atlas

Hermes expert: Talos, Flux, network, Authentik, Matrix, monitoring

clio

Hermes expert: Paperless, wiki, ical-proxy, TRMNL, websites

daedalus

Hermes expert: Gitea, Woodpecker, Renovate, review-bot, Hermes

hestia

Hermes expert: Home Assistant, IoT, MQTT, ESPHome, voice

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

alert-monitor-bot claude-bot (Claude Bot) flux hermes-bot issue-fix-bot k.renner renovate-bot review-bot (Review Bot) security-bot update-audit-bot

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#476