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 ;))
>
> 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
