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