Backups: add restic errors

docs/drafts/ansible-review.md

@@ -0,0 +1,150 @@
+# Ревью плейбуков: best practices и конвенции Ansible
+
+Дата: 2026-05-25. Статус: черновик (заметки по итогам ревью, не план работ).
+
+Проанализированы инвентарь, `ansible.cfg`, роли (`owner`, `eget`, `secrets`) и
+репрезентативная выборка плейбуков: `gitea`, `memos`, `wanderer`, `backups`,
+`system`, `caddyproxy`, `authelia`, `netdata`, `docker`, `eget`, `all-*`.
+Находки отсортированы по влиянию.
+
+## Договорённость по структуре (важно для контекста)
+
+Изначальная рекомендация «вынести общий деплой в одну generic-роль `docker_app`»
+**отклонена осознанно** и не должна предлагаться снова:
+
+- приложения реально разные, мелкие отличия больно загонять в единую абстракцию;
+- catch-all роль обрастает флагами `when:` и читается хуже, чем N честных плейбуков;
+- per-playbook дублирование даёт locality of behavior и возможность обкатать новый
+  подход на одном сервисе, затем раскатать на остальные.
+
+Правильное направление — **набор маленьких composable-ролей на инвариантных швах**
+(как уже сделано с `owner`), а не одна роль на всё. Per-app конфиг остаётся локально
+в плейбуке сервиса.
+
+## 1. Extraction только на чистых швах (не мега-роль)
+
+Per-app конфиг (каталоги, шаблоны, env, порты, особенности compose) — оставляем в
+плейбуке сервиса. Выносим лишь то, что реально инвариантно и повторилось многократно:
+
+- **Бэкап** — самый чистый шов: `gobackup.yml` + `backup.sh` + `backup-targets` +
+  интеграция с restic. Механизм одинаков у всех, различается только список целей.
+  Роль `backup` с параметром «список targets» не трогает индивидуальность сервиса.
+- `owner` уже сделан как отдельная composable-роль — это правильный размер абстракции.
+
+## 2. `vars_files` в каждом плейбуке → `group_vars/all/`
+
+В каждом плейбуке повторяется:
+
+```yaml
+vars_files:
+  - vars/secrets.yml
+  - vars/vars.yml
+```
+
+Ansible автоматически подхватывает `group_vars/all.yml` и `group_vars/all/secrets.yml`
+(vault) для группы `all`. Перенос `vars/vars.yml` → `group_vars/all/main.yml` и
+`vars/secrets.yml` → `group_vars/all/vault.yml` убирает boilerplate из всех плейбуков.
+Адаптируется по одному плейбуку за раз.
+
+## 3. Нет handlers — `state: restarted` безусловный
+
+Ни в одном плейбуке нет `handlers:`. Вместо этого:
+
+- `playbook-caddyproxy.yml:106`, `playbook-netdata.yml:143`, `playbook-authelia.yml:92` —
+  задача `state: restarted` выполняется **всегда**, рестартит контейнер на каждом
+  прогоне даже без изменений (не идемпотентно, лишний downtime);
+- `playbook-gitea.yml` — рестарта нет вовсе (несогласованность).
+
+Канонический паттерн: шаблон конфига `notify`-ит handler, который рестартит только при
+реальном изменении.
+
+```yaml
+- name: "Copy docker compose file"
+  ansible.builtin.template: { ... }
+  notify: Restart app
+
+handlers:
+  - name: Restart app
+    community.docker.docker_compose_v2:
+      project_src: "{{ base_dir }}"
+      state: restarted
+```
+
+Связанное: в `playbook-memos.yml:76` результат шаблона регистрируется в
+`docker_compose_file_result`, но нигде не используется — задумывалось под `when`/`notify`,
+не доведено.
+
+Внедряется инкрементально, по одному сервису.
+
+## 4. Идемпотентность и `changed_when`
+
+- **`playbook-netdata.yml:118-125`** — `changed_when: netdata_docker_group_output.rc != 0`
+  для read-only запроса лишено смысла (помечает «changed» только при ошибке). Должно быть
+  `changed_when: false`. Лучше заменить `shell: grep docker /etc/group | cut ...` на модуль:
+
+  ```yaml
+  - ansible.builtin.getent:
+      database: group
+      key: docker
+  # далее: getent_group['docker'][1]
+  ```
+
+  Уйдёт и `set -o pipefail`, и хрупкий парсинг.
+
+- **`playbook-eget.yml:23-78`** — восемь `command` помечены `changed_when: false`, хотя
+  реально ставят/обновляют бинарники. Прогон всегда рапортует «ok» — теряется честность
+  `--diff`. Сама роль `eget` делает корректную проверку версии; те же инсталляции через
+  неё или через проверку версии были бы идемпотентны по-настоящему.
+
+- **`playbook-memos.yml:57-67`** (и аналоги) — сборка `backup-targets` через `lineinfile`
+  в цикле не удаляет устаревшие строки при изменении списка, а `mode: "0750"` на
+  файле-списке выглядит как copy-paste. Чище — `template`/`copy: content` со всем списком.
+
+## 5. Роль `owner` — несогласованность с ролью `eget`
+
+- **`roles/owner/tasks/main.yml:2-10`** — валидация аргументов через `fail` + `when`,
+  причём две задачи с **идентичным именем**. Роль `eget` для того же делает `assert`
+  (`roles/eget/tasks/main.yml:15`). Привести к одному стилю — `assert` либо современный
+  `meta/argument_specs.yml` (декларативная валидация).
+- **`roles/owner/tasks/main.yml:32,53`** — `with_items`/`with_dict` устарели; конвенция —
+  `loop`: `loop: "{{ owner_ssh_keys }}"`, `loop: "{{ owner_env_dict | dict2items }}"`.
+- У `owner` нет `meta/main.yml` и README, тогда как у `eget` и `secrets` они есть.
+- Имена задач в `owner` с точкой на конце (`"Prepare env variables."`), в остальных без —
+  ansible-lint в строгом профиле это ловит.
+
+## 6. Инвентарь и `become`
+
+- **`production.yml` и `timeweb.yml`** оба объявляют хост с именем `server` под ключом
+  `ungrouped:`. Хост-специфичные данные (`application_dir`, `mount_external_storage`,
+  `ansible_host`, `ansible_user`) вписаны инлайн. Конвенциональнее — `host_vars/server.yml`,
+  хосты в именованной группе. Два инвентаря с одинаковым именем хоста + `hosts: all` =
+  ошибка `-i` молча уедет не туда.
+- `ansible_become: true` глобально в инвентаре — всё бежит под root. Для личного сервера
+  прагматично; точечный `become`/`become_user` ближе к наименьшим привилегиям. Низкий приоритет.
+
+## 7. Конкретный баг
+
+- **`playbook-wanderer.yml:2`** — `name: "Configure gramps application"`, хотя
+  `app_name: "wanderer"`. Копипаст из gramps, поправить имя play.
+
+## 8. Мелочи стиля и конфигурации
+
+- **sudoers**: `playbook-backups.yml:52-59` правит `/etc/sudoers` через `lineinfile`.
+  Конвенция — отдельный файл в `/etc/sudoers.d/` (через `copy`/`template` с
+  `validate: visudo -cf %s`), а не модификация центрального файла.
+- **`.ansible-lint.yml`** содержит только `exclude_paths` — профиль не задан явно.
+  AGENTS.md утверждает «профиль production»; либо прописать `profile: production`, либо
+  поправить документацию.
+- **`ansible.cfg`** минимален. Стоит добавить `stdout_callback = yaml`,
+  `interpreter_python = auto_silent`, `force_handlers = true`.
+- Несогласованные кавычки и пути: `'directory'` vs `"directory"`, `src: "./files/..."` vs
+  `src: "files/..."`, одинарные кавычки в `playbook-all-setup.yml` против двойных в остальных.
+- `playbook-system.yml:24` — `apt` без `cache_valid_time`, обновляет кэш каждый прогон.
+
+## Приоритеты
+
+1. **#3 handlers** — убирает безусловный рестарт; внедряется по одному сервису.
+2. **#1 роль `backup`** — самый чистый шов для extraction; обкатать на одном сервисе.
+3. **#4, #7** — быстрые точечные фиксы без структурных изменений.
+4. **#2 group_vars** — убирает boilerplate; низкий риск.
+5. **#5, #6, #8** — фоновая зачистка стиля и структуры.

files/backups/backup-all.py

@@ -6,18 +6,19 @@ then creates restic backups and sends notifications.
 """
 
 import itertools
-import os
-import sys
-import subprocess
 import logging
+import os
 import pwd
+import subprocess
+import sys
 import time
+import tomllib
 from abc import ABC
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Dict, List, Optional, Any
+from typing import Any, Dict, List, Optional
+
 import requests
-import tomllib
 
 # Default config path
 CONFIG_PATH = Path("/etc/backup/config.toml")
@@ -54,6 +55,12 @@ class Application:
     backup_targets: List[Path]
 
 
+@dataclass
+class BackupResult:
+    success: bool
+    error: Optional[str] = None
+
+
 @dataclass
 class StorageRunResult:
     name: str
@@ -76,7 +83,7 @@ def format_duration(seconds: float) -> str:
 class Storage(ABC):
     name: str
 
-    def backup(self, backup_dirs: List[str]) -> bool:
+    def backup(self, backup_dirs: List[str]) -> BackupResult:
         """Backup directories"""
         raise NotImplementedError()
 
@@ -101,17 +108,17 @@ class ResticStorage(Storage):
                 f"Missing storage configuration values for backend ResticStorage: '{self.name}'"
             )
 
-    def backup(self, backup_dirs: List[str]) -> bool:
+    def backup(self, backup_dirs: List[str]) -> BackupResult:
         if not backup_dirs:
             logger.warning("No backup directories found")
-            return True
+            return BackupResult(success=True)
         try:
             return self.__backup_internal(backup_dirs)
         except Exception as exc:  # noqa: BLE001
             logger.error("Restic backup process failed: %s", exc)
-            return False
+            return BackupResult(success=False, error=str(exc))
 
-    def __backup_internal(self, backup_dirs: List[str]) -> bool:
+    def __backup_internal(self, backup_dirs: List[str]) -> BackupResult:
         logger.info("Starting restic backup for storage '%s'", self.name)
         logger.info("Destination: %s", self.restic_repository)
 
@@ -120,25 +127,13 @@ class ResticStorage(Storage):
         env["RESTIC_PASSWORD"] = self.restic_password
         env.update(self.env)
 
-        backup_cmd = ["restic", "backup", "--verbose"] + backup_dirs
-        result = subprocess.run(backup_cmd, env=env, capture_output=True, text=True)
-
-        if result.returncode != 0:
-            logger.error("Restic backup failed: %s", result.stderr)
-            return False
-
-        logger.info("Restic backup completed successfully")
-
         check_cmd = ["restic", "check"]
-        result = subprocess.run(check_cmd, env=env, capture_output=True, text=True)
+        steps = [
-
+            ("backup", ["restic", "backup", "--verbose"] + backup_dirs),
-        if result.returncode != 0:
+            ("check", check_cmd),
-            logger.error("Restic check failed: %s", result.stderr)
+            (
-            return False
+                "forget/prune",
-
+                [
-        logger.info("Restic check completed successfully")
-
-        forget_cmd = [
                     "restic",
                     "forget",
                     "--compact",
@@ -147,23 +142,31 @@ class ResticStorage(Storage):
                     "90",
                     "--keep-monthly",
                     "36",
+                ],
+            ),
+            ("final check", check_cmd),
         ]
-        result = subprocess.run(forget_cmd, env=env, capture_output=True, text=True)
+
+        for step, cmd in steps:
+            error = self.__run_step(step, cmd, env)
+            if error is not None:
+                return BackupResult(success=False, error=f"restic {step}: {error}")
+
+        return BackupResult(success=True)
+
+    def __run_step(
+        self, step: str, cmd: List[str], env: Dict[str, str]
+    ) -> Optional[str]:
+        """Run a single restic command. Return None on success or error text."""
+        result = subprocess.run(cmd, env=env, capture_output=True, text=True)
 
         if result.returncode != 0:
-            logger.error("Restic forget/prune failed: %s", result.stderr)
+            error = result.stderr.strip() or result.stdout.strip() or "no output"
-            return False
+            logger.error("Restic %s failed: %s", step, error)
+            return error
 
-        logger.info("Restic forget/prune completed successfully")
+        logger.info("Restic %s completed successfully", step)
-
+        return None
-        result = subprocess.run(check_cmd, env=env, capture_output=True, text=True)
-
-        if result.returncode != 0:
-            logger.error("Final restic check failed: %s", result.stderr)
-            return False
-
-        logger.info("Final restic check completed successfully")
-        return True
 
 
 class Notifier(ABC):
@@ -357,12 +360,12 @@ class BackupManager:
                 logger.error(
                     "Storage '%s' raised an unexpected error: %s", storage.name, exc
                 )
-                backup_result = False
+                backup_result = BackupResult(success=False, error=str(exc))
             storage_duration = time.monotonic() - storage_start
             self.storage_results.append(
                 StorageRunResult(
                     name=storage.name,
-                    success=backup_result,
+                    success=backup_result.success,
                     duration=storage_duration,
                 )
             )
@@ -370,13 +373,16 @@ class BackupManager:
                 "Storage '%s' finished in %s (success=%s)",
                 storage.name,
                 format_duration(storage_duration),
-                backup_result,
+                backup_result.success,
             )
-            if not backup_result:
+            if not backup_result.success:
-                self.errors.append(f"Storage '{storage.name}' backup failed")
+                error_msg = f"Storage '{storage.name}' backup failed"
+                if backup_result.error:
+                    error_msg += f": {backup_result.error}"
+                self.errors.append(error_msg)
 
             # Determine overall success
-            overall_success = overall_success and backup_result
+            overall_success = overall_success and backup_result.success
 
         # Send notification
         self._send_notification(overall_success)

files/memos/docker-compose.template.yml

@@ -3,7 +3,7 @@
 services:
 
   memos_app:
-    image: neosmemo/memos:0.29.0
+    image: neosmemo/memos:0.29.1
     container_name: memos_app
     restart: unless-stopped
     user: "{{ owner_create_result.uid }}:{{ owner_create_result.group }}"

files/outline/docker-compose.template.yml

@@ -3,7 +3,7 @@ services:
   # See sample https://github.com/outline/outline/blob/main/.env.sample
 
   outline_app:
-    image: outlinewiki/outline:1.8.0
+    image: outlinewiki/outline:1.8.1
     container_name: outline_app
     user: "{{ owner_create_result.uid }}:{{ owner_create_result.group }}"
     restart: unless-stopped