Add ansible playbooks review

2026-06-09 10:17:32 +03:00
parent 94b09be53c
commit a50b399a85
1 changed files with 150 additions and 0 deletions
@@ -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** — фоновая зачистка стиля и структуры.