[pve-devel] [PATCH] fix #1569: add shared flag to disks

[Thomas Lamprecht]

On 2/15/18 1:25 PM, Chris Hofstaedtler | Deduktiva wrote:
> * Thomas Lamprecht <t.lamprecht at proxmox.com> [180215 13:22]:
>> On 2/15/18 12:03 PM, Chris Hofstaedtler wrote:
>>> With shared=1, (live) migration ignores the disk and assumes it is
>>> present on all target nodes. This works similar to shared=1 on LXC
>>> mountpoints.
>>
>> Why not just mark the storage shared through the storage config?
>> If the storage is in fact available on all nodes then that would
>> be the correct way, i.e., this setting is there to tell PVE that
>> a possible local-only (e.g., Directory storage) is in fact shared
>> already between nodes.
> 
> In this case this is a disk that is not managed by normal PVE
> storage.
> 
> Example:
> 
> scsi1: /dev/disk/by-id/dm-uuid-mpath-360002ac00000000006007a1800004b3c,cache=writeback,discard=on,shared=1,size=1T
> 
> I'd like to see "direct" multipath LUNs integrated into the storage
> abstraction, but for now ...
> 

OK, thanks for providing an use case!

A few comments to you patch:

> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 937a855..cf1edef 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -318,6 +318,7 @@ sub sync_disks {
>  	    my ($volid, $attr) = @_;
>  
>  	    if ($volid =~ m|^/|) {
> +		return if $attr->{shared};
>  		$local_volumes->{$volid}->{ref} = 'config';
>  		die "local file/device\n";
>  	    }
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 20d6682..7c68d17 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -819,6 +819,13 @@ my %drivedesc_base = (
>  	maxLength => 20*3, # *3 since it's %xx url enoded
>  	description => "The drive's reported serial number, url-encoded, up to 20 bytes long.",
>  	optional => 1,
> +    },
> +    shared => {
> +        type => 'boolean',
> +        description => 'Mark this locally-managed volume as available on all nodes',
> +        verbose_description => "Mark this locally-managed volume as available on all nodes.\n\nWARNING: This option does not share the mount point automatically, it assumes it is shared already!",

"mount point" is left over from copying this from pve-container,
please change this to volume or something else fitting.

> +        optional => 1,
> +        default => 0,
>      }
>  );
>  
> @@ -2837,7 +2844,7 @@ sub foreach_volid {
>      my $volhash = {};
>  
>      my $test_volid = sub {
> -	my ($volid, $is_cdrom, $replicate, $snapname) = @_;
> +	my ($volid, $is_cdrom, $replicate, $shared, $snapname) = @_;
>  
>  	return if !$volid;
>  
> @@ -2847,6 +2854,9 @@ sub foreach_volid {
>  	$volhash->{$volid}->{replicate} //= 0;
>  	$volhash->{$volid}->{replicate} = 1 if $replicate;
>  
> +	$volhash->{$volid}->{shared} //= 0;
> +	$volhash->{$volid}->{shared} = 1 if $shared;
> +

I'd rather see:
$volhash->{$volid}->{shared} = $shared // 0;
here, but as the context use this pattern too let it just be,
better we clean it up in on go later on...

>  	$volhash->{$volid}->{referenced_in_config} //= 0;
>  	$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
>  
> @@ -2856,7 +2866,7 @@ sub foreach_volid {
>  
>      foreach_drive($conf, sub {
>  	my ($ds, $drive) = @_;
> -	$test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, undef);
> +	$test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared} // 1, undef);

Why is the default 1 here (and below)?
In the description you say the default is '0', as does the code
one hunk above. But the other code in the context has also this
strange pattern, so I guess you just oriented yourself on it.

Please use the correct default here (i.e., omit the // 1), I'll
give the existing code a deeper look in the meanwhile...

Could you please address the mentioned notes and send a v2 with your
Signed-off-by tag?

thanks,
Thomas

>      });
>  
>      foreach my $snapname (keys %{$conf->{snapshots}}) {
> @@ -2864,7 +2874,7 @@ sub foreach_volid {
>  	$test_volid->($snap->{vmstate}, 0, 1, $snapname);
>  	foreach_drive($snap, sub {
>  	    my ($ds, $drive) = @_;
> -	    $test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $snapname);
> +	    $test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared} // 1, $snapname);
>          });
>      }
>  
>