ansible-scylla-monitoring: optional systemd boot service#536
Conversation
Allow opt-in install of scylla-monitoring.service so the stack restarts after reboot via the existing start/stop wrappers. Co-authored-by: Cursor <cursoragent@cursor.com>
ManjotS
left a comment
ManjotS
left a comment
There was a problem hiding this comment.
This adds an opt-in systemd unit (install_scylla_monitoring_service, default false) that installs and enables scylla-monitoring.service, wiring it to the existing start/stop wrapper scripts so the monitoring stack survives reboots and docker.service restarts via PartOf=. The change is small, additive, and off by default, so it carries low risk to existing deployments.
Overall this looks solid — the Requires=/After=/PartOf=docker.service combination on the unit is the correct, idiomatic way to tie a docker-compose-style service to docker.service's lifecycle (see inline praise). A few things worth a look:
- The unit is
enabled but neverstarted by Ansible, sosystemctl status scylla-monitoringwill reportinactive (dead)right after a deploy even though the stack is actually running — see inline comment for details. - The new code path (
install_scylla_monitoring_service: true) isn't exercised anywhere in the molecule suite, so CI won't catch regressions in the template or task logic. - A small style nit (FQCN vs. the file's existing short module names) and a question about the disable/cleanup story when the flag is flipped back off.
Nothing here is blocking — the feature is off by default and functionally sound for the stated goal (reboot survival + docker-restart propagation).
Coverage note: reviewed correctness/logic, security, reliability/operability, maintainability, and test coverage for the diff. I read the role's task graph and templates but did not have a live host to actually exercise the reboot / systemctl restart docker test-plan items from the PR description — those are assessed from the unit file and task logic, not from running them.
- Start the unit (state: started) so systemctl status reflects the running stack right after deploy, not just after reboot. - Use short systemd module name for consistency with the rest of docker.yml. - Add disable/remove cleanup when install_scylla_monitoring_service is false so flipping the flag off no longer leaves an enabled unit behind; running stack is left untouched. - Add reload systemd handler for unit removal. - Exercise the flag in molecule: enable it in converge and assert the unit is installed and enabled in verify. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Security review flagged a local privilege escalation: the root-run systemd
unit executed a wrapper script owned by the (potentially non-root) deploy
user, who could edit it and have it run as root on boot.
- Run the unit as the deploy user by default (User=/Group={{ ansible_user_id }}),
matching the manual start path so root no longer executes a user-owned script.
- When run_docker_with_sudo is set, docker requires root and the unit runs as
root, so write the wrapper scripts root:root to keep them non-writable by the
deploy user.
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
install_scylla_monitoring_service(defaultfalse)scylla-monitoring.serviceso the stack starts after reboot (and whendocker.servicerestarts viaPartOf=)scylla_monitoring_deploy_pathTest plan
-e install_scylla_monitoring_service=true— confirm unit present, enabled, andsystemctl cat scylla-monitoringpoints at wrapper scriptssystemctl restart docker— monitoring stops/starts with Docker (PartOf=docker.service)Made with Cursor