[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