[pve-devel] [PATCH v3 manager 1/1] backup: move logic to include guests into method

[Fabian Grünbichler]

On March 18, 2020 11:57 am, Aaron Lauterer wrote:
> 
> 
> On 3/17/20 3:33 PM, Fabian Grünbichler wrote:
>> On March 16, 2020 4:44 pm, Aaron Lauterer wrote:
>>> This extracts the logic which guests are to be included in a backup job
>>> into its own method 'get_included_guests'. This makes it possible to
>>> develop other features around backup jobs.
>>>
>>> Logic which was spread out accross the API2/VZDump.pm file and the
>>> VZDump.pm file is refactored into the new method, most notably the
>>> logic that dealt with excluded guests.
>>>
>>> The new method will return the VMIDs for the whole cluster. If a VMID is
>>> present on the local node and thus part of the local backup job is still
>>> determined in the VZDump::exec_backup method.
>> 
>> this is not true. previously, that was handled partly by the API
>> already, so behaviour changed in some cases.
>> 
>>>
>>> Permission checks are kept where they are to be able to handle missing
>>> permissions according to the current context. The old behavior to die
>>> on a backup job when the user is missing the permission to a guest and
>>> the job is not an 'all' or 'exclude' job is kept.
>>>
>>> The creation of the skiplist is moved to the VZDump::exec_backup method,
>>> close to the place where it is used.
>> 
>> but it was used already in the API, so it made sense to have it there.
>> see comments in-line. I am not saying we can't change this behaviour,
>> but from this description it does not sound like it was (fully)
>> intentional.
> 
> Thanks for the feedback!
> 
> It seems like the early exit right after the skiplist creation got lost 
> while I was working on the whole thing :/
> Also thanks to point out the taskid thing for a single guest backup 
> further down.
> 
> The idea driving this patch is to have one place to decide which guests 
> will be included in a backup job so it can be used in more places than 
> just the actual backups. Right now that logic is very spread out and 
> sometimes even a bit redundant AFAICT.
> 
> For example in the PVE/API2/VZDump.pm where @vmids will contain only 
> local vmids if the `all` parameter is not set and then in 
> PVE/VZDump->exec_backup the vmids are checked again against 
> $plugin->vmlist().
> 
> After going through all the changes and existing code once more I am not 
> sure if we should even try to keep the exact same behavior in place.
> 
> What if the new PVE/VZDump->get_included_guests sub will return
> * all included VMIDs
> * local VMIDs
> * local skiplist
> for the current node?

yes, or just included/local and skipped/external. a caller interested in 
all of them can just merge those two mutually exclusive lists, a caller 
interested only in the former (e.g., vzdump with --all) can just ignore 
the latter, the regular vzdump call can just pass them both along so 
that exec_backup can print a skiplist and act on the filtered one.

> This way the logic could be nicely contained in one sub instead of being 
> spread out over multiple modules and subs. We can still exit quietly if 
> there are no local VMIDs and will do so even in other situations like 
> when `all` is set but there are either no guests on the local node or 
> they are all in the `exclude` list.
> 
> On the other hand there will be a bit higher computational load to 
> generate these lists all at once and not spread out with early exits 
> like now.

I don't see how the load would be much higher? we are talking about 
calls that either end up doing a backup, or end up parsing the configs 
of all the returned VMIDs. in both cases, iterating once over the 
(cached) vmlist provided by pmxcfs to find out which guest is owned by 
which node is very low-effort. this is not in any performance-critical 
hot-path ;)

> I hope I did not miss anything.