add JobModel: persistent scheduler job store (#2360, phase 1) - #2530
add JobModel: persistent scheduler job store (#2360, phase 1)#2530ebuzerdrmz44 wants to merge 6 commits into
Conversation
|
Reworked this from the bare model into something with an actual use. The three schema questions are decisions now. Profile and repo are plain strings like I also folded in the first consumer, skip-reason recording, so there's a real path through the table instead of an empty schema sitting there. The skip write goes through Ready for another look whenever you get a chance. |
|
@m3nu Since this is the first of a stack, here's the full shape of #2360 so it's clearer what this PR is (and isn't) trying to do. Roughly 4 phases,10-11 small PRs ( I broke my original roadmap in #2360 to make all of it easy to review 😆 ): Phase A : Persistence (the jobs store)
Phase B : Split VortaScheduler into State / Scheduling / Execution (PR4–6). The Execution one wires the job→result link and makes post-backup tasks run in order. Phase C : Reliability (PR7–8): the QTimer 24.8-day overflow, and sleep/resume detection on non-logind systems. Phase D : Jobs View UI (PR9–11): the Jobs page, filters, cancel/re-queue. I'm holding this whole phase until you weigh in on three open questions on the issue . This PR is deliberately the narrow shape-approval slice: get the table right, prove it works with one consumer, then stack the rest on top once you're happy with it. Everything downstream depends on this one merging first. |
m3nu
left a comment
There was a problem hiding this comment.
Thanks for folding in the first consumer — a table with a real write path is much easier to judge than a bare schema, and the phased breakdown on #2360 is genuinely helpful.
The overall shape is defensible: JobModel records intent (including the things that never produce a log line), EventLogModel stays the record of what executed, linked through the event_log FK. Plain strings for profile/repo_url mirror EventLogModel exactly. Your no-migration claim checks out — init_db calls DB.create_tables([...]) unconditionally with safe=True, so the table appears on fresh and existing DBs alike.
Requesting changes on three points, then this is good to build on.
Required
1. Failures are being recorded as skips. In create_backup, the JobModel.create(...) sits in the outer block covering both branches of the level == 'error' check. So a genuine failure — "Conditions for backup not met", repo unreachable, source missing — gets written with status='skipped' and the error text in skip_reason. That defeats the table's stated purpose of capturing intent, and it'll poison the Phase D Jobs View from day one. Use status='failed' when level == 'error'; keep skipped for the expected WiFi/metered/busy cases.
2. No retention policy → unbounded growth. init_db purges EventLogModel rows older than six months. JobModel gets no equivalent, so on a machine with an aggressive schedule and a flaky network the table only ever grows. Please add the purge alongside the existing one in this PR — retrofitting it after users have accumulated rows is strictly worse.
3. Extract the triplicated write. Three near-identical with db_lock: JobModel.create(...) blocks. Collapse into a _record_skip(self, profile, trigger, reason, status='skipped') helper on VortaScheduler. With ~10 more PRs stacked on this, it will not stay at three.
Cheap, should fix here
4. Move db_lock out of vorta.borg.borg_job. Importing a DB-serialization lock from the borg job module into the scheduler is a layering inversion — it belongs in vorta/store/. Every PR in the stack will want it, so move it now, while there's exactly one new importer.
5. Constrain the string enums. status, job_type and trigger are free-form CharFields written with bare literals. Module-level constants (or a small JobStatus class) in models.py will stop the eight downstream PRs drifting on 'skipped' vs 'skip'.
6. Missing test for the third write site. test_scheduler.py covers the prepare-failure and repo-busy skips, but not the catchup/network-down write in set_timer_for_profile.
Worth settling before the stack builds on this schema
7. profile stores the profile id as a string. Consistent with EventLogModel, so I'm not objecting — but the rationale in your comment ("a job's history sticks around even after its profile gets deleted") isn't actually achieved: what survives is a dangling numeric id with no name. If the Phase D Jobs View is meant to render history for deleted profiles, store profile_name alongside it.
8. Dead columns. scheduled_at, event_log and the scheduled/running/interrupted statuses are never written by production code — only by the test. Fine as a phase-1 slice given the roadmap, but say so explicitly in the description so a reviewer doesn't go hunting for the writer.
9. No index. The Jobs View will filter on profile/status/created_at. Adding it now avoids a migration later.
On your original three questions: agreed on plain strings (2), and yes, a startup sweep marking leftover running jobs as interrupted is enough for crash recovery (3). For (1), your instinct is right — keep next-run derivation in set_timer_for_profile as the single source of truth, and persist only skipped/interrupted/completed. Don't materialise scheduled rows.
Minor: this PR tracks #2360, not #2361 — worth keeping the cross-links straight since the two refactors are running in parallel.
|
want to flag this before you look again: I moved Narrowing the lock in |
…tLogModel so history survives profile/repo deletion
…th their reason, the first JobModel consumer
…up points as JobModel rows
69e58b8 to
43662e8
Compare
Description
First PR of the scheduler refactor (#2360), which I'm splitting into small stacked PRs (full breakdown is on the issue). This one just adds
JobModel; nothing reads or writes it yet.No migration needed.
connection.pybuilds tables withcreate_tables(), which is safe by default, so the table shows up on both fresh and existing DBs.The idea:
JobModeltracks a job's lifecycle and intent (scheduled, running, done, skipped, interrupted). When a job actually runs it links to the matchingEventLogModelrow throughevent_log, so I'm not duplicating start/end/returncode across two tables.EventLogModelstays the record of what executed, andJobModeladds the intent plus the things that never produce a log line, like a job that got skipped.A few schema questions before I build on it. The rest of the plan doesn't depend on these, but this table's shape does:
set_timer_for_profile. I lean toward persisting skipped and interrupted jobs while leaving next-run derivation as is.EventLogModeluses strings, so its rows survive a profile being deleted. Same for the jobs history, or is a FK fine here?@m3nu If you'd rather this not merge as a bare model, I can fold in the first consumer (skip-reason recording) before then.