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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 18, 2024 11:27 am, Gabriel Goller wrote:
> 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@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?

no, what I meant is:

in open, split out the checks there (that base is absolute, and that the
chunk dir is accessible) into a new helper fn. then call that new helper
fn in open (so that the behaviour of open stays the same).

then, also call this helper in your patch here - it doesn't really give
much extra benefit atm, but it makes it less easy to miss that this
should be done in both places when adding new checks.

I don't think we want to do the full verification on each open, but when
creating a datastore config entry with an existing datastore, we want to
do both the extensive checks and those we'd do when opening it.

of course another option would be to have a single helper, and a
parameter that controls whether the extensive checks should be done or
not. even harder to miss that there are two related sets of checks then
;)

>>>>
>>>>> +    } 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@lists.proxmox.com
>>https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel