Ubuntu24 installer: Updates to installer service #1226
Conversation
At system restart, the api and log mounts are not there and thus the UI does not start due to failures. Introducing a pre-start service section to set those up
This is now part of the pre-start script
WalkthroughTmpfs creation and initialization for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor systemd
participant unit as netalertx.service
participant pre as pre-start.sh
participant fs as Filesystem
participant app as NetAlertX
systemd->>unit: Start unit
Note over unit: ExecStartPre (new) runs before ExecStart
unit->>pre: /usr/bin/bash ${INSTALLER_DIR}/pre-start.sh
pre->>fs: Unmount previous mounts (if any)
pre->>fs: Create dirs/files (INSTALL_DIR/log, INSTALL_DIR/api, logs, plugins, user_notifications.json)
pre->>fs: Mount tmpfs for log and api
pre->>fs: chown/chmod (www-data, perms)
pre-->>unit: exit 0
unit->>app: ExecStart (unchanged) -> run server
app-->>unit: Running
unit-->>systemd: Active
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
install/ubuntu24/pre-start.sh (1)
5-10: Consider adding error handling for the source command.If
/etc/default/netalertxdoesn't exist or fails to source, the script continues and only fails later when checking INSTALL_DIR. Adding explicit error handling would provide clearer diagnostics.Apply this diff to improve error handling:
-source /etc/default/netalertx +source /etc/default/netalertx || { + echo "[NetAlertX Pre-Start] Failed to source /etc/default/netalertx" + exit 1 +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
install/ubuntu24/install.sh(1 hunks)install/ubuntu24/netalertx.service(1 hunks)install/ubuntu24/pre-start.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (1)
install/ubuntu24/netalertx.service (1)
7-7: INSTALLER_DIR is correctly exported
install/ubuntu24/install.sh creates/etc/default/netalertxand writesINSTALLER_DIR(alongside the other env vars) before enabling and starting the service.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install/ubuntu24/install.sh (1)
279-285: Guard api/log ownership and perms to avoid noisy errors pre–pre-start.If pre-start hasn’t run yet,
${INSTALL_DIR}/api/user_notifications.jsonand${INSTALL_DIR}/logmay not exist. Guard existence or silence benign failures.Apply:
-if [ -f "$FILEDB" ]; then - chown -R www-data:www-data "$FILEDB" -fi -chown root:www-data "${INSTALL_DIR}"/api/user_notifications.json +if [ -f "$FILEDB" ]; then + chown -R www-data:www-data "$FILEDB" +fi +if [ -f "${INSTALL_DIR}/api/user_notifications.json" ]; then + chown root:www-data "${INSTALL_DIR}/api/user_notifications.json" +fi chgrp -R www-data "${INSTALL_DIR}" chmod -R u+rwx,g+rwx,o=rx "$WEB_UI_DIR" chmod -R u+rwx,g+rwx,o=rx "${INSTALL_DIR}" -chmod -R u+rwX,g+rwX,o=rX "${INSTALL_DIR}/log" +if [ -d "${INSTALL_DIR}/log" ]; then + chmod -R u+rwX,g+rwX,o=rX "${INSTALL_DIR}/log" +fi chmod -R u+rwX,g+rwX,o=rX "${INSTALL_DIR}/config"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
install/ubuntu24/install.sh(1 hunks)install/ubuntu24/netalertx.service(1 hunks)install/ubuntu24/pre-start.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- install/ubuntu24/pre-start.sh
- install/ubuntu24/netalertx.service
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
| # We moved the log and api folder creation to the pre-start script | ||
| # Ref pre-start.sh | ||
| # Otherwise the system does not work as the tmp mount points are not there yet |
There was a problem hiding this comment.
🧩 Analysis chain
Pre-start move looks good; verify systemd variable expansion and env wiring.
Ensure netalertx.service either:
- uses an absolute path in ExecStartPre, or
- uses
/usr/bin/bash -lc '<cmd>'(or-c) withEnvironmentFile=/etc/default/netalertxso${INSTALLER_DIR}expands. PlainExecStartPre=/usr/bin/bash ${INSTALLER_DIR}/pre-start.shwon’t expand${…}under systemd.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== netalertx.service ExecStartPre and env =="
rg -nC3 -e '^(Environment(File)?=|ExecStartPre=)' install/ubuntu24/netalertx.service || true
echo
echo "== Full ExecStartPre lines =="
rg -n 'ExecStartPre=' install/ubuntu24/netalertx.service
echo
echo "== pre-start.sh shebang and header =="
sed -n '1,25p' install/ubuntu24/pre-start.sh || trueLength of output: 1252
Fix ExecStartPre in netalertx.service to expand ${INSTALLER_DIR}
In install/ubuntu24/netalertx.service, replace
ExecStartPre=/usr/bin/bash "${INSTALLER_DIR}/pre-start.sh"
with
ExecStartPre=/usr/bin/bash -c '$INSTALLER_DIR/pre-start.sh'
so the shell expands the environment variable before running the script.
🤖 Prompt for AI Agents
In install/ubuntu24/install.sh around lines 253 to 255, the systemd unit's
ExecStartPre currently uses a quoted literal path that prevents ${INSTALLER_DIR}
expansion; update the unit generation to use ExecStartPre=/usr/bin/bash -c
'$INSTALLER_DIR/pre-start.sh' (i.e. invoke bash -c with the single-quoted
command so the shell expands the environment variable and runs the pre-start.sh
from the installer directory).
|
Hello @ingoratsdorf . I've noticed lot of commits from you on the hardware installers. I'm watching and worried because I'm in the middle of a month's long revamp of the main docker container, docker compose, and devcontainer, working toward a new and more secure, reliable, unified user/developer experience. I'd like it if you could please try to focus on making this a single script which deploys everything required with a single copy-paste operation. The hardware installers are not officially supported due to lack of ability to cleanly uninstall/reinstall on a hardware installation. They will be moving into docs only soon. Combining the installer into a document will make your work live longer eg Currently the hardware installers all download the code directly from the github repository making them entirely separate from the code, and not actually needing to be located within the code. Because they aren't actually part of the repo, they can't be tested with the repo. The upcoming changes will likely break a lot of things and I'd like you to be prepared for these changes. |
|
@ingoratsdorf I think we should combine the best of both scripts and just have one, non docker, installer. The one I did for proxmox will work on ubuntu as well, but I do have to address an issue with the tmps file mount strategy. I haven't had the chance to mess with that, I need to finish a server upgrade so I can use this with multiple VLAN's. |
|
@JVKeller yes, I noticed that, after restart of the server / container, the tmpfs is gone. The bare metal installer have to change significantly now that we have a production file system. I think I'll drop work on the bare metal installer and use an Alpine docker LXC again ;-) |
|
The proxmox should really be called debian 13 not proxmox. It's designed to support Debian 13. A proper proxmox script would setup an environment within proxmox and then install. |
|
After you're done with the code base overhaul, we'll revisit this and maybe make it a full fledged Proxmox installer and add it to the proxmox helper scripts repo. I never even fully got NetAlertx working for my network. It wasn't picking up devices on other VLAN's, and adding additional eth devices wasn't working for me either so I have to look at my network config but haven't had the time. |
|
Layer 2 scanners don't cross network boundaries. You'll need to use one of the higher layer scanners. |
|
If someone can have a look that would be great - we are not planning to touch the file structure for a while. One proxmox related issue reported here as well #1336 |
Summary by CodeRabbit
Refactor
Chores