[pve-devel] applied: [PATCH container] apply_pending: call cleanup_pending between change/delete loops

[Thomas Lamprecht]

On 2/6/20 3:48 PM, Dominik Csapak wrote:
> lgtm, did not break anything obvious, and fixed my problem i reported yesterday[0]
> 
> Tested-By: Dominik Csapak <d.csapak at proxmox.com>
> 

Thanks, with your T-b applied. Oguz, the commit message lacked the total
"why it happens" and "why does it get solved" parts. Those are important.
Linking to outside info can be good but the core part of the explanation
should always be inline, links tend to go dead sometimes and it's easier
to read a few sentences inline than to click on one, or even multiple links,
to find out what's and why actually something is being done they way it is.

Further, while this resolves the issue of a broken config in general the
underlying "when are config property values equal" is not solved. I can
still trigger a fake pending change. For example, assume the following
config property present and applied already:

mp0: tom-nasi:110/vm-110-disk-0.raw,mp=/foo,backup=1,size=102M

Now a API or CLI client (in this case simulated through pct) sets it to the
same semantic value, but switched order of property strings:
# pct set 110 --mp0 mp=/foo,tom-nasi:110/vm-110-disk-0.raw,backup=1,size=102M

I get a pending change, but there'd be none. Same issue if I do not switch
order of properties in the string but one time a default_key is present
verbose "enabled=1", and one time in it's short form "1".

The correct solution would be parsing the properties and doing a deterministic
(deep) compare.
A heuristic could be ensuring that at least our webinterface and backend always
print property strings the same way (i.e., sorted) - that would be possible
cheaper but not solve that effect for all clients using the API.

> 0: https://pve.proxmox.com/pipermail/pve-devel/2020-February/041548.html
> 
> On 2/5/20 3:03 PM, Oguz Bektas wrote:
>> instead of calling it while iterating, inbetween the loops is a better
>> place in terms of similarity with qemu side (also this should fix the bug that
>> dominik found[0])
>>
>> [0]: https://pve.proxmox.com/pipermail/pve-devel/2020-February/041573.html
>>
>> Signed-off-by: Oguz Bektas <o.bektas at proxmox.com>
>> ---
>>   src/PVE/LXC/Config.pm | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
>> index 310aba6..e88ba0b 100644
>> --- a/src/PVE/LXC/Config.pm
>> +++ b/src/PVE/LXC/Config.pm
>> @@ -1268,7 +1268,6 @@ sub vmconfig_apply_pending {
>>       # FIXME: $force deletion is not implemented for CTs
>>       foreach my $opt (sort keys %$pending_delete_hash) {
>>       next if $selection && !$selection->{$opt};
>> -    $class->cleanup_pending($conf);
>>       eval {
>>           if ($opt =~ m/^mp(\d+)$/) {
>>           my $mp = $class->parse_ct_mountpoint($conf->{$opt});
>> @@ -1289,6 +1288,8 @@ sub vmconfig_apply_pending {
>>       }
>>       }
>>   +    $class->cleanup_pending($conf);
>> +
>>       foreach my $opt (sort keys %{$conf->{pending}}) { # add/change
>>       next if $opt eq 'delete'; # just to be sure
>>       next if $selection && !$selection->{$opt};
>> @@ -1304,7 +1305,6 @@ sub vmconfig_apply_pending {
>>       if (my $err = $@) {
>>           $add_apply_error->($opt, $err);
>>       } else {
>> -        $class->cleanup_pending($conf);
>>           $conf->{$opt} = delete $conf->{pending}->{$opt};
>>       }
>>       }
>>