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

[Alwin Antreich]

On Wed, Mar 06, 2019 at 11:23:51AM +0100, Thomas Lamprecht wrote:
> 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);
> >  		    }
> >  		}
> >  	    };
> > 
I guess, I shot myself in the foot with this one. :/ A new version is coming.

Thanks guys for the patience.

--
Cheers,
Alwin