On 2/5/20 11:50 AM, Fabian Grünbichler wrote:
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.eb...@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 ;))


I'll do this for v2. The problem of incompatible volume names when migrating between storages with 'path' and without 'path' remains, but (as already discussed off-list) introducing a print_volname as an inverse to parse_volname should solve that.


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@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to