Re: [PATCH proxmox-backup v5 1/3] api: move statefile loading into compute_schedule_status

Christian Ebner
2026-04-16T11:06:57Z

tiny nit, but fixing this can be a followup when applying.

On 4/13/26 3:19 PM, Michael Köppl wrote:
> Centralize loading of the job statefiles in compute_schedule_status,
> reducing code duplication across the job management API endpoints.
>
> This also changes the error handling for UPID parsing errors with
> garbage collection state files, aligning it to the rest of the API
> handler behavior.
>
> Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> src/api2/admin/datastore.rs | 15 ++++++---------
> src/api2/admin/prune.rs | 9 +++------
> src/api2/admin/sync.rs | 9 +++------
> src/api2/admin/verify.rs | 9 +++------
> src/api2/tape/backup.rs | 9 +++------
> src/server/jobstate.rs | 8 ++++++--
> 6 files changed, 24 insertions(+), 35 deletions(-)
>
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index fcb81ec5b..757b31145 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1172,19 +1172,14 @@ pub fn garbage_collection_status(
>
> let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::Read))?;
> let status_in_memory = datastore.last_gc_status();
> - let state_file = JobState::load("garbage_collection", &store)
> - .map_err(|err| log::error!("could not open GC statefile for {store}: {err}"))
> - .ok();
>
> let mut last = proxmox_time::epoch_i64();
>
> + let jobtype = "garbage_collection";
> +
> if let Some(ref upid) = status_in_memory.upid {
> - let mut computed_schedule: JobScheduleStatus = JobScheduleStatus::default();
> - if let Some(state) = state_file {
> - if let Ok(cs) = compute_schedule_status(&state, Some(upid)) {
> - computed_schedule = cs;
> - }
> - }
> + let computed_schedule: JobScheduleStatus =
> + compute_schedule_status(jobtype, &store, Some(upid))?;
>
> if let Some(endtime) = computed_schedule.last_run_endtime {
> last = endtime;
> @@ -1196,6 +1191,8 @@ pub fn garbage_collection_status(
> info.next_run = computed_schedule.next_run;
> info.last_run_endtime = computed_schedule.last_run_endtime;
> info.last_run_state = computed_schedule.last_run_state;
> + } else if let Err(err) = JobState::load(jobtype, &store) {
> + log::error!("could not open statefile for {store}: {err}");
> }
>
> info.next_run = info
> diff --git a/src/api2/admin/prune.rs b/src/api2/admin/prune.rs
> index a5ebf2975..1b1d2f1ba 100644
> --- a/src/api2/admin/prune.rs
> +++ b/src/api2/admin/prune.rs
> @@ -1,6 +1,6 @@
> //! Datastore Prune Job Management
>
> -use anyhow::{format_err, Error};
> +use anyhow::Error;
> use serde_json::Value;
>
> use proxmox_router::{
> @@ -18,7 +18,7 @@ use pbs_config::CachedUserInfo;
>
> use crate::server::{
> do_prune_job,
> - jobstate::{compute_schedule_status, Job, JobState},
> + jobstate::{compute_schedule_status, Job},
> };
>
> #[api(
> @@ -73,10 +73,7 @@ pub fn list_prune_jobs(
> let mut list = Vec::new();
>
> for job in job_config_iter {
> - let last_state = JobState::load("prunejob", &job.id)
> - .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> - let mut status = compute_schedule_status(&last_state, Some(&job.schedule))?;
> + let mut status = compute_schedule_status("prunejob", &job.id, Some(&job.schedule))?;
> if job.disable {
> status.next_run = None;
> }
> diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs
> index 6722ebea0..2384ede75 100644
> --- a/src/api2/admin/sync.rs
> +++ b/src/api2/admin/sync.rs
> @@ -1,6 +1,6 @@
> //! Datastore Synchronization Job Management
>
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Error};
> use serde::{Deserialize, Serialize};
> use serde_json::Value;
>
> @@ -19,7 +19,7 @@ use pbs_config::CachedUserInfo;
>
> use crate::{
> api2::config::sync::{check_sync_job_modify_access, check_sync_job_read_access},
> - server::jobstate::{compute_schedule_status, Job, JobState},
> + server::jobstate::{compute_schedule_status, Job},
> server::sync::do_sync_job,
> };
>
> @@ -112,10 +112,7 @@ pub fn list_config_sync_jobs(
> continue;
> }
>
> - let last_state = JobState::load("syncjob", &job.id)
> - .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> - let status = compute_schedule_status(&last_state, job.schedule.as_deref())?;
> + let status = compute_schedule_status("syncjob", &job.id, job.schedule.as_deref())?;
>
> list.push(SyncJobStatus {
> config: job,
> diff --git a/src/api2/admin/verify.rs b/src/api2/admin/verify.rs
> index 66695236c..af5b7fff4 100644
> --- a/src/api2/admin/verify.rs
> +++ b/src/api2/admin/verify.rs
> @@ -1,6 +1,6 @@
> //! Datastore Verify Job Management
>
> -use anyhow::{format_err, Error};
> +use anyhow::Error;
> use serde_json::Value;
>
> use proxmox_router::{
> @@ -19,7 +19,7 @@ use pbs_config::CachedUserInfo;
>
> use crate::server::{
> do_verification_job,
> - jobstate::{compute_schedule_status, Job, JobState},
> + jobstate::{compute_schedule_status, Job},
> };
>
> #[api(
> @@ -73,10 +73,7 @@ pub fn list_verification_jobs(
> let mut list = Vec::new();
>
> for job in job_config_iter {
> - let last_state = JobState::load("verificationjob", &job.id)
> - .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> - let status = compute_schedule_status(&last_state, job.schedule.as_deref())?;
> + let status = compute_schedule_status("verificationjob", &job.id, job.schedule.as_deref())?;
>
> list.push(VerificationJobStatus {
> config: job,
> diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
> index c254c6d8b..cd68fe279 100644
> --- a/src/api2/tape/backup.rs
> +++ b/src/api2/tape/backup.rs
> @@ -1,6 +1,6 @@
> use std::sync::{Arc, Mutex};
>
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Error};
> use serde_json::Value;
> use tracing::{info, warn};
>
> @@ -23,7 +23,7 @@ use pbs_datastore::{DataStore, StoreProgress};
> use crate::tape::{assert_datastore_type, TapeNotificationMode};
> use crate::{
> server::{
> - jobstate::{compute_schedule_status, Job, JobState},
> + jobstate::{compute_schedule_status, Job},
> TapeBackupJobSummary,
> },
> tape::{
> @@ -97,10 +97,7 @@ pub fn list_tape_backup_jobs(
> continue;
> }
>
> - let last_state = JobState::load("tape-backup-job", &job.id)
> - .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
> -
> - let status = compute_schedule_status(&last_state, job.schedule.as_deref())?;
> + let status = compute_schedule_status("tape-backup-job", &job.id, job.schedule.as_deref())?;
>
> let next_run = status.next_run.unwrap_or(current_time);
>
> diff --git a/src/server/jobstate.rs b/src/server/jobstate.rs
> index dc9f6c90d..ceac8dde8 100644
> --- a/src/server/jobstate.rs
> +++ b/src/server/jobstate.rs
> @@ -301,11 +301,15 @@ impl Job {
> }
>
> pub fn compute_schedule_status(
> - job_state: &JobState,
> + jobtype: &str,
> + jobname: &str,

since these parameters are now passed along instead of &JobState...

> schedule: Option<&str>,
> ) -> Result<JobScheduleStatus, Error> {
> + let job_state = JobState::load(jobtype, jobname)

... and this binding therefore now is of type JobState ...

> + .map_err(|err| format_err!("could not open statefile for {jobname}: {err}"))?;
> +
> let (upid, endtime, state, last) = match job_state {
> - JobState::Created { time } => (None, None, None, *time),
> + JobState::Created { time } => (None, None, None, time),
> JobState::Started { upid } => {
> let parsed_upid: UPID = upid.parse()?;
> (Some(upid), None, None, parsed_upid.starttime)

... this introduces a clippy warning "useless conversion to the same
type: `std::string::String`" on line 333:

last_run_upid: upid.map(String::from),

since that is now no longer a &String.