Re: [pbs-devel] [PATCH proxmox-backup] api2: add keep-job-configs flag to datastore remove endpoint

[Dominik Csapak <d.csapak@proxmox.com>]

high level comments:

this changes the current behaviour of that call, so we should
document this in the release notes/changelog explicitely
else some users might wonder where their jobs are ...

was probably not on your radar, but tape backup jobs would
also fit in here

remaining comments inline

On 4/28/21 12:25, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> Adds a flag that controls whether related sync and verify-jobs should
> be kept or deleted. If not set or set to false, job configs will be removed
> if set to true, job config files will be left untouched and may be used
> in a new datastore with the same name.
> 
>   src/api2/config/datastore.rs | 30 +++++++++++++++++++++++++++---
>   1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 6ca98b53..f9794e7f 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -1,13 +1,15 @@
>   use std::path::PathBuf;
>   
>   use anyhow::{bail, Error};
> -use serde_json::Value;
> +use serde_json::{Value, json};
>   use ::serde::{Deserialize, Serialize};
>   
>   use proxmox::api::{api, Router, RpcEnvironment, Permission};
>   use proxmox::api::schema::parse_property_string;
>   use proxmox::tools::fs::open_file_locked;
>   
> +use crate::api2::config::sync::{list_sync_jobs, delete_sync_job};
> +use crate::api2::config::verify::{list_verification_jobs, delete_verification_job};
>   use crate::api2::types::*;
>   use crate::backup::*;
>   use crate::config::cached_user_info::CachedUserInfo;
> @@ -392,6 +394,12 @@ pub fn update_datastore(
>               name: {
>                   schema: DATASTORE_SCHEMA,
>               },
> +            "keep-job-configs": {
> +                description: "If enabled, the job configurations related to this datastore will be kept.",
> +                type: bool,
> +                optional: true,
> +                default: false,
> +            },
>               digest: {
>                   optional: true,
>                   schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
> @@ -403,8 +411,12 @@ pub fn update_datastore(
>       },
>   )]
>   /// Remove a datastore configuration.
> -pub fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Error> {
> -
> +pub fn delete_datastore(
> +    name: String,
> +    keep_job_configs: Option<bool>,

if the api schema has a 'default', you can omit the 'Option' and
it will be magically set to the default. this makes
the code below much nicer since you can simply do

if !keep_job_configs {
     ...
}

> +    digest: Option<String>,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<(), Error> {
>       let _lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
>   
>       let (mut config, expected_digest) = datastore::config()?;
> @@ -419,6 +431,18 @@ pub fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Erro
>           None => bail!("datastore '{}' does not exist.", name),
>       }
>   
> +    match keep_job_configs {
> +        Some(true) => {}
> +        _ => { // not set or set to false
> +            for job_config in list_verification_jobs(json!({ "store": name }), rpcenv)? {
> +                delete_verification_job(job_config.id, None, rpcenv)?
> +            }
> +            for job_config in list_sync_jobs(json!({ "store": name }), rpcenv)? {
> +                delete_sync_job(job_config.id, None, rpcenv)?
> +            }

is there some other patch missing? you call 'list_verification_jobs' 
with a parameter, but that api call does not do anything with them?
afaics, this would remove all sync and verification jobs

or did you mean to call crate::api2::admin::sync::list_sync_jobs ?

note the difference of 'api2::admin::' and 'api2::config'

> +        }
> +    }
> +
>       datastore::save_config(&config)?;
>   
>       // ignore errors
>