[pve-devel] [PATCH qemu 2/3] add sub for local disks check

[Thomas Lamprecht]

some context would be great here, i.e., we have already foreach_volid
etc so _why_ this new method?

I prefer adding new methods with there initial use, to see it in context,
that may be personal but I honestly do not see a real benefit of splitting
such things up, they're not revertable on each one for example.. 
If you prefer it that way, fine, but else I'd just squasg 2/3 and 3/3.


Could it make sense to pull out the assembly of $local_volumes in
PVE::QemuMigrate::sync_disks in it's own function and re-use it here?

On 5/22/19 3:25 PM, Tim Marx wrote:
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>  PVE/API2/Qemu.pm | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index a771a1a..fa4ff63 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -3141,6 +3141,51 @@ __PACKAGE__->register_method({
>  	return PVE::QemuConfig->lock_config($vmid, $updatefn);
>      }});
>  
> +my $check_vm_disks_local = sub {
> +    my ($storecfg, $vmconf, $vmid) = @_;
> +
> +    my $local_disks = {};
> +
> +    my @sids = PVE::Storage::storage_ids($storecfg);
> +
> +    # check each storage for disks, even if they aren't referenced in VM config
> +    foreach my $storeid (@sids) {
> +	my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	next if $scfg->{shared};
> +	next if !PVE::Storage::storage_check_enabled($storecfg, $storeid, undef, 1);
> +
> +	# get list from PVE::Storage (for unused volumes)
> +	my $dl = PVE::Storage::vdisk_list($storecfg, $storeid, $vmid);

pretty expensive, isn't it? Most people do not have unreferenced disks if
they do not do strange stuff, so maybe do this only conditional for a full
check, or just jeep this heuristic and let it work for the common use, if
one has unreferenced disks the "higher" price of getting an error in the
task is a good hint that something is off...

> +	next if @{$dl->{$storeid}} == 0;
> +
> +	PVE::Storage::foreach_volid($dl, sub {
> +	    my ($volid, $sid, $volname) = @_;
> +	    $local_disks->{$volid} = $volname;
> +	});
> +    }
> +
> +    # add some more information to the disks e.g. cdrom
> +    PVE::QemuServer::foreach_volid($vmconf, sub {
> +	my ($volid, $attr) = @_;
> +
> +	my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
> +	if ($storeid) {
> +	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	    return if $scfg->{shared};
> +	}
> +	# The shared attr here is just a special case where the vdisk
> +	# is marked as shared manually
> +	return if $attr->{shared};
> +	return if $attr->{cdrom} and $volid eq "none";
> +
> +	if (exists $local_disks->{$volid}) {
> +	    @{$local_disks->{$volid}}{keys %$attr} = values %$attr
> +	} else {
> +	    $local_disks->{$volid} = $attr;
> +	    # ensure volid is present in case it's needed
> +	    $local_disks->{$volid}->{volid} = $volid;
> +	}
> +    });

The following hunk misses here and landed in the next patch, 3/3 ;)

+
+    return $local_disks;
+};
+

>  __PACKAGE__->register_method({
>      name => 'migrate_vm',
>      path => '{vmid}/migrate',
>