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

[Gabriel Goller]

On 15.07.2024 13:23, Fabian Grünbichler wrote:
>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 :)
>

Ooh, I think I got it...
So basically we could just call `ChunkStore::verify_chunkstore` in
`ChunkStore::open` and drop the `open` call above, right?

>>>
>>>> +    } 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(),
>>>> +        )?;
>>>> +    }
>
>
>_______________________________________________
>pbs-devel mailing list
>pbs-devel at lists.proxmox.com
>https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel