[pve-devel] [PATCH container 3/3] collect errors from all local volumes

[Wolfgang Bumiller]

Minor (non-)issues inline, otherwise the whole series is Acked-by me.

On Thu, Jul 07, 2016 at 04:45:27PM +0200, Fabian Grünbichler wrote:
> and then die with more meaningful/complete output, instead
> of on the first encountered error.
> ---
>  src/PVE/LXC/Migrate.pm | 65 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
> index 83d2cb9..e846e3b 100644
> --- a/src/PVE/LXC/Migrate.pm
> +++ b/src/PVE/LXC/Migrate.pm
> @@ -101,6 +101,16 @@ sub phase1 {
>  
>      $self->{volumes} = []; # list of already migrated volumes
>      my $volhash = {}; # 'config', 'snapshot' or 'storage' for local volumes
> +    my $volhash_errors = {};
> +    my $other_errors = [];

I don't see $other_errors being used anywhere - leftover from previous
version or part of another pending patch?

> +    my $abort = 0;
> +
> +    my $log_error = sub {
> +	my ($msg, $volid) = @_;
> +
> +	$volhash_errors->{$volid} = $msg if !defined($volhash_errors->{$volid});
> +	$abort = 1;
> +    };
>  
>      my $test_volid = sub {
>  	my ($volid, $snapname) = @_;
> @@ -113,13 +123,17 @@ sub phase1 {
>  	my $scfg = PVE::Storage::storage_check_node($self->{storecfg}, $sid);
>  	PVE::Storage::storage_check_node($self->{storecfg}, $sid, $self->{node});
>  
> -	return if $scfg->{shared};
> +	if ($scfg->{shared}) {
> +	    $self->log('info', "volume '$volid' is on shared storage '$sid'")
> +		if !$snapname;
> +	    return;
> +	}
>  
>  	$volhash->{$volid} = defined($snapname) ? 'snapshot' : 'config';
>  
>  	my ($path, $owner) = PVE::Storage::path($self->{storecfg}, $volid);
>  
> -	die "can't migrate volume '$volid' - owned by other guest (owner = $owner)\n"
> +	die "owned by other guest (owner = $owner)\n"
>  	    if !$owner || ($owner != $self->{vmid});
>  
>  	if (defined($snapname)) {
> @@ -128,7 +142,7 @@ sub phase1 {
>  	    if (($scfg->{type} eq 'zfspool')) {
>  		return;
>  	    }
> -	    die "can't migrate snapshot of local volume '$volid'\n";
> +	    die "non-migratable snapshot exists\n";
>  	}
>      };
>  
> @@ -144,17 +158,11 @@ sub phase1 {
>  	    return;
>  	}
>  
> -	my ($storage, $volname) = PVE::Storage::parse_volume_id($volid);
> -	my $scfg = PVE::Storage::storage_check_node($self->{storecfg}, $storage);
> +	eval {
> +	    &$test_volid($volid, $snapname);
> +	};
>  
> -	if (!$scfg->{shared}) {
> -	    $self->log('info', "copy mountpoint '$ms' ($volid) to node ' $self->{node}'")
> -		if !$snapname;
> -	} else {
> -	    $self->log('info', "mountpoint '$ms' is on shared storage '$storage'")
> -		if !$snapname;
> -	}
> -	&$test_volid($volid, $snapname);
> +	&$log_error($@, $volid) if $@;
>      };
>  
>      # first unused / lost volumes owned by this container
> @@ -192,19 +200,22 @@ sub phase1 {
>  
>      # additional checks for local storage
>      foreach my $volid (keys %$volhash) {
> -	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
> -	my $scfg =  PVE::Storage::storage_config($self->{storecfg}, $sid);
> +	eval {
> +	    my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
> +	    my $scfg =  PVE::Storage::storage_config($self->{storecfg}, $sid);
>  
> -	my $migratable = ($scfg->{type} eq 'dir') || ($scfg->{type} eq 'zfspool') ||
> -	    ($scfg->{type} eq 'lvmthin') || ($scfg->{type} eq 'lvm');
> +	    my $migratable = ($scfg->{type} eq 'dir') || ($scfg->{type} eq 'zfspool') ||
> +	    	($scfg->{type} eq 'lvmthin') || ($scfg->{type} eq 'lvm');

indentation error
/home/wbumiller/Sources/pve-container/.git/rebase-apply/patch:90: space before tab in indent.
                ($scfg->{type} eq 'lvmthin') || ($scfg->{type} eq 'lvm');

>  
> -	die "can't migrate '$volid' - storage type '$scfg->{type}' not supported\n"
> -	    if !$migratable;
> +	    die "storage type '$scfg->{type}' not supported\n"
> +	    	if !$migratable;

indentation error
/home/wbumiller/Sources/pve-container/.git/rebase-apply/patch:95: space before tab in indent.
                if !$migratable;

>  
> -	# image is a linked clone on local storage, se we can't migrate.
> -	if (my $basename = (PVE::Storage::parse_volname($self->{storecfg}, $volid))[3]) {
> -	    die "can't migrate '$volid' as it's a clone of '$basename'";
> -	}
> +	    # image is a linked clone on local storage, se we can't migrate.
> +	    if (my $basename = (PVE::Storage::parse_volname($self->{storecfg}, $volid))[3]) {
> +		die "clone of '$basename'";
> +	    }
> +	};
> +	&$log_error($@, $volid) if $@;
>      }
>  
>      foreach my $volid (sort keys %$volhash) {
> @@ -219,6 +230,14 @@ sub phase1 {
>  	}
>      }
>  
> +    foreach my $volid (sort keys %$volhash_errors) {
> +	$self->log('warn', "can't migrate local volume '$volid': $volhash_errors->{$volid}");
> +    }
> +
> +    if ($abort) {
> +	die "can't migrate CT - check log\n";
> +    }
> +
>      foreach my $volid (keys %$volhash) {
>  	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid);
>  	push @{$self->{volumes}}, $volid;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>