Re: [pbs-devel] [PATCH proxmox-backup 02/26] config: make RemovableDeviceConfig savable to config file

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 07/07/2022 10:35, Hannes Laimer wrote:
> Am 06.07.22 um 13:44 schrieb Thomas Lamprecht:
>> On 06/07/2022 13:33, Wolfgang Bumiller wrote:
>>> Do we need this to be a separate config file though? Can this not simply
>>> be part of the datastore directly? We already "link" them by having to
>>> define the datastore as `removable`, so can we not just put all the
>>> values in there?
>>
>> IIRC we talked about adding just a "backing-device", or the like (probably
>> something a bit more explicit w.r.t. to removable), property to existing
>> datastores, and then pretty much handle them like existing ones.
>>
>> That way we can reuse most of existing infrastructure and functionality,
>> what changes is a different (or no) error on sync, GC, etc. (or repeat skipped
>> jobs when plugged in) and the "auto-mount + activate in PBS" via udev helper.
> 
> Yes, we could put all the removable-device data directly into the
> datastore config. But I think this is a cleaner and more flexible
> approach, I guess you could argue similarly for why sync and prune jobs
> have their own configs.

Having less config files def. reduces complexity, that is for both, us
_and_ the user/admins to maintain.

Having two separate API and possible GUI endpoints makes whole PBS more
complex to manage, especially for new users (IME the perceived complexity
grows over-proportionally with the amount of UI/API knobs exposed), so
reducing that has more weight than the complexity of a potential
implementation, or better the change to one, as most of the time simple
UI/UX can mean simpler end result (even if the way to get there definitively
can be more complex).

> What do you mean with we can't reuse
> infrastructure with the own config approach? Sync/Prune/GC work like on
> a normal datastore, just with the possibility of failing with "no
> device present".

Yeah, exactly, a separate config or section type doesn't provides much value
there, at least fwict with having only a short look now and relying more on
my previous experience with that parts of the code base.

> 
> I think by having a removable-device as its own thing we actually end up
> reusing more of the already existing API/UI functionality, because
> having it directly in the datastore config would mean a lot of "manual"
> parsing of property strings and a "custom" update/add implementation.

hmm, not sure why that should be, albeit  wouldn't it be just a normal,
optional property? And updater support probably isn't required as we don't
want to expose altering this after creation, at least not initially.

W.r.t. "reuse more": is this meant as "we can then factor out some stuff and
use it twice with the special logic for each case upfront"? As while I'm not
having all that code base in my head atm., I'd think that adding some simple
special function call to the respective run job handlers (either in the task
if we want to still log a skipped run as task log, or upfront, in the "should
a job run now" logic, if we don't want a task log entry) would seem fine and
def. less code churn without duplicating config (or section types).