From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 46038A03DF for ; Wed, 8 Nov 2023 14:26:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2ECE785CC for ; Wed, 8 Nov 2023 14:26:12 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 8 Nov 2023 14:26:10 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 27E5F473E7 for ; Wed, 8 Nov 2023 14:26:10 +0100 (CET) Message-ID: <2d33b03e-5263-47bf-9070-d0ed7b1ab236@proxmox.com> Date: Wed, 8 Nov 2023 14:26:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox Backup Server development discussion References: <20231006140529.723988-1-h.laimer@proxmox.com> <20231006140529.723988-2-h.laimer@proxmox.com> Content-Language: en-US From: Hannes Laimer In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.007 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [pull.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup v5 1/6] api: make Remote for SyncJob optional 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: , X-List-Received-Date: Wed, 08 Nov 2023 13:26:12 -0000 On 11/8/23 11:53, Thomas Lamprecht wrote: > Am 06/10/2023 um 16:05 schrieb Hannes Laimer: >> diff --git a/src/api2/pull.rs b/src/api2/pull.rs >> index daeba7cf..9ed83046 100644 >> --- a/src/api2/pull.rs >> +++ b/src/api2/pull.rs > >> @@ -65,7 +75,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters { >> PullParameters::new( >> &sync_job.store, >> sync_job.ns.clone().unwrap_or_default(), >> - &sync_job.remote, >> + sync_job.remote.as_deref().unwrap_or("local"), > > isn't this still the same issue that Wolfgang asked about in his review for > v3 and v2? (Please be a bit more communicative and answer to review on list > too, else it's hard to know if you overlooked it or if it's actually OK the > way it is...) > > faking a remote ID is probably never a good idea, should this stay None here? > > But I actually checked the remaining code, and it fixes this in commit 4/6 > "pull: refactor pulling from a datastore", so question is, why do we add API > support before it can even correctly work? Shouldn't this commit be ordered > after the aforementioned refactor one, and the "pull: add support for pulling > from local datastore" one, avoiding such intermediate "bugs" and making review > easier due to a more causal order. > makes sense, making it optional felt like the first logical step, that's why they are ordered this way. But sure, can re-order. > >> &sync_job.remote_store, >> sync_job.remote_ns.clone().unwrap_or_default(), >> sync_job >> @@ -91,7 +101,7 @@ pub fn do_sync_job( >> ) -> Result { >> let job_id = format!( >> "{}:{}:{}:{}:{}", >> - sync_job.remote, >> + sync_job.remote.as_deref().unwrap_or("-"), >> sync_job.remote_store, >> sync_job.store, >> sync_job.ns.clone().unwrap_or_default(), >> @@ -99,6 +109,13 @@ pub fn do_sync_job( >> ); >> let worker_type = job.jobtype().to_string(); >> >> + if sync_job.remote.is_none() >> + && sync_job.store == sync_job.remote_store >> + && sync_job.ns == sync_job.remote_ns > > I'd disallow syncing to the same store even if different NS, as they still > might overlap and it's IMO just an odd use-case. > If, we should allow mocving around groups and namespaces inside the same > datastore as separate functionallity, without any jobs or syncing involved, > but that'd be unrelated to this series in any way. It is kinda odd, the idea was that currently its not possible to move backups within a datastore (using the UI). But yes, a sync job is probably not the right place for this. > >> + { >> + bail!("can't sync, source equals the target"); >> + } >> + >> let (email, notify) = crate::server::lookup_datastore_notify_settings(&sync_job.store); >> >> let upid_str = WorkerTask::spawn( >> @@ -122,13 +139,33 @@ pub fn do_sync_job( >> } >> task_log!( >> worker, >> - "sync datastore '{}' from '{}/{}'", >> + "sync datastore '{}' from '{}{}'", >> sync_job.store, >> - sync_job.remote, >> + sync_job >> + .remote >> + .as_deref() >> + .map_or(String::new(), |remote| format!("{remote}/")), > > nit: might be nicer to pull this out via a `let source_prefix = match ...` > statement up front. > >> sync_job.remote_store, >> ); >> >> - pull_store(&worker, &client, pull_params).await?; >> + if sync_job.remote.is_some() { >> + pull_store(&worker, &client, pull_params).await?; >> + } else { >> + if let (Some(target_ns), Some(source_ns)) = (sync_job.ns, sync_job.remote_ns) { >> + if target_ns.path().starts_with(source_ns.path()) >> + && sync_job.store == sync_job.remote_store >> + && sync_job.max_depth.map_or(true, |sync_depth| { >> + target_ns.depth() + sync_depth > MAX_NAMESPACE_DEPTH >> + }) { >> + task_log!( >> + worker, >> + "Can't sync namespace into one of its sub-namespaces, would exceed maximum namespace depth, skipping" >> + ); >> + } >> + } else { >> + pull_store(&worker, &client, pull_params).await?; >> + } >> + } >> >> task_log!(worker, "sync job '{}' end", &job_id); >> > >> @@ -256,7 +294,7 @@ async fn pull( >> let pull_params = PullParameters::new( >> &store, >> ns, >> - &remote, >> + remote.as_deref().unwrap_or("local"), > > same here w.r.t. intermediate bug (or at least hard to follow commit order) > >> &remote_store, >> remote_ns.unwrap_or_default(), >> auth_id.clone(), >