* [pve-devel] [PATCH qemu-server 1/3] migrate: improve early error messages @ 2021-03-19 13:49 Fabian Ebner 2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage Fabian Ebner 2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 3/3] code cleanup: migrate: avoid post-ifs Fabian Ebner 0 siblings, 2 replies; 7+ messages in thread From: Fabian Ebner @ 2021-03-19 13:49 UTC (permalink / raw) To: pve-devel; +Cc: Thomas Lamprecht by re-using the log_error-closure and not dying too early. In the case of unreferenced volumes, it was not clear why a certain storage would be required for migration. Now, the list of all volumes is always printed, and in the error case, all problems will be printed before we abort. Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> --- PVE/QemuMigrate.pm | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm index 5c019fc..3597cc9 100644 --- a/PVE/QemuMigrate.pm +++ b/PVE/QemuMigrate.pm @@ -400,12 +400,19 @@ sub sync_disks { my $targetsid = PVE::QemuServer::map_storage($self->{opts}->{storagemap}, $storeid); # check if storage is available on target node - PVE::Storage::storage_check_node($storecfg, $targetsid, $self->{node}); + my $target_scfg = PVE::Storage::storage_check_node( + $storecfg, + $targetsid, + $self->{node}, + 1, + ); + + $log_error->("storage '$targetsid' is not available on node '$self->{node}'") + if !$target_scfg; # grandfather in existing mismatches - if ($targetsid ne $storeid) { - my $target_scfg = PVE::Storage::storage_config($storecfg, $targetsid); - die "content type 'images' is not available on storage '$targetsid'\n" + if ($targetsid ne $storeid && $target_scfg) { + $log_error->("content type 'images' is not available on storage '$targetsid'") if !$target_scfg->{content}->{images}; } -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage 2021-03-19 13:49 [pve-devel] [PATCH qemu-server 1/3] migrate: improve early error messages Fabian Ebner @ 2021-03-19 13:49 ` Fabian Ebner 2021-03-19 14:16 ` Fabian Grünbichler 2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 3/3] code cleanup: migrate: avoid post-ifs Fabian Ebner 1 sibling, 1 reply; 7+ messages in thread From: Fabian Ebner @ 2021-03-19 13:49 UTC (permalink / raw) To: pve-devel it's cheap and saves code. Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> --- PVE/QemuMigrate.pm | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm index 3597cc9..44cecce 100644 --- a/PVE/QemuMigrate.pm +++ b/PVE/QemuMigrate.pm @@ -410,11 +410,8 @@ sub sync_disks { $log_error->("storage '$targetsid' is not available on node '$self->{node}'") if !$target_scfg; - # grandfather in existing mismatches - if ($targetsid ne $storeid && $target_scfg) { - $log_error->("content type 'images' is not available on storage '$targetsid'") - if !$target_scfg->{content}->{images}; - } + $log_error->("content type 'images' is not available on storage '$targetsid'") + if $target_scfg && !$target_scfg->{content}->{images}; PVE::Storage::foreach_volid($dl, sub { my ($volid, $sid, $volinfo) = @_; -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage 2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage Fabian Ebner @ 2021-03-19 14:16 ` Fabian Grünbichler 2021-03-22 8:56 ` Fabian Ebner 0 siblings, 1 reply; 7+ messages in thread From: Fabian Grünbichler @ 2021-03-19 14:16 UTC (permalink / raw) To: Proxmox VE development discussion On March 19, 2021 2:49 pm, Fabian Ebner wrote: > it's cheap and saves code. but also changes behaviour in a non-backwards-compatible fashion. previously, if a disk was already on a storage that does not have images configured, and the migration leaves it on that storage, this config mismatch was ignored (hence the "grandfather in existing mismatches" comment). note that users might be able to migrate, but not able to change storage.cfg to fix this "misconfiguration". > > Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> > --- > PVE/QemuMigrate.pm | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm > index 3597cc9..44cecce 100644 > --- a/PVE/QemuMigrate.pm > +++ b/PVE/QemuMigrate.pm > @@ -410,11 +410,8 @@ sub sync_disks { > $log_error->("storage '$targetsid' is not available on node '$self->{node}'") > if !$target_scfg; > > - # grandfather in existing mismatches > - if ($targetsid ne $storeid && $target_scfg) { > - $log_error->("content type 'images' is not available on storage '$targetsid'") > - if !$target_scfg->{content}->{images}; > - } > + $log_error->("content type 'images' is not available on storage '$targetsid'") > + if $target_scfg && !$target_scfg->{content}->{images}; > > PVE::Storage::foreach_volid($dl, sub { > my ($volid, $sid, $volinfo) = @_; > -- > 2.20.1 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage 2021-03-19 14:16 ` Fabian Grünbichler @ 2021-03-22 8:56 ` Fabian Ebner 2021-03-22 10:01 ` Fabian Grünbichler 0 siblings, 1 reply; 7+ messages in thread From: Fabian Ebner @ 2021-03-22 8:56 UTC (permalink / raw) To: pve-devel, Fabian Grünbichler Am 19.03.21 um 15:16 schrieb Fabian Grünbichler: > On March 19, 2021 2:49 pm, Fabian Ebner wrote: >> it's cheap and saves code. > > but also changes behaviour in a non-backwards-compatible fashion. > > previously, if a disk was already on a storage that does not have images > configured, and the migration leaves it on that storage, this config > mismatch was ignored (hence the "grandfather in existing mismatches" > comment). note that users might be able to migrate, but not able to > change storage.cfg to fix this "misconfiguration". > What about the recent change [0] in pve-storage then, i.e. not listing VM disks for storages without an appropriate content type? I'd argue that unreferenced images that lie on storages without an 'image' content type should not be picked up in the first place. If we don't agree on this, then [0] needs to be reverted... If we do agree on this, we should double down on [0] and actually be precise about the content type in vdisk_list by either: A) adding a content/guest type parameter to vdisk_list. B) adapting the call sites to filter storages. Afterwards, this patch could be applied with the appropriate dependency bump ;) Note that referenced unused images will still be picked up by the PVE::QemuServer::foreach_volid iteration, no matter what the content type of their storage is. [0]: https://git.proxmox.com/?p=pve-storage.git;a=commit;h=a44c18925d223a971296801a0985db34707ada4d >> >> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> >> --- >> PVE/QemuMigrate.pm | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm >> index 3597cc9..44cecce 100644 >> --- a/PVE/QemuMigrate.pm >> +++ b/PVE/QemuMigrate.pm >> @@ -410,11 +410,8 @@ sub sync_disks { >> $log_error->("storage '$targetsid' is not available on node '$self->{node}'") >> if !$target_scfg; >> >> - # grandfather in existing mismatches >> - if ($targetsid ne $storeid && $target_scfg) { >> - $log_error->("content type 'images' is not available on storage '$targetsid'") >> - if !$target_scfg->{content}->{images}; >> - } >> + $log_error->("content type 'images' is not available on storage '$targetsid'") >> + if $target_scfg && !$target_scfg->{content}->{images}; >> >> PVE::Storage::foreach_volid($dl, sub { >> my ($volid, $sid, $volinfo) = @_; >> -- >> 2.20.1 >> >> >> >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >> >> >> > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage 2021-03-22 8:56 ` Fabian Ebner @ 2021-03-22 10:01 ` Fabian Grünbichler 2021-03-22 11:08 ` Fabian Ebner 0 siblings, 1 reply; 7+ messages in thread From: Fabian Grünbichler @ 2021-03-22 10:01 UTC (permalink / raw) To: Fabian Ebner, pve-devel On March 22, 2021 9:56 am, Fabian Ebner wrote: > Am 19.03.21 um 15:16 schrieb Fabian Grünbichler: >> On March 19, 2021 2:49 pm, Fabian Ebner wrote: >>> it's cheap and saves code. >> >> but also changes behaviour in a non-backwards-compatible fashion. >> >> previously, if a disk was already on a storage that does not have images >> configured, and the migration leaves it on that storage, this config >> mismatch was ignored (hence the "grandfather in existing mismatches" >> comment). note that users might be able to migrate, but not able to >> change storage.cfg to fix this "misconfiguration". >> > > What about the recent change [0] in pve-storage then, i.e. not listing > VM disks for storages without an appropriate content type? I'd have to think more about that (I mean, it obviously makes sense, the question is whether it breaks something ;)) > > I'd argue that unreferenced images that lie on storages without an > 'image' content type should not be picked up in the first place. If we > don't agree on this, then [0] needs to be reverted... but this is not just about unreferenced disks? I mean, this code runs for ALL disks returned by pve-storage. if pve-storage now filters, then this does not apply for the '"old == new" storage which has wrong content type' scenario, since this part of the code thinks there are no volumes there, so what I originally thought is broken is not, > If we do agree on this, we should double down on [0] and actually be > precise about the content type in vdisk_list by either: > A) adding a content/guest type parameter to vdisk_list. > B) adapting the call sites to filter storages. > Afterwards, this patch could be applied with the appropriate dependency > bump ;) > > Note that referenced unused images will still be picked up by the > PVE::QemuServer::foreach_volid iteration, no matter what the content > type of their storage is. but this would then mean that for referenced images, '"old != new" storage where both old and new storage have wrong content type' now suddenly works where it previously didn't? not because of this patch, but because of vdisk_list not returning those, thus skipping the check that is changed in this patch? oh, and shared storages with wrong content type also just work, but since we don't really touch volumes there anyway with migration that's probably fine.. IFF we want to say "wrong content type" is wrong and should make the VM unmigratable, we should probably move the check into test_volid so that it catches ALL volumes always.. > > [0]: > https://git.proxmox.com/?p=pve-storage.git;a=commit;h=a44c18925d223a971296801a0985db34707ada4d > >>> >>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> >>> --- >>> PVE/QemuMigrate.pm | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm >>> index 3597cc9..44cecce 100644 >>> --- a/PVE/QemuMigrate.pm >>> +++ b/PVE/QemuMigrate.pm >>> @@ -410,11 +410,8 @@ sub sync_disks { >>> $log_error->("storage '$targetsid' is not available on node '$self->{node}'") >>> if !$target_scfg; >>> >>> - # grandfather in existing mismatches >>> - if ($targetsid ne $storeid && $target_scfg) { >>> - $log_error->("content type 'images' is not available on storage '$targetsid'") >>> - if !$target_scfg->{content}->{images}; >>> - } >>> + $log_error->("content type 'images' is not available on storage '$targetsid'") >>> + if $target_scfg && !$target_scfg->{content}->{images}; >>> >>> PVE::Storage::foreach_volid($dl, sub { >>> my ($volid, $sid, $volinfo) = @_; >>> -- >>> 2.20.1 >>> >>> >>> >>> _______________________________________________ >>> pve-devel mailing list >>> pve-devel@lists.proxmox.com >>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>> >>> >>> >> >> >> _______________________________________________ >> pve-devel mailing list >> pve-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >> >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage 2021-03-22 10:01 ` Fabian Grünbichler @ 2021-03-22 11:08 ` Fabian Ebner 0 siblings, 0 replies; 7+ messages in thread From: Fabian Ebner @ 2021-03-22 11:08 UTC (permalink / raw) To: Fabian Grünbichler, pve-devel Am 22.03.21 um 11:01 schrieb Fabian Grünbichler: > On March 22, 2021 9:56 am, Fabian Ebner wrote: >> Am 19.03.21 um 15:16 schrieb Fabian Grünbichler: >>> On March 19, 2021 2:49 pm, Fabian Ebner wrote: >>>> it's cheap and saves code. >>> >>> but also changes behaviour in a non-backwards-compatible fashion. >>> >>> previously, if a disk was already on a storage that does not have images >>> configured, and the migration leaves it on that storage, this config >>> mismatch was ignored (hence the "grandfather in existing mismatches" >>> comment). note that users might be able to migrate, but not able to >>> change storage.cfg to fix this "misconfiguration". >>> >> >> What about the recent change [0] in pve-storage then, i.e. not listing >> VM disks for storages without an appropriate content type? > > I'd have to think more about that (I mean, it obviously makes sense, the > question is whether it breaks something ;)) > For referenced volumes on storages without an 'image' or 'rootfs' content type, it does break updating the disk size before migration. >> >> I'd argue that unreferenced images that lie on storages without an >> 'image' content type should not be picked up in the first place. If we >> don't agree on this, then [0] needs to be reverted... > > but this is not just about unreferenced disks? I mean, this code runs > for ALL disks returned by pve-storage. if pve-storage now filters, then > this does not apply for the '"old == new" storage which has wrong content > type' scenario, since this part of the code thinks there are no volumes > there, so what I originally thought is broken is not, It does apply when the content type of the storage is only 'rootfs' for example (and we don't currently have a dependency bump on the new pve-storage behavior ;) > >> If we do agree on this, we should double down on [0] and actually be >> precise about the content type in vdisk_list by either: >> A) adding a content/guest type parameter to vdisk_list. >> B) adapting the call sites to filter storages. >> Afterwards, this patch could be applied with the appropriate dependency >> bump ;) >> >> Note that referenced unused images will still be picked up by the >> PVE::QemuServer::foreach_volid iteration, no matter what the content >> type of their storage is. > > but this would then mean that for referenced images, '"old != new" > storage where both old and new storage have wrong content type' now > suddenly works where it previously didn't? not because of this patch, > but because of vdisk_list not returning those, thus skipping the check > that is changed in this patch? oh, and shared storages with wrong > content type also just work, but since we don't really touch volumes > there anyway with migration that's probably fine.. > For shared ones, we skip the scan, and if there is a --targetstorge map, there already is a check as part of the API call itself. > IFF we want to say "wrong content type" is wrong and should make the VM > unmigratable, we should probably move the check into test_volid so that > it catches ALL volumes always.. > But there might be more subtle things like the disk size issue, so I'd be in favor of reverting [0] (for now) and instead adapting the image removal on VM destruction to not scan storages without 'images' content. >> >> [0]: >> https://git.proxmox.com/?p=pve-storage.git;a=commit;h=a44c18925d223a971296801a0985db34707ada4d >> >>>> >>>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> >>>> --- >>>> PVE/QemuMigrate.pm | 7 ++----- >>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm >>>> index 3597cc9..44cecce 100644 >>>> --- a/PVE/QemuMigrate.pm >>>> +++ b/PVE/QemuMigrate.pm >>>> @@ -410,11 +410,8 @@ sub sync_disks { >>>> $log_error->("storage '$targetsid' is not available on node '$self->{node}'") >>>> if !$target_scfg; >>>> >>>> - # grandfather in existing mismatches >>>> - if ($targetsid ne $storeid && $target_scfg) { >>>> - $log_error->("content type 'images' is not available on storage '$targetsid'") >>>> - if !$target_scfg->{content}->{images}; >>>> - } >>>> + $log_error->("content type 'images' is not available on storage '$targetsid'") >>>> + if $target_scfg && !$target_scfg->{content}->{images}; >>>> >>>> PVE::Storage::foreach_volid($dl, sub { >>>> my ($volid, $sid, $volinfo) = @_; >>>> -- >>>> 2.20.1 >>>> >>>> >>>> >>>> _______________________________________________ >>>> pve-devel mailing list >>>> pve-devel@lists.proxmox.com >>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>>> >>>> >>>> >>> >>> >>> _______________________________________________ >>> pve-devel mailing list >>> pve-devel@lists.proxmox.com >>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>> >>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH qemu-server 3/3] code cleanup: migrate: avoid post-ifs 2021-03-19 13:49 [pve-devel] [PATCH qemu-server 1/3] migrate: improve early error messages Fabian Ebner 2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage Fabian Ebner @ 2021-03-19 13:49 ` Fabian Ebner 1 sibling, 0 replies; 7+ messages in thread From: Fabian Ebner @ 2021-03-19 13:49 UTC (permalink / raw) To: pve-devel Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> --- PVE/QemuMigrate.pm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm index 44cecce..64794a7 100644 --- a/PVE/QemuMigrate.pm +++ b/PVE/QemuMigrate.pm @@ -407,11 +407,11 @@ sub sync_disks { 1, ); - $log_error->("storage '$targetsid' is not available on node '$self->{node}'") - if !$target_scfg; - - $log_error->("content type 'images' is not available on storage '$targetsid'") - if $target_scfg && !$target_scfg->{content}->{images}; + if (!$target_scfg) { + $log_error->("storage '$targetsid' is not available on node '$self->{node}'"); + } elsif (!$target_scfg->{content}->{images}) { + $log_error->("content type 'images' is not available on storage '$targetsid'"); + } PVE::Storage::foreach_volid($dl, sub { my ($volid, $sid, $volinfo) = @_; -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-22 11:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-19 13:49 [pve-devel] [PATCH qemu-server 1/3] migrate: improve early error messages Fabian Ebner 2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 2/3] migrate: always check if content type images is available for target storage Fabian Ebner 2021-03-19 14:16 ` Fabian Grünbichler 2021-03-22 8:56 ` Fabian Ebner 2021-03-22 10:01 ` Fabian Grünbichler 2021-03-22 11:08 ` Fabian Ebner 2021-03-19 13:49 ` [pve-devel] [PATCH qemu-server 3/3] code cleanup: migrate: avoid post-ifs Fabian Ebner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox