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

[Oguz Bektas]

On Thu, Feb 06, 2020 at 04:53:04PM +0100, Thomas Lamprecht wrote:
> 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.
ah, i thought the link was enough. i'll write more detailed next time.
thanks for adding the details in the commit message.
> 
> 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".
indeed. but i think this isn't that tragic since it doesn't break any
functionality (i think?).

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

but still if you want i can take a look at implementing that soon.

> > 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};
> >>       }
> >>       }
> >>
> 
> 
>