[pbs-devel] [PATCH proxmox-backup] fix #5439: disallow creation of datastore in root

[Gabriel Goller]

On 16.05.2024 13:42, Fabian Grünbichler wrote:
>On May 16, 2024 12:58 pm, Gabriel Goller wrote:
>> On 16.05.2024 12:15, Fabian Grünbichler wrote:
>>>On May 10, 2024 11:58 am, Gabriel Goller wrote:
>>>> Creating a datastore in root ('/') works, but afterwards gc fails (can't
>>>> traverse all directories). It might be sensible to restrict this and
>>>> disallow creation of datastores in the root directory.
>>>
>>>if we do this, we should also forbid it on the frontend side ;)
>>
>> Yes, we can do that as well.
>> This will probably be difficult, because AFAIK extjs doesn't support
>> anything like this out of the box. But creating a custom input field +
>> listeners to change the style should work.
>
>just add a validator that forbids '/', or am I missing something?
>
>>>I wonder whether we shouldn't handle this in a more generic fashion
>>>though:
>>>- disallow path being non-empty (ignoring .zfs ?) -> `/` is not allowed
>>>  by default
>>>- unless a flag is set -> in case we forget to handle something, we need
>>>  an escape hatch
>>>- if the flag is set, check whether .chunks already exists, and if it
>>>  does, do not recreate the chunk store
>>>
>>>that way, we could also solve the "re-add datastore after re-install"
>>>issue users are frequently facing..
>>
>> Wait, what is the issue here?
>
>the issue is that right now, if you lose your datastore.cfg (entry) for
>whatever reason (the most common would be - move the actual datastore
>from one PBS to another, or re-install, or test disaster recovery), you
>cannot recreate it via our API/CLI. to re-add an already existing
>(on-disk) datastore you have to manually edit datastore.cfg, since
>attempting to create a "new" one using the already existing path fails
>since it can't create the chunk store (it's already there after all).
>
>this happens more often than you'd think, and even if just for
>stream-lining disaster recovery it would be a nice addition. of course,
>the question is whether to focus on that (and just add a flag that
>signifies "expect/check that the datastore is already initialized"), or
>whether to have a generic "ignore that path is not empty" flag that also
>covers this (that would also cover more niche use cases, but most of
>those would go against our recommendations anyway, since they would
>entail sharing the datastore path with other usage).

ooof, I thought we did that already (the ignore .chunks if it already
exists).

What if instead of using a flag we do this automatically? So if one
creates a datastore it automatically checks if the .chunks dir exists,
and creates or doesn't create a datastore.