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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id E9D9E622BA for ; Mon, 21 Feb 2022 14:49:53 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D8FEB19829 for ; Mon, 21 Feb 2022 14:49:23 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 2916019820 for ; Mon, 21 Feb 2022 14:49:23 +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 E658C462B8; Mon, 21 Feb 2022 14:49:22 +0100 (CET) Message-ID: <61a0b12a-c793-3435-4617-725e498468e3@proxmox.com> Date: Mon, 21 Feb 2022 14:49:20 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:98.0) Gecko/20100101 Thunderbird/98.0 Content-Language: en-US To: Thomas Lamprecht , Proxmox Backup Server development discussion , Dylan Whyte References: <20220126153836.2053113-1-d.whyte@proxmox.com> <4dec537e-4526-0db7-bb06-bf25a3c04ddb@proxmox.com> <05266c31-3897-aed9-abe8-69ec07e84e44@proxmox.com> From: Dominik Csapak In-Reply-To: <05266c31-3897-aed9-abe8-69ec07e84e44@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.156 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) 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 - Subject: Re: [pbs-devel] [PATCH V2 proxmox-backup] Fix 3335: Allow removing datastore contents on delete 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: Mon, 21 Feb 2022 13:49:54 -0000 On 2/21/22 14:39, Thomas Lamprecht wrote: > On 21.02.22 12:10, Dominik Csapak wrote: >> while i know that we don't recommend having other data in the datastore, >> imho it's not wise to remove *everything* in the datastore path >> >> AFAICT we can know what we touch (ct/host/vm/.chunk dirs, .gc-status, .lock) >> so we could only remove that? >> > > meh, I'm torn a bit; While PBS owns the datastore directory and this > is really fine to do IMO, it'd be relatively simple to avoid doing, as > like you say, there are few and always known top-level directories we need > to iterate over; if empty I'd also delete the top-level directory itself > though. i just saw too many posts where users nested datastores or other things to think that this will not bite anybody.... yes, removing the empty top-level would be good > >> also we should probably remove the datastore from the config before >> removing files from it? otherwise i can start deleting >> and another user could start his backup which will >> interfere with each other and leave the datastore in an >> undefined state? > > it should be put in maintenance state prior to starting off deletion, > that handles this already nicely and can set a readable info message; once > it's finally applied that is. > >> >> what we at least should have here is some mechanism that nobody >> can backup/restore/etc. from a datastore while it's deleted > > see above yes that'll work, but then we should probably go forward with the maintenance mode first and apply this after (with the appropriate maintenance mode check) > >>> + let info = &api2::config::datastore::API_METHOD_DELETE_DATASTORE; >>> + let result = match info.handler { >>> + ApiHandler::Async(handler) => (handler)(param, info, rpcenv).await?, >>> + _ => unreachable!(), >>> + }; >>> + >>> + crate::wait_for_local_worker(result.as_str().unwrap()).await?; >>> + Ok(Value::Null) >>> + >> >> i know the other functions in this file do it exactly like this, >> but i'd probably prefer to have a http client here to >> do the deletion via the api? (like we do in most other cases) > > hmm, while I agree in general for most commands I'm not so sure about this > specific one; this is something a admin may need to do when no space is left > anymore, having less parts involved could make it more robust in some cases. mhmm... yes in that case, doing it here makes it more robust. in that case i'd add a comment though *why* we do it here this way (and we should convert the other calls)