[pve-devel] [RFC storage 6/16] pvesm import: allow specifying storage+vmid instead of full volumeid

Fabian Grünbichler f.gruenbichler at proxmox.com
Wed Feb 5 11:50:12 CET 2020


On January 29, 2020 2:30 pm, Fabian Ebner wrote:
> Extends the API so that 'volume' can also only be a storage identifier. In
> that case the VMID needs to be specified as well. In 'import_volume' a new
> name for the allocation is determined. This is useful for migration where
> the storage on the target is a different type, since the volume ID might
> look differently. E.g. 'mydir:102/vm-102-disk-0.raw' on a 'dir'
> storage would be 'mylvm:vm-102-disk-0' on an 'lvm' storage.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
> 
> An alternative approach would be to translate the volids as mentioned
> in the cover letter.

as just discussed off-list, I'd rather add a new optional parameter 
"allow-rename" instead of $vmid. volume could stay the same then, and if 
allow-rename is true and a volume of the requested name already exists,  
volume_import finds a new name using the VMID contained in $volume.

this would also allow us to opt-into the new behaviour more explicitly, 
and not-yet-updated third-party plugins should remain compatible (of 
course only with regards to the old behaviour ;))

> 
> It's not a pretty interface, but to not break backwards compatibility
> the 'volume' parameter needs to be non-optional (or is there a way to get
> around this?) and so it is a hybrid of either full ID or only storage ID.
> 
>  PVE/CLI/pvesm.pm             | 30 +++++++++++++++++++++++----
>  PVE/Storage.pm               |  6 ++----
>  PVE/Storage/LVMPlugin.pm     | 28 ++++++++++++++++----------
>  PVE/Storage/Plugin.pm        | 39 ++++++++++++++++++++++--------------
>  PVE/Storage/ZFSPoolPlugin.pm | 32 ++++++++++++++++++++---------
>  5 files changed, 91 insertions(+), 44 deletions(-)
> 
> diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
> index 74294b4..d91bc31 100755
> --- a/PVE/CLI/pvesm.pm
> +++ b/PVE/CLI/pvesm.pm
> @@ -261,7 +261,9 @@ __PACKAGE__->register_method ({
>  	additionalProperties => 0,
>  	properties => {
>  	    volume => {
> -		description => "Volume identifier",
> +		description => "Either a complete volume identifier of the form " .
> +		  "'storage:volume_name' or just the name of a storage. In the " .
> +		  "latter case the parameter vmid is required.",
>  		type => 'string',
>  		completion => \&PVE::Storage::complete_volume,
>  	    },
> @@ -297,6 +299,11 @@ __PACKAGE__->register_method ({
>  		maxLength => 80,
>  		optional => 1,
>  	    },
> +	    vmid => get_standard_option('pve-vmid', {
> +		description => "Only needed when no volume name is specified.",
> +		optional => 1,
> +		completion => \&PVE::Cluster::complete_vmid,
> +	    }),
>  	},
>      },
>      returns => { type => 'string' },
> @@ -350,10 +357,25 @@ __PACKAGE__->register_method ({
>  	}
>  
>  	my $cfg = PVE::Storage::config();
> -	my $volume = $param->{volume};
> +	my $storeid_or_volid = $param->{volume};
> +	my $vmid = $param->{vmid};
> +
> +	my ($storeid, $volname) = PVE::Storage::parse_volume_id($storeid_or_volid, 1);
> +	if (!defined($volname) || !defined($storeid)) { # assume it's only a storeid
> +	    $storeid = PVE::JSONSchema::parse_storage_id($storeid_or_volid);
> +	    $volname = undef;
> +	    die "vmid needs to be specified if there is no volume name\n"
> +		if !defined($vmid);
> +	}
> +
> +	if (defined($vmid) && defined($volname)) {
> +	    warn "ignoring parameter vmid='$vmid' since a volume name was specified\n";
> +	    $vmid = undef;
> +	}
> +
>  	my $delete = $param->{'delete-snapshot'};
> -	my $volid = PVE::Storage::volume_import($cfg, $infh, $volume, $param->{format},
> -	    $param->{base}, $param->{'with-snapshots'});
> +	my $volid = PVE::Storage::volume_import($cfg, $infh, $storeid, $volname,
> +	    $param->{format}, $param->{base}, $param->{'with-snapshots'}, $vmid);
>  	PVE::Storage::volume_snapshot_delete($cfg, $volid, $delete)
>  	    if defined($delete);
>  	return $volid;
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 57d723d..7d18b11 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1420,14 +1420,12 @@ sub volume_export {
>  }
>  
>  sub volume_import {
> -    my ($cfg, $fh, $volid, $format, $base_snapshot, $with_snapshots) = @_;
> +    my ($cfg, $fh, $storeid, $volname, $format, $base_snapshot, $with_snapshots, $vmid) = @_;
>  
> -    my ($storeid, $volname) = parse_volume_id($volid, 1);
> -    die "cannot import into volume '$volid'\n" if !$storeid;
>      my $scfg = storage_config($cfg, $storeid);
>      my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>      return $plugin->volume_import($scfg, $storeid, $fh, $volname, $format,
> -                                  $base_snapshot, $with_snapshots);
> +                                  $base_snapshot, $with_snapshots, $vmid);
>  }
>  
>  sub volume_export_formats {
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index 6c575f2..7250876 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -624,26 +624,32 @@ sub volume_import_formats {
>  }
>  
>  sub volume_import {
> -    my ($class, $scfg, $storeid, $fh, $volname, $format, $base_snapshot, $with_snapshots) = @_;
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $base_snapshot, $with_snapshots, $vmid) = @_;
>      die "volume import format $format not available for $class\n"
>  	if $format ne 'raw+size';
>      die "cannot import volumes together with their snapshots in $class\n"
>  	if $with_snapshots;
>      die "cannot import an incremental stream in $class\n" if defined($base_snapshot);
>  
> -    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> -	$class->parse_volname($volname);
> -    die "cannot import format $format into a file of format $file_format\n"
> -	if $file_format ne 'raw';
> +    my $name;
>  
> -    my $vg = $scfg->{vgname};
> -    my $lvs = lvm_list_volumes($vg);
> +    if (defined($volname)) {
> +	(undef, $name, $vmid, undef, undef, undef, my $file_format) =
> +	    $class->parse_volname($volname);
> +	die "cannot import format $format into a file of format $file_format\n"
> +	    if $file_format ne 'raw';
>  
> -    if ($lvs->{$vg}->{$volname}) {
> -	warn "volume $vg/$volname already exists - importing with a different name\n";
> -	$name = undef;
> +	my $vg = $scfg->{vgname};
> +	my $lvs = lvm_list_volumes($vg);
> +
> +	if ($lvs->{$vg}->{$volname}) {
> +	    warn "volume $vg/$volname already exists - importing with a different name\n";
> +	    $name = undef;
> +	}
>      }
>  
> +    die "no vmid specified\n" if !defined($vmid);
> +
>      my ($size) = PVE::Storage::Plugin::read_common_header($fh);
>      $size = int($size/1024);
>  
> @@ -651,7 +657,7 @@ sub volume_import {
>  	my $allocname = $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size);
>  	my $oldname = $volname;
>  	$volname = $allocname;
> -	if (defined($name) && $allocname ne $oldname) {
> +	if (defined($name) && defined($oldname) && $allocname ne $oldname) {
>  	    die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
>  	}
>  	my $file = $class->path($scfg, $volname, $storeid)
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 358a831..4e0ae8f 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -1176,7 +1176,7 @@ sub volume_export_formats {
>  
>  # Import data from a stream, creating a new or replacing or adding to an existing volume.
>  sub volume_import {
> -    my ($class, $scfg, $storeid, $fh, $volname, $format, $base_snapshot, $with_snapshots) = @_;
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $base_snapshot, $with_snapshots, $vmid) = @_;
>  
>      die "volume import format '$format' not available for $class\n"
>  	if $format !~ /^(raw|tar|qcow2|vmdk)\+size$/;
> @@ -1187,22 +1187,31 @@ sub volume_import {
>      die "format $format cannot be imported with snapshots\n"
>  	if $with_snapshots && ($data_format eq 'raw' || $data_format eq 'tar');
>  
> -    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $file_format) =
> -	$class->parse_volname($volname);
> +    my $name;
> +    my $file_format;
> +
> +    if (defined($volname)) {
> +	(undef, $name, $vmid, undef, undef, undef, $file_format) =
> +	    $class->parse_volname($volname);
> +
> +	# XXX: Should we bother with conversion routines at this level? This won't
> +	# happen without manual CLI usage, so for now we just error out...
> +	die "cannot import format $format into a file of format $file_format\n"
> +	    if $data_format ne $file_format && !($data_format eq 'tar' && $file_format eq 'subvol');
>  
> -    # XXX: Should we bother with conversion routines at this level? This won't
> -    # happen without manual CLI usage, so for now we just error out...
> -    die "cannot import format $format into a file of format $file_format\n"
> -	if $data_format ne $file_format && !($data_format eq 'tar' && $file_format eq 'subvol');
> -
> -    # Check for an existing file first since interrupting alloc_image doesn't
> -    # free it.
> -    my $file = $class->path($scfg, $volname, $storeid);
> -    if (-e $file) {
> -	warn "file '$file' already exists - importing with a different name\n";
> -	$name = undef;
> +	# Check for an existing file first since interrupting alloc_image doesn't
> +	# free it.
> +	my $file = $class->path($scfg, $volname, $storeid);
> +	if (-e $file) {
> +	    warn "file '$file' already exists - importing with a different name\n";
> +	    $name = undef;
> +	}
> +    } else {
> +	$file_format = ($data_format eq 'tar') ? 'subvol' : $data_format;
>      }
>  
> +    die "no vmid specified\n" if !defined($vmid);
> +
>      my ($size) = read_common_header($fh);
>      $size = int($size/1024);
>  
> @@ -1210,7 +1219,7 @@ sub volume_import {
>  	my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size);
>  	my $oldname = $volname;
>  	$volname = $allocname;
> -	if (defined($name) && $allocname ne $oldname) {
> +	if (defined($name) && defined($oldname) && $allocname ne $oldname) {
>  	    die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
>  	}
>  	my $file = $class->path($scfg, $volname, $storeid)
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 6394692..0ef6768 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -741,7 +741,7 @@ sub volume_export_formats {
>  }
>  
>  sub volume_import {
> -    my ($class, $scfg, $storeid, $fh, $volname, $format, $base_snapshot, $with_snapshots) = @_;
> +    my ($class, $scfg, $storeid, $fh, $volname, $format, $base_snapshot, $with_snapshots, $vmid) = @_;
>  
>      die "unsupported import stream format for $class: $format\n"
>  	if $format ne 'zfs';
> @@ -750,15 +750,27 @@ sub volume_import {
>      die "internal error: invalid file handle for volume_import\n"
>  	if !defined($fd);
>  
> -    my (undef, $dataset, $vmid) = $class->parse_volname($volname);
> -    my $zfspath = "$scfg->{pool}/$dataset";
> -    my $suffix = defined($base_snapshot) ? "\@$base_snapshot" : '';
> -    my $exists = 0 == run_command(['zfs', 'get', '-H', 'name', $zfspath.$suffix],
> -			     noerr => 1, errfunc => sub {});
> -    if (defined($base_snapshot)) {
> -	die "base snapshot '$zfspath\@$base_snapshot' doesn't exist\n" if !$exists;
> -    } elsif ($exists) {
> -	warn "volume '$zfspath' already exists - importing with a different name\n";
> +    die "base snapshot was specified, but no volume name\n"
> +	if defined($base_snapshot) && !defined($volname);
> +
> +    my $zfspath;
> +    my $dataset;
> +
> +    if (defined($volname)) {
> +	(undef, $dataset, $vmid) = $class->parse_volname($volname);
> +	$zfspath = "$scfg->{pool}/$dataset";
> +	my $suffix = defined($base_snapshot) ? "\@$base_snapshot" : '';
> +	my $exists = 0 == run_command(['zfs', 'get', '-H', 'name', $zfspath.$suffix],
> +				 noerr => 1, errfunc => sub {});
> +	if (defined($base_snapshot)) {
> +	    die "base snapshot '$zfspath\@$base_snapshot' doesn't exist\n" if !$exists;
> +	} elsif ($exists) {
> +	    warn "volume '$zfspath' already exists - importing with a different name\n";
> +	    $dataset = $class->find_free_diskname($storeid, $scfg, $vmid, $format);
> +	    $zfspath = "$scfg->{pool}/$dataset";
> +	}
> +    } else {
> +	die "no vmid specified\n" if !defined($vmid);
>  	$dataset = $class->find_free_diskname($storeid, $scfg, $vmid, $format);
>  	$zfspath = "$scfg->{pool}/$dataset";
>      }
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 




More information about the pve-devel mailing list