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

Fabian Ebner f.ebner at proxmox.com
Wed Jan 29 14:30:04 CET 2020


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.

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





More information about the pve-devel mailing list