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

[Tim Marx]

> Thomas Lamprecht <t.lamprecht at proxmox.com> hat am 27. Mai 2019 um 14:16 geschrieben:
> 
> 
> 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.
> 

Yeah sure, just squash it.
> 
> 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?
> 

Could be handy, but my initial idea was to refactor the whole sync_disks function, because of various repetitive checks.
To ensure a stable transition, I decided not to do this in one step.

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

I agree, but I think it would increase the overall user experience.

In general I would like to make a clearer distinction between cli and web ui. The current approach is solid on the command line, where someone is used to read the log after executing a command, but it is completely unnatural for the web gui IMHO.
If we can detect problems in advance, we should do so and inform the user. Starting a task from the GUI where it is already clear that there is a problem isn't something I consider good ux. This of course is not only valid for this special case but in general.

> > +	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',
> >