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

[Fabian Grünbichler]

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).

>>obviously, even with that we can explicitly always forbid '/' (before or
>>after implementing such a mechanism), since that one is always wrong.
> 
> TBH, I don't think there is much to gain from adding all of this.
> This is still very much a niche issue IMO, as most user have a separate
> disk for datastores, which makes a lot of these issues uncommon.

see above ;)