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

[Fabian Grünbichler]

On May 16, 2024 5:35 pm, Gabriel Goller wrote:
> 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.

I guess doing it always is also possible, provided
- we at least check that all the prefix dirs are there and have the
  right owner (else the chunk store is broken)
- we print a line that we found and re-used a pre-existing chunk store