Re: [pbs-devel] [PATCH proxmox-backup 3/8] config: add JobState helper

[Dietmar Maurer <dietmar@proxmox.com>]

comments inline

> On 07/31/2020 2:43 PM Dominik Csapak <d.csapak@proxmox.com> wrote:
> 
>  
> this is intended to be a generic helper to (de)serialize job states
> (e.g., sync, verify, and so on)
> 
> writes a json file into '/var/lib/proxmox-backup/jobstates/TYPE-ID.json'
> 
> the api creates the directory with the correct permissions, like
> the rrd directory
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/bin/proxmox-backup-api.rs |   1 +
>  src/config.rs                 |   1 +
>  src/config/jobstate.rs        | 126 ++++++++++++++++++++++++++++++++++
>  3 files changed, 128 insertions(+)
>  create mode 100644 src/config/jobstate.rs
> 
> diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
> index 9dde46c0..ea306cf0 100644
> --- a/src/bin/proxmox-backup-api.rs
> +++ b/src/bin/proxmox-backup-api.rs
> @@ -37,6 +37,7 @@ async fn run() -> Result<(), Error> {
>      config::update_self_signed_cert(false)?;
>  
>      proxmox_backup::rrd::create_rrdb_dir()?;
> +    proxmox_backup::config::jobstate::create_jobstate_dir()?;
>  
>      if let Err(err) = generate_auth_key() {
>          bail!("unable to generate auth key - {}", err);
> diff --git a/src/config.rs b/src/config.rs
> index e805c38e..f9e08814 100644
> --- a/src/config.rs
> +++ b/src/config.rs
> @@ -22,6 +22,7 @@ pub mod acl;
>  pub mod cached_user_info;
>  pub mod network;
>  pub mod sync;
> +pub mod jobstate;
>  
>  /// Check configuration directory permissions
>  ///
> diff --git a/src/config/jobstate.rs b/src/config/jobstate.rs
> new file mode 100644
> index 00000000..fb5a8cbf
> --- /dev/null
> +++ b/src/config/jobstate.rs
> @@ -0,0 +1,126 @@
> +use std::fs::File;
> +use std::path::{Path, PathBuf};
> +use std::time::Duration;
> +
> +use serde::{Serialize, Deserialize};
> +use anyhow::{bail, Error, format_err};
> +use proxmox::tools::fs::{file_read_optional_string, replace_file, create_path, CreateOptions};
> +
> +use crate::tools::{epoch_now_u64, open_file_locked};
> +
> +#[serde(rename_all="kebab-case")]
> +#[derive(Serialize,Deserialize)]
> +pub struct JobState {
> +    #[serde(skip_serializing_if="Option::is_none")]
> +    pub upid: Option<String>,
> +    pub starttime: i64,


Why is upid optional, and starttime not? Also, Upid contains the startime, so
why do we store twice?

Also, please add documentation to pub structs and functions.

> +    #[serde(skip_serializing_if="Option::is_none")]
> +    pub endtime: Option<i64>,
> +    #[serde(skip_serializing_if="Option::is_none")]
> +    pub state: Option<String>,
> +}
> +
> +const JOB_STATE_BASEDIR: &str = "/var/lib/proxmox-backup/jobstates";
> +
> +/// Create jobstate stat dir with correct permission
> +pub fn create_jobstate_dir() -> Result<(), Error> {
> +
> +    let backup_user = crate::backup::backup_user()?;
> +    let opts = CreateOptions::new()
> +        .owner(backup_user.uid)
> +        .group(backup_user.gid);
> +
> +    create_path(JOB_STATE_BASEDIR, None, Some(opts))
> +        .map_err(|err: Error| format_err!("unable to create rrdb stat dir - {}", err))?;
> +
> +    Ok(())
> +}
> +
> +fn get_path(jobtype: &str, jobname: &str) -> Result<PathBuf, Error> {
> +    let mut path = PathBuf::from(JOB_STATE_BASEDIR);
> +    path.push(format!("{}-{}.json", jobtype, jobname));
> +    Ok(path)
> +}
> +
> +fn get_lock<P>(path: P) -> Result<File, Error>
> +where
> +    P: AsRef<Path>
> +{
> +    let mut path = path.as_ref().to_path_buf();
> +    path.set_extension("lck");
> +    let lock = open_file_locked(path, Duration::new(10, 0))?;
> +    Ok(lock)
> +}
> +
> +pub fn remove_state_file(jobtype: &str, jobname: &str) -> Result<(), Error> {
> +    let path = get_path(jobtype, jobname)?;
> +    let _lock = get_lock(&path)?;
> +    match std::fs::remove_file(&path) {
> +        Ok(()) => Ok(()),
> +        Err(err) => {
> +            bail!("cannot remove statefile for {} - {}: {}", jobtype, jobname, err);
> +        }
> +    }
> +}
> +
> +impl JobState {
> +    pub fn new(upid: &str) -> Self {
> +        Self{
> +            upid: Some(upid.to_string()),
> +            starttime: epoch_now_u64().unwrap_or_else(|_| 0) as i64,

upid already contains a start time?

> +            endtime: None,
> +            state: None,
> +        }
> +    }
> +
> +    pub fn finish(&mut self, state: &str) -> Result<(), Error> {
> +        self.state = Some(state.to_string());
> +        self.endtime = Some(epoch_now_u64()? as i64);
> +        Ok(())
> +    }
> +
> +    pub fn try_read_or_create(jobtype: &str, jobname: &str) -> Result<Self, Error> {
> +        let path = get_path(jobtype, jobname)?;
> +
> +        let lock = get_lock(&path)?;
> +
> +        if let Some(state) = file_read_optional_string(path)? {
> +            Ok(serde_json::from_str(&state)?)
> +        } else {
> +            let state = Self{
> +                upid: None,
> +                endtime: None,
> +                state: None,
> +                starttime: epoch_now_u64()? as i64,
> +            };

why do we create a file with no info? Or is that now() relevant?

> +            state.write_state(jobtype, jobname, Some(lock))?;
> +            Ok(state)
> +        }
> +    }
> +
> +    pub fn write_state(&self, jobtype: &str, jobname: &str, lock: Option<File>) -> Result<(), Error> {

This is a strange pub API. I would remove parameter lock.

   fn write_state_no_lock(&self, path: &Path) -> Result<(), Error> {
        let serialized = serde_json::to_string(&self)?;
 
        let backup_user = crate::backup::backup_user()?;
        let mode = nix::sys::stat::Mode::from_bits_truncate(0o0644);
        // set the correct owner/group/permissions while saving file
        // owner(rw) = backup, group(r)= backup
        let options = CreateOptions::new()
            .perm(mode)
            .owner(backup_user.uid)
            .group(backup_user.gid);

        replace_file(
            path,
            serialized.as_bytes(),
            options,
        )
    }

    pub fn write_state(&self, jobtype: &str, jobname: &str) -> Result<(), Error> {
        let path = get_path(jobtype, jobname)?;

        let _lock = get_lock(&path)?;

        self.write_state_no_lock(&path)
    }

> +        let serialized = serde_json::to_string(&self)?;
> +        let path = get_path(jobtype, jobname)?;
> +
> +        let _lock = if let Some(lock) = lock {
> +            lock
> +        } else {
> +            get_lock(&path)?
> +        };
> +
> +        let backup_user = crate::backup::backup_user()?;
> +        let mode = nix::sys::stat::Mode::from_bits_truncate(0o0644);
> +        // set the correct owner/group/permissions while saving file
> +        // owner(rw) = backup, group(r)= backup
> +        let options = CreateOptions::new()
> +            .perm(mode)
> +            .owner(backup_user.uid)
> +            .group(backup_user.gid);
> +
> +        replace_file(
> +            path,
> +            serialized.as_bytes(),
> +            options,
> +        )
> +    }
> +}
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel