From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id A1AAD1FF185 for ; Mon, 21 Jul 2025 16:47:39 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A066712EE4; Mon, 21 Jul 2025 16:48:50 +0200 (CEST) Mime-Version: 1.0 Date: Mon, 21 Jul 2025 16:48:17 +0200 Message-Id: From: "Hannes Laimer" To: "Christian Ebner" , "Proxmox Backup Server development discussion" , "Hannes Laimer" X-Mailer: aerc 0.20.1-112-gd31995f1e20b References: <20250719125035.9926-1-c.ebner@proxmox.com> <20250719125035.9926-40-c.ebner@proxmox.com> <27b3c6fe-7264-4056-8f7a-fbb0bb8febfe@proxmox.com> In-Reply-To: <27b3c6fe-7264-4056-8f7a-fbb0bb8febfe@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1753109289821 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.025 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup v9 36/46] api/datastore: implement refresh endpoint for stores with s3 backend X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Cc: pbs-devel Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On Mon Jul 21, 2025 at 4:42 PM CEST, Christian Ebner wrote: > On 7/21/25 4:31 PM, Hannes Laimer wrote: >> On Mon Jul 21, 2025 at 4:26 PM CEST, Christian Ebner wrote: >>> On 7/21/25 4:16 PM, Hannes Laimer wrote: >>>> On Sat Jul 19, 2025 at 2:50 PM CEST, Christian Ebner wrote: >>>>> Allows to easily refresh the contents on the local cache store for >>>>> datastores backed by an S3 object store. >>>>> >>>>> In order to guarantee that no read or write operations are ongoing, >>>>> the store is first set into the maintenance mode `S3Refresh`. Objects >>>>> are then fetched into a temporary directory to avoid loosing contents >>>>> and consistency in case of an error. Once all objects have been >>>>> fetched, clears out existing contents and moves the newly fetched >>>>> contents in place. >>>>> >>>>> Signed-off-by: Christian Ebner >>>>> --- >>>>> changes since version 8: >>>>> - refactor s3 refresh into more compact methods >>>>> - drop un-necessary drop(_lock) >>>>> - use missing tokio::task::spawn_blocking context for blocking >>>>> maintenance mode setting >>>>> >>>>> pbs-datastore/src/datastore.rs | 175 +++++++++++++++++++++++++++++++++ >>>>> src/api2/admin/datastore.rs | 34 +++++++ >>>>> 2 files changed, 209 insertions(+) >>>>> >>>>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs >>>>> index a524d7b32..b2af05eac 100644 >>>>> --- a/pbs-datastore/src/datastore.rs >>>>> +++ b/pbs-datastore/src/datastore.rs >>>>> @@ -10,6 +10,7 @@ use anyhow::{bail, format_err, Context, Error}; >>>>> use http_body_util::BodyExt; >>>>> use nix::unistd::{unlinkat, UnlinkatFlags}; >>>>> use pbs_tools::lru_cache::LruCache; >>>>> +use tokio::io::AsyncWriteExt; >>>>> use tracing::{info, warn}; >>>>> >>>>> use proxmox_human_byte::HumanByte; >>>>> @@ -2200,4 +2201,178 @@ impl DataStore { >>>>> pub fn old_locking(&self) -> bool { >>>>> *OLD_LOCKING >>>>> } >>>>> + >>>>> + /// Set the datastore's maintenance mode to `S3Refresh`, fetch from S3 object store, clear and >>>>> + /// replace the local cache store contents. Once finished disable the maintenance mode again. >>>>> + /// Returns with error for other datastore backends without setting the maintenance mode. >>>>> + pub async fn s3_refresh(self: &Arc) -> Result<(), Error> { >>>>> + match self.backend()? { >>>>> + DatastoreBackend::Filesystem => bail!("store '{}' not backed by S3", self.name()), >>>>> + DatastoreBackend::S3(s3_client) => { >>>>> + let self_clone = Arc::clone(self); >>>>> + tokio::task::spawn_blocking(move || { >>>>> + self_clone.maintenance_mode(Some(MaintenanceMode { >>>>> + ty: MaintenanceType::S3Refresh, >>>>> + message: None, >>>>> + })) >>>>> + }) >>>>> + .await? >>>>> + .context("failed to set maintenance mode")?; >>>> >>>> I think we should hold the config lock, so it can't be changed while we >>>> refresh, no? >>> >>> Yes, but that is handled by the method itself, also to limit lock scope. >>> >>> See further below... >>> >> >> maybe I'm missing something, but the limited scope is what I mean. I >> think we should try to prevent changing the maintenance mode away from >> `S3Refresh` before we're done, so basically holding the lock while we >> refresh. > > No, the intention here is to allow the user to manually clear the > maintenance mode again, as the s3 refresh might fail. So the refresh > manually sets it to guarantee consistency and clears it if finished. > > If the lock on the config is kept for the whole refresh, than the user > will never be able to recover from an error. ACK, you is right! _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel