[pve-devel] [PATCH v3 storage 13/22] Introduce allow_rename parameter for pvesm import and storage_migrate
Fabian Grünbichler
f.gruenbichler at proxmox.com
Mon Mar 16 12:06:36 CET 2020
two nits in-line
On March 12, 2020 1:08 pm, Fabian Ebner wrote:
> and also return the ID of the allocated volume. This option
> allows plugins to choose a new name if there is a collision.
>
> In storage_migrate, the API version for the receiving side is checked.
>
> In Storage.pm's volume_import, when a plugin returns 'undef',
> it can be assumed that the import with the requested volid was
> successful (it should've died otherwise) and so volid is returned.
> This is done for backwards compatibility with foreign plugins.
>
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
> PVE/CLI/pvesm.pm | 22 ++++++++++----
> PVE/Storage.pm | 56 +++++++++++++++++++++++++++---------
> PVE/Storage/LVMPlugin.pm | 17 +++++++----
> PVE/Storage/Plugin.pm | 16 +++++++----
> PVE/Storage/ZFSPoolPlugin.pm | 13 +++++----
> 5 files changed, 89 insertions(+), 35 deletions(-)
>
> diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
> index 7c0e259..bc7e5cc 100755
> --- a/PVE/CLI/pvesm.pm
> +++ b/PVE/CLI/pvesm.pm
> @@ -321,9 +321,16 @@ __PACKAGE__->register_method ({
> maxLength => 80,
> optional => 1,
> },
> + 'allow-rename' => {
> + description => "Choose a new volume ID if the requested " .
> + "volume ID already exists, instead of throwing an error.",
> + type => 'boolean',
> + optional => 1,
> + default => 0,
> + },
> },
> },
> - returns => { type => 'null' },
> + returns => { type => 'string' },
> code => sub {
> my ($param) = @_;
>
> @@ -376,11 +383,11 @@ __PACKAGE__->register_method ({
> my $cfg = PVE::Storage::config();
> my $volume = $param->{volume};
> my $delete = $param->{'delete-snapshot'};
> - PVE::Storage::volume_import($cfg, $infh, $volume, $param->{format},
> - $param->{base}, $param->{'with-snapshots'});
> - PVE::Storage::volume_snapshot_delete($cfg, $volume, $delete)
> + my $imported_volid = PVE::Storage::volume_import($cfg, $infh, $volume, $param->{format},
> + $param->{base}, $param->{'with-snapshots'}, $param->{'allow-rename'});
> + PVE::Storage::volume_snapshot_delete($cfg, $imported_volid, $delete)
> if defined($delete);
> - return;
> + return $imported_volid;
> }
> });
>
> @@ -801,7 +808,10 @@ our $cmddef = {
> path => [ __PACKAGE__, 'path', ['volume']],
> extractconfig => [__PACKAGE__, 'extractconfig', ['volume']],
> export => [ __PACKAGE__, 'export', ['volume', 'format', 'filename']],
> - import => [ __PACKAGE__, 'import', ['volume', 'format', 'filename']],
> + import => [ __PACKAGE__, 'import', ['volume', 'format', 'filename'], {}, sub {
> + my $volid = shift;
> + print PVE::Storage::volume_imported_message($volid);
> + }],
> apiinfo => [ __PACKAGE__, 'apiinfo', [], {}, sub {
> my $res = shift;
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index ab6a543..cac3ba7 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -39,11 +39,11 @@ use PVE::Storage::DRBDPlugin;
> use PVE::Storage::PBSPlugin;
>
> # Storage API version. Icrement it on changes in storage API interface.
> -use constant APIVER => 3;
> +use constant APIVER => 4;
> # Age is the number of versions we're backward compatible with.
> # This is like having 'current=APIVER' and age='APIAGE' in libtool,
> # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 2;
> +use constant APIAGE => 3;
>
> # load standard plugins
> PVE::Storage::DirPlugin->register();
> @@ -578,7 +578,7 @@ sub storage_migrate {
> my $scfg = storage_config($cfg, $storeid);
>
> # no need to migrate shared content
> - return if $storeid eq $target_storeid && $scfg->{shared};
> + return $volid if $storeid eq $target_storeid && $scfg->{shared};
>
> my $tcfg = storage_config($cfg, $target_storeid);
>
> @@ -611,6 +611,12 @@ sub storage_migrate {
> $import_fn = "tcp://$net";
> }
>
> + my $target_apiver = 1; # if there is no apiinfo call, assume 1
> + my $get_api_version = [@$ssh, 'pvesm', 'apiinfo'];
> + my $match_api_version = sub { $target_apiver = $1 if $_[0] =~ m!^APIVER (\d+)$!; };
> + eval { run_command($get_api_version, logfunc => $match_api_version); };
> +
> + $with_snapshots = $with_snapshots ? 1 : 0; # sanitize for passing as cli parameter
this slipped through? already happens at the start of this sub
> my $send = ['pvesm', 'export', $volid, $format, '-', '-with-snapshots', $with_snapshots];
> my $recv = [@$ssh, '--', 'pvesm', 'import', $target_volid, $format, $import_fn, '-with-snapshots', $with_snapshots];
> if (defined($snapshot)) {
> @@ -619,6 +625,7 @@ sub storage_migrate {
> if ($migration_snapshot) {
> push @$recv, '-delete-snapshot', $snapshot;
> }
> + push @$recv, '-allow-rename', $allow_rename if $target_apiver >= 4;
>
> if (defined($base_snapshot)) {
> # Check if the snapshot exists on the remote side:
> @@ -626,6 +633,19 @@ sub storage_migrate {
> push @$recv, '-base', $base_snapshot;
> }
>
> + my $new_volid;
> + my $pattern = volume_imported_message(undef, 1);
> + my $match_volid_and_log = sub {
> + my $line = shift;
> +
> + $new_volid = $1 if ($line =~ m!$pattern!);
see below
> +
> + if ($logfunc) {
> + chomp($line);
> + $logfunc->($line);
> + }
> + };
> +
> volume_snapshot($cfg, $volid, $snapshot) if $migration_snapshot;
> eval {
> if ($insecure) {
> @@ -643,13 +663,8 @@ sub storage_migrate {
> shutdown($socket, 1);
>
> # wait for the remote process to finish
> - if ($logfunc) {
> - while (my $line = <$info>) {
> - chomp($line);
> - $logfunc->("[$target_sshinfo->{name}] $line");
> - }
> - } else {
> - 1 while <$info>;
> + while (my $line = <$info>) {
> + $match_volid_and_log->("[$target_sshinfo->{name}] $line");
> }
>
> # now close the socket
> @@ -659,8 +674,11 @@ sub storage_migrate {
> die "import failed: exit code ".($?>>8)."\n";
> }
> } else {
> - run_command([$send, @cstream, $recv], logfunc => $logfunc);
> + run_command([$send, @cstream, $recv], logfunc => $match_volid_and_log);
> }
> +
> + die "unable to get ID of the migrated volume\n"
> + if !defined($new_volid) && $target_apiver >= 4;
> };
> my $err = $@;
> warn "send/receive failed, cleaning up snapshot(s)..\n" if $err;
> @@ -669,6 +687,8 @@ sub storage_migrate {
> warn "could not remove source snapshot: $@\n" if $@;
> }
> die $err if $err;
> +
> + return $new_volid // $target_volid;
> }
>
> sub vdisk_clone {
> @@ -1425,14 +1445,14 @@ sub volume_export {
> }
>
> sub volume_import {
> - my ($cfg, $fh, $volid, $format, $base_snapshot, $with_snapshots) = @_;
> + my ($cfg, $fh, $volid, $format, $base_snapshot, $with_snapshots, $allow_rename) = @_;
>
> 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, $allow_rename) // $volid;
> }
>
> sub volume_export_formats {
> @@ -1467,6 +1487,16 @@ sub volume_transfer_formats {
> return @common;
> }
>
> +sub volume_imported_message {
> + my ($volid, $want_pattern) = @_;
> +
> + if ($want_pattern) {
> + return "successfully imported '([^']*)'\$";
why not return a quoted RE via qr here?
> + } else {
> + return "successfully imported '$volid'\n";
> + }
> +}
> +
> # bash completion helper
>
> sub complete_storage {
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index c9fc191..c0740d4 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -631,7 +631,7 @@ 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, $allow_rename) = @_;
> 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"
> @@ -645,17 +645,20 @@ sub volume_import {
>
> my $vg = $scfg->{vgname};
> my $lvs = lvm_list_volumes($vg);
> - die "volume $vg/$volname already exists\n"
> - if $lvs->{$vg}->{$volname};
> + if ($lvs->{$vg}->{$volname}) {
> + die "volume $vg/$volname already exists\n" if !$allow_rename;
> + warn "volume $vg/$volname already exists - importing with a different name\n";
> + $name = undef;
> + }
>
> my ($size) = PVE::Storage::Plugin::read_common_header($fh);
> $size = int($size/1024);
>
> eval {
> my $allocname = $class->alloc_image($storeid, $scfg, $vmid, 'raw', $name, $size);
> - if ($allocname ne $volname) {
> - my $oldname = $volname;
> - $volname = $allocname; # Let the cleanup code know what to free
> + my $oldname = $volname;
> + $volname = $allocname;
> + if (defined($name) && $allocname ne $oldname) {
> die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
> }
> my $file = $class->path($scfg, $volname, $storeid)
> @@ -668,6 +671,8 @@ sub volume_import {
> warn $@ if $@;
> die $err;
> }
> +
> + return "$storeid:$volname";
> }
>
> sub volume_import_write {
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 2232261..4299eb5 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -1222,7 +1222,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, $allow_rename) = @_;
>
> die "volume import format '$format' not available for $class\n"
> if $format !~ /^(raw|tar|qcow2|vmdk)\+size$/;
> @@ -1244,16 +1244,20 @@ sub volume_import {
> # Check for an existing file first since interrupting alloc_image doesn't
> # free it.
> my $file = $class->path($scfg, $volname, $storeid);
> - die "file '$file' already exists\n" if -e $file;
> + if (-e $file) {
> + die "file '$file' already exists\n" if !$allow_rename;
> + warn "file '$file' already exists - importing with a different name\n";
> + $name = undef;
> + }
>
> my ($size) = read_common_header($fh);
> $size = int($size/1024);
>
> eval {
> my $allocname = $class->alloc_image($storeid, $scfg, $vmid, $file_format, $name, $size);
> - if ($allocname ne $volname) {
> - my $oldname = $volname;
> - $volname = $allocname; # Let the cleanup code know what to free
> + my $oldname = $volname;
> + $volname = $allocname;
> + if (defined($name) && $allocname ne $oldname) {
> die "internal error: unexpected allocated name: '$allocname' != '$oldname'\n";
> }
> my $file = $class->path($scfg, $volname, $storeid)
> @@ -1273,6 +1277,8 @@ sub volume_import {
> warn $@ if $@;
> die $err;
> }
> +
> + return "$storeid:$volname";
> }
>
> sub volume_import_formats {
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index b538e3b..b4fd65f 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -743,7 +743,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, $allow_rename) = @_;
>
> die "unsupported import stream format for $class: $format\n"
> if $format ne 'zfs';
> @@ -752,15 +752,18 @@ sub volume_import {
> die "internal error: invalid file handle for volume_import\n"
> if !defined($fd);
>
> - my $dataset = ($class->parse_volname($volname))[1];
> + 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;
> - } else {
> - die "volume '$zfspath' already exists\n" if $exists;
> + } elsif ($exists) {
> + die "volume '$zfspath' already exists\n" if !$allow_rename;
> + warn "volume '$zfspath' already exists - importing with a different name\n";
> + $dataset = $class->find_free_diskname($storeid, $scfg, $vmid, $format);
> + $zfspath = "$scfg->{pool}/$dataset";
> }
>
> eval { run_command(['zfs', 'recv', '-F', '--', $zfspath], input => "<&$fd") };
> @@ -773,7 +776,7 @@ sub volume_import {
> die $err;
> }
>
> - return;
> + return "$storeid:$dataset";
> }
>
> sub volume_import_formats {
> --
> 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