[pbs-devel] [PATCH proxmox-backup v2 1/2] fix #5439: allow to reuse existing datastore

[Fabian Grünbichler]

On July 12, 2024 1:57 pm, Gabriel Goller wrote:
> On 10.07.2024 15:28, Fabian Grünbichler wrote:
>>On June 12, 2024 3:22 pm, Gabriel Goller wrote:
>>> Disallow creating datastores in non-empty directories. Allow adding
>>> existing datastores via a 'reuse-datastore' checkmark. This only checks
>>> if all the necessary directories (.chunks + subdirectories and .lock)
>>> are existing and have the correct permissions, then opens the
>>> datastore.
>>>
>>> Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
>>> ---
>>>
>>> v2, thanks @Fabian:
>>>  - also check on frontend for root
>>>  - forbid datastore creation if dir not empty
>>>  - add reuse-datastore option
>>>  - verify chunkstore directories permissions and owners
>>>
>>>  pbs-datastore/src/chunk_store.rs | 49 ++++++++++++++++++++++++++++-
>>>  src/api2/config/datastore.rs     | 54 +++++++++++++++++++++++++-------
>>>  src/api2/node/disks/directory.rs |  1 +
>>>  src/api2/node/disks/zfs.rs       |  1 +
>>>  4 files changed, 93 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
>>> index 9f6289c9..077bc316 100644
>>> --- a/pbs-datastore/src/chunk_store.rs
>>> +++ b/pbs-datastore/src/chunk_store.rs
>>> @@ -164,7 +164,7 @@ impl ChunkStore {
>>>      /// Note that this must be used with care, as it's dangerous to create two instances on the
>>>      /// same base path, as closing the underlying ProcessLocker drops all locks from this process
>>>      /// on the lockfile (even if separate FDs)
>>
>>given this ^

this part here was important ;)

>>
>>> -    pub(crate) fn open<P: Into<PathBuf>>(
>>> +    pub fn open<P: Into<PathBuf>>(
>>
>>wouldn't it make more sense to split out the path checks and call them
>>both in open and in verify_chunkstore below
> 
> Why call them both in open and verify_chunkstore? create_datastore
> always calls the verify method before open, so we could move this to
> the verify function? I think I don't understand this correctly... 

see below..

>>>          name: &str,
>>>          base: P,
>>>          sync_level: DatastoreFSyncLevel,
>>> @@ -563,6 +563,53 @@ impl ChunkStore {
>>>          // unwrap: only `None` in unit tests
>>>          ProcessLocker::try_exclusive_lock(self.locker.clone().unwrap())
>>>      }
> 
>>> +
>>>      let tuning: DatastoreTuning = serde_json::from_value(
>>>          DatastoreTuning::API_SCHEMA
>>>              .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
>>>      )?;
>>> -    let backup_user = pbs_config::backup_user()?;
>>> -    let _store = ChunkStore::create(
>>> -        &datastore.name,
>>> -        path,
>>> -        backup_user.uid,
>>> -        backup_user.gid,
>>> -        worker,
>>> -        tuning.sync_level.unwrap_or_default(),
>>> -    )?;
>>> +
>>> +    if !datastore_empty && !reuse_datastore {
>>> +        bail!("Directory is not empty!");
>>> +    } else if !datastore_empty && reuse_datastore {
>>> +        ChunkStore::verify_chunkstore(&path)?;
>>> +
>>> +        let _store =
>>> +            ChunkStore::open(&datastore.name, path, tuning.sync_level.unwrap_or_default())?;
>>
>>(continued from first comment above) and leave out this call here?

this was the continuation ;) if we drop the open call here (to avoid
side-effects via the process locker), then we need the path checks in
verify *and* open. hope that makes sense now :)

>>
>>> +    } else {
>>> +        let backup_user = pbs_config::backup_user()?;
>>> +        let _store = ChunkStore::create(
>>> +            &datastore.name,
>>> +            path,
>>> +            backup_user.uid,
>>> +            backup_user.gid,
>>> +            worker,
>>> +            tuning.sync_level.unwrap_or_default(),
>>> +        )?;
>>> +    }