[pve-devel] [PATCH container v2] Fix #2109: resize rbd volume for container failed

[Thomas Lamprecht]

On 3/5/19 12:07 PM, Alwin Antreich wrote:
> The returned path of the volume was not a mapped device. This patch uses
> un-/map_volume for rbd storage to get a device mapped and its path.

commit message was not updated for v2, it does it now for all storages
having a map_volume, if in the future one such gets added (iSCIS for CTs?)
as no hardcoded 'rbd' check is there anymore.

> 
> Signed-off-by: Alwin Antreich <a.antreich at proxmox.com>
> ---
> V1 -> V2: implemented suggestions from Thomas
> 	https://pve.proxmox.com/pipermail/pve-devel/2019-March/035896.html

but I effectively suggested two different alternative ways, so it's not clear
what changed, or why you chosen one. Is one approach wrong, was it just easier
for now this way, ...?
I may annoy you with those request but IMO it really would be great if you write
out what changed in short bulletin points. Helps when looking back at this, on
review and to ensure both author and reviewers understood each others points.

One, as this naturally does not only applies for you, could take a look at some
series with multiple revisions from Dominik or Fabian as an example how to do this
nicely.

> 
>  src/PVE/API2/LXC.pm | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
> index 27d26d5..b8d3f42 100644
> --- a/src/PVE/API2/LXC.pm
> +++ b/src/PVE/API2/LXC.pm
> @@ -1582,6 +1582,8 @@ __PACKAGE__->register_method({
>  
>  	    PVE::Storage::activate_volumes($storage_cfg, [$volid]);
>  
> +	    my $path = PVE::Storage::map_volume($storage_cfg, $volid) // PVE::Storage::path($storage_cfg, $volid);
> +

why do you do this here and not below in real command?
* often it should be tried to put declaration should be near usage
* besides that style nit you actually run into an "issue" where the image gets
  mapped, then the "$newsize < $size" check asserts and you do not clean up after
  that. 

so...

>  	    my $size = PVE::Storage::volume_size_info($storage_cfg, $volid, 5);
>  	    $newsize += $size if $ext;
>  	    $newsize = int($newsize);
> @@ -1602,7 +1604,6 @@ __PACKAGE__->register_method({
>  		PVE::LXC::Config->write_config($vmid, $conf);
>  
>  		if ($format eq 'raw') {
> -		    my $path = PVE::Storage::path($storage_cfg, $volid, undef);

... why not keep it here? It's only used in this scope, or do I overlook something?

>  		    if ($running) {
>  
>  			$mp->{mp} = '/';
> @@ -1630,6 +1631,8 @@ __PACKAGE__->register_method({
>  			    PVE::Tools::run_command(['resize2fs', $path]);
>  			};
>  			warn "Failed to update the container's filesystem: $@\n" if $@;
> +
> +			PVE::Storage::unmap_volume($storage_cfg, $volid);
>  		    }
>  		}
>  	    };
>