Re: [PATCH proxmox-backup 21/28] sync: pull: allow to set retention timestamp for synced snapshots

[Robert Obkircher <r.obkircher@proxmox.com>]

> Allow the pull sync job to set the retention timestamp when pulling
> the snapshot from the remote.
> 
> The timestamp is calculated based on the start time of the sync job
> by pull parameter construction.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index 6b53c55b7..c197c161f 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -10,8 +10,8 @@ use pbs_api_types::{
>      Authid, BackupNamespace, CRYPT_KEY_ID_SCHEMA, DATASTORE_SCHEMA, GROUP_FILTER_LIST_SCHEMA,
>      GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_BACKUP,
>      PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
> -    RESYNC_CORRUPT_SCHEMA, RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA, SYNC_VERIFIED_ONLY_SCHEMA,
> -    SYNC_WORKER_THREADS_SCHEMA, SyncJobConfig, TRANSFER_LAST_SCHEMA,
> +    RESYNC_CORRUPT_SCHEMA, RateLimitConfig, RetentionTimespan, SYNC_ENCRYPTED_ONLY_SCHEMA,
> +    SYNC_VERIFIED_ONLY_SCHEMA, SYNC_WORKER_THREADS_SCHEMA, SyncJobConfig, TRANSFER_LAST_SCHEMA,
>  };
>  use pbs_config::CachedUserInfo;
>  use proxmox_rest_server::WorkerTask;
> @@ -93,6 +93,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
>              sync_job.resync_corrupt,
>              sync_job.worker_threads,
>              sync_job.associated_key.clone(),
> +            sync_job.retention_timespan.clone(),
>          )
>      }
>  }
> @@ -162,6 +163,10 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
>                  },
>                  optional: true,
>              },
> +            "retention-timespan": {
> +                type: RetentionTimespan,
> +                optional: true,
> +            },
>          },
>      },
>      access: {
> @@ -191,6 +196,7 @@ async fn pull(
>      resync_corrupt: Option<bool>,
>      worker_threads: Option<usize>,
>      decryption_keys: Option<Vec<String>>,
> +    retention_timespan: Option<RetentionTimespan>,
>      rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<String, Error> {
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> @@ -233,6 +239,7 @@ async fn pull(
>          resync_corrupt,
>          worker_threads,
>          decryption_keys,
> +        retention_timespan,
>      )?;
>  
>      // fixme: set to_stdout to false?
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 856e0fc5a..816e64ce7 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -23,7 +23,7 @@ use pbs_api_types::{
>      ArchiveType, Authid, BackupDir, BackupGroup, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode,
>      Fingerprint, GroupFilter, MANIFEST_BLOB_NAME, MAX_NAMESPACE_DEPTH, Operation,
>      PRIV_DATASTORE_APPEND, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, RateLimitConfig, Remote,
> -    SnapshotListItem, VerifyState, print_store_and_ns,
> +    RetentionTimespan, SnapshotListItem, VerifyState, print_store_and_ns,
>  };
>  use pbs_client::BackupRepository;
>  use pbs_config::CachedUserInfo;
> @@ -77,6 +77,9 @@ pub(crate) struct PullParameters {
>      worker_threads: Option<usize>,
>      /// Decryption key ids and configs to decrypt snapshots with matching key fingerprint
>      crypt_configs: Vec<(String, Arc<CryptConfig>)>,
> +    // Retention timestamp to be set for newly pulled snapshots on sync target, overwrites
> +    // existing retentions if present on source manifest.
> +    retain_until: Option<i64>,
>  }
>  
>  impl PullParameters {
> @@ -99,6 +102,7 @@ impl PullParameters {
>          resync_corrupt: Option<bool>,
>          worker_threads: Option<usize>,
>          decryption_keys: Option<Vec<String>>,
> +        retention_timespan: Option<RetentionTimespan>,
>      ) -> Result<Self, Error> {
>          if let Some(max_depth) = max_depth {
>              ns.check_max_depth(max_depth)?;
> @@ -140,6 +144,10 @@ impl PullParameters {
>  
>          let group_filter = group_filter.unwrap_or_default();
>  
> +        let retain_until = retention_timespan
> +            .map(|timespan| timespan.to_timestamp_from_systemtime())
> +            .transpose()?;
> +
>          let crypt_configs = if let Some(key_ids) = &decryption_keys {
>              let mut crypt_configs = Vec::with_capacity(key_ids.len());
>              for key_id in key_ids {
> @@ -165,6 +173,7 @@ impl PullParameters {
>              resync_corrupt,
>              worker_threads,
>              crypt_configs,
> +            retain_until,
>          })
>      }
>  }
> @@ -768,7 +777,14 @@ async fn pull_snapshot<'a>(
>      )
>      .await?
>      {
> -        (None, false) => (None, None), // regular pull without decryption
> +        (None, false) => {
> +            let new_manifest = if params.retain_until.is_some() {
> +                Some(Arc::new(Mutex::new(BackupManifest::new(snapshot.into()))))
> +            } else {
> +                None
> +            };

Bug: BackupManifest::new does not copy the list of files and 
pull_single_archive currently only calls add_to_decrypted_manifest for
the encrypted case. (I noticed that the files list in the manifest was
empty after getting "refusing to overwrite local snapshot: content
differs from source" in a repeated pull sync, where I had manually
removed the retention timestamp of the target.)


Other thoughts:

If the manifest already contains a retain_until but the param is None,
do we just keep using the old value? Do we need a max retention check
in that case or is it safe to trust the manifest?

Further up in this method there are also a fast paths for
`manifest_blob.raw_data() == tmp_manifest_blob.raw_data()` and
`ignore_not_verified_or_encrypted`. If you pull sync with a newly added
retain_until parameter it will be effectively ignored in those cases,
which may be unexpected.

I wonder if it would make sense to store a duration instead of a
timestamp. If we then interpret it relative to mtime that would
automatically "renew" on every sync without manifest modifications.

-- 
Robert Obkircher <r.obkircher@proxmox.com>