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.csa...@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.bek...@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}; >> } >> } >> _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
