From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 79E761FF0AB for ; Wed, 09 Sep 2026 14:32:11 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0A99A215AC; Wed, 09 Sep 2026 14:32:11 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH proxmox-backup 21/28] sync: pull: allow to set retention timestamp for synced snapshots From: Robert Obkircher To: Christian Ebner In-Reply-To: <20260813171002.809441-22-c.ebner@proxmox.com> References: <20260813171002.809441-1-c.ebner@proxmox.com> <20260813171002.809441-22-c.ebner@proxmox.com> Date: Wed, 09 Sep 2026 14:32:04 +0200 Message-Id: <178895712462.166875.8747321609651298391.b4-review@b4> X-Mailer: b4 0.16-dev X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1788957117880 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.557 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 3W76DJRZEU2ULDUQLKLGVSMFXF776JLS X-Message-ID-Hash: 3W76DJRZEU2ULDUQLKLGVSMFXF776JLS X-MailFrom: r.obkircher@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: > 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 > > 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, > worker_threads: Option, > decryption_keys: Option>, > + retention_timespan: Option, > rpcenv: &mut dyn RpcEnvironment, > ) -> Result { > 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, > /// Decryption key ids and configs to decrypt snapshots with matching key fingerprint > crypt_configs: Vec<(String, Arc)>, > + // Retention timestamp to be set for newly pulled snapshots on sync target, overwrites > + // existing retentions if present on source manifest. > + retain_until: Option, > } > > impl PullParameters { > @@ -99,6 +102,7 @@ impl PullParameters { > resync_corrupt: Option, > worker_threads: Option, > decryption_keys: Option>, > + retention_timespan: Option, > ) -> Result { > 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