[pve-devel] [RFC storage 6/16] pvesm import: allow specifying storage+vmid instead of full volumeid
Fabian Ebner
f.ebner at proxmox.com
Thu Feb 6 09:59:22 CET 2020
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.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 ;))
>
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 at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>
> _______________________________________________
> 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