Re: [pbs-devel] [PATCH V2 proxmox-backup] Fix 3335: Allow removing datastore contents on delete

[Dominik Csapak <d.csapak@proxmox.com>]

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)