Leonid Pershin
2005b00175
- Added `ollama-models.yaml` to .gitignore and implemented logic to copy it in gpu-rent.ps1 and gpu-rent.sh. - Enhanced env.example to include new variables for LLM runtime options and local watchdog configuration. - Updated CLI commands to support LLM options during setup and execution, including new flags for Ollama and llama.cpp. - Improved documentation in cli.md and README.md to reflect changes in LLM integration and local watchdog functionality. - Adjusted architecture and decisions documentation to clarify the role of LLMs and local watchdog in the system.
112 lines
5.9 KiB
Markdown
112 lines
5.9 KiB
Markdown
# Project Review — 2026-08-21 (1)
|
||
|
||
## Prior Reviews Summary
|
||
|
||
> Based on the last 3 review files analysed in Phase 0.
|
||
|
||
### Still Open (carried forward)
|
||
None.
|
||
|
||
### Resolved Since Last Review
|
||
None.
|
||
|
||
---
|
||
|
||
## Phase 1: Code Quality
|
||
|
||
### SOLID
|
||
- `session.cmd_up` / `_bind_access` orchestrate cloud, SSH, bootstrap, provision, snapshot, notify — god-flow (`src/gpu_rent/session.py`).
|
||
- Docs name separate modules (`civitai_seed`, `models_push`, `git_seed`, `watchdog`, `reconcile`) that do not exist as packages; logic is folded into `provision.py`, `sync_files.py`, `tunnel.py`, `cli.status`.
|
||
- `Config` dataclass is a wide bag of unrelated settings (auth, paths, idle, autocomplete, civitai).
|
||
|
||
### Performance
|
||
- `push_tree` / `pull_tree`: per-file `remote_sha256` + `put_file` each open a new SSH (`sync_files.py`, `ssh_ops.py`).
|
||
- Every `up` re-runs full `bootstrap.sh` (apt-get) via `_bind_access` even when already bootstrapped.
|
||
- `scan_pools` on every `up`: extra Keystone authorize + per-flavor HTTP extra_specs (ThreadPool 16).
|
||
- Tunnel watchdog: `connect()` / Keystone authorize every ~30s.
|
||
|
||
### Correctness & Bugs
|
||
- `wait_ssh` aborts after 3 `AuthenticationException`s (~15s) while cloud-init may still be injecting keys (`ssh_ops.py` ~114–120).
|
||
- `cmd_stop` leaves `bootstrapped=True` (`session.py` ~397–400) → next ACTIVE path mis-labeled / wrong branch when FIP cleared.
|
||
- `phase` set to `ready_cloud` before bootstrap finishes (`session.py` ~344–346).
|
||
- Application credential created without role/access_rules restriction (`idle_killer.py` ~52–57) vs architecture “delete/shelve only”.
|
||
- `GIT_TOKEN` embedded in git remote URL and never stripped (`clone_ext.py` `with_token` / clone).
|
||
- SG rules accumulate old CIDRs; never prune (`cloud.py` `ensure_security_group`).
|
||
- Session tests out of sync: no `bootstrapped`+FIP setup for “second GPU”; `run_bootstrap` mock missing `update=` (`tests/test_session.py`).
|
||
|
||
### Code Quality
|
||
- State machine phases in docs (`bootstrapping`, `seeding_*`, `waiting_ui`) never written by code (only `idle` / `provisioning` / `ready_*`).
|
||
- `os_client.connect` reports `app_version="0.1.0"` while package is `0.2.0`.
|
||
- Duplicated SwarmUI busy/Idle polling logic in `ready.py` and `remote/idle_killer.py`.
|
||
|
||
---
|
||
|
||
## Phase 2: Logical Consistency
|
||
|
||
### Domain & Application Layer
|
||
- Architecture table invents module names; actual layout is flatter (`cli` → `session`/`doctor`/`tunnel` → `cloud`/`provision`/`ssh_ops`).
|
||
|
||
### Data Flow
|
||
- Config sources: `.env` (dotenv override=False) then `gpu-rent.vars` (override empty only) — OK.
|
||
- Volume IDs: `state` preferred over `BOOT_VOLUME_ID` / `DATA_VOLUME_ID` env — OK; AZ not re-validated on reuse.
|
||
|
||
### State Management
|
||
- `bootstrapped` not cleared on `stop`.
|
||
- `bootstrapped` True only at end of `_bind_access`; failure mid-bind leaves `ready_cloud` + `bootstrapped=False`.
|
||
- Marker files on VM (`/opt/swarmui/.gpu-rent-bootstrapped`) independent of local `state.bootstrapped`.
|
||
|
||
### Consistency
|
||
- Error handling: mostly `GpuRentError` / `CloudError`; some soft-log (`wait_backend_idle`, snapshot, idle-killer arm).
|
||
- Civitai bad token is **blocking** in doctor → blocks `up` even if user only needs default SwarmUI models.
|
||
|
||
---
|
||
|
||
## Phase 3: UI/UX (CLI)
|
||
|
||
### Usability
|
||
- `up` always prints full doctor table (noisy).
|
||
- Invalid/expired `CIVITAI_API_TOKEN` blocks entire session start.
|
||
- `destroy` has no `--no-pull` (always respects `PULL_OUTPUT` via `cmd_stop`).
|
||
|
||
### Visual & Consistency
|
||
- N/A (CLI). Mixed RU/EN messages by design.
|
||
|
||
### Interaction & Feedback
|
||
- Confirm on `up` without `--yes` is good; `--yes` skips.
|
||
- Tunnel Ctrl+C messaging is clear.
|
||
- `logs` only tails cloud-init + `is-active`, not `journalctl -u swarmui` as docs imply.
|
||
|
||
### Accessibility
|
||
- N/A.
|
||
|
||
---
|
||
|
||
## Full exploration notes (sections 1–10)
|
||
|
||
See chat report for narrative. Tasks below are actionable.
|
||
|
||
---
|
||
|
||
## Tasks
|
||
|
||
- [x] 1. [Bug] Soften `wait_ssh` early auth abort (retry until timeout / distinguish “sshd up, keys not ready”) — `src/gpu_rent/ssh_ops.py` line 114
|
||
- [x] 2. [Bug] Clear `bootstrapped=False` (and align phase) in `cmd_stop` — `src/gpu_rent/session.py` line 397
|
||
- [x] 3. [Security] Restrict idle-killer application credential (access_rules / roles delete-only) — `src/gpu_rent/idle_killer.py` line 52
|
||
- [x] 4. [Security] Strip `GIT_TOKEN` from git `origin` after clone/fetch — `src/gpu_rent/remote/clone_ext.py` line 30
|
||
- [x] 5. [Security] Stop accumulating stale SSH SG CIDRs; replace or prune old /32 — `src/gpu_rent/cloud.py` line 154
|
||
- [x] 6. [Security] Revisit default `GPU_RENT_SSH_CIDR=0.0.0.0/0` in `env.example` — `env.example` line 16
|
||
- [x] 7. [Performance] Reuse one SSH/SFTP session in `push_tree` / `pull_tree` / hash checks — `src/gpu_rent/sync_files.py` line 15
|
||
- [x] 8. [Performance] Skip full bootstrap (or apt) when VM already marked bootstrapped — `src/gpu_rent/session.py` line 87
|
||
- [x] 9. [Logic] Do not set `phase=ready_cloud` before `_bind_access` succeeds — `src/gpu_rent/session.py` line 344
|
||
- [x] 10. [Logic] Make doctor Civitai failure non-blocking when seed is optional — `src/gpu_rent/doctor.py` line 271
|
||
- [x] 11. [CodeQuality] Fix `tests/test_session.py` mocks (`bootstrapped`+FIP; `run_bootstrap(..., update=)`) — `tests/test_session.py` line 40
|
||
- [x] 12. [UX] `logs` should include `journalctl -u swarmui` as docs claim — `src/gpu_rent/cli.py` line 404
|
||
- [x] 13. [Logic] Update `docs/architecture.md` module names, phases, questionary, hold `--minutes` wording — `docs/architecture.md` line 37
|
||
|
||
## Closure notes (2026-08-21)
|
||
|
||
- `wait_ssh`: auth streak give-up after 180s continuous AuthenticationException (not 3 attempts).
|
||
- Idle-killer: access_rules for GET/DELETE server; fallback without rules if Keystone rejects.
|
||
- Bootstrap light path: `GPU_RENT_BOOTSTRAP_LIGHT=1` skips apt when marker present.
|
||
- Also fixed `os_client` `app_version` → package `__version__` (noted in Phase 1, not a numbered task).
|