[virt-tools-list] [PATCH] fix: do cleanupLocation only at last time for cached fetcher

[Chun Yan Liu]

>>> On 9/14/2015 at 10:36 PM, in message <55F6DB84.3060809 at redhat.com>, Cole
Robinson <crobinso at redhat.com> wrote: 
> On 09/13/2015 10:06 PM, Chun Yan Liu wrote: 
> >  
> >  
> >>>> On 9/11/2015 at 11:22 PM, in message <55F2F19A.30302 at redhat.com>, Cole  
> Robinson 
> > <crobinso at redhat.com> wrote:  
> >> On 09/11/2015 01:12 AM, Chun Yan Liu wrote:  
> >>>   
> >>>   
> >>>>>> On 9/9/2015 at 08:07 PM, in message <55F020E4.40206 at redhat.com>, Cole   
> >> Robinson  
> >>> <crobinso at redhat.com> wrote:   
> >>>> On 09/09/2015 01:56 AM, Chunyan Liu wrote:   
> >>>>> Regression introduced after commit d8d6af5. Failed to install guests   
> >>>>> through ftp network source, see below:   
> >>>>>    
> >>>>> /usr/bin/virt-install   --name sled-12-fcs-64-pv-nnm-nete5250fdfecc94f922b5a21a   
> >>>>>  -p --location ftp://xen100.virt.lab.novell.com/install/sled12/x86_64   
> >>>>>  --extra-args "autoyast=http://192.168.123.1/install/autoyast/   
> >>>>> sled-12-fcs-64-pv-nnm-nete5250fdfecc94f922b5a21a"   --disk path=/var/lib/   
> >>>>> libvirt/images/sled-12-fcs-64-pv-nnm-nete5250fdfecc94f922b5a21a.qcow2,size=10,   
> >>>>> format=qcow2   --network=bridge=br0   --ram=1024   --vcpu=2   --vnc   
> >>>>>  --serial pty   --noautoconsole   --video cirrus   
> >>>>>    
> >>>>> ERROR    'NoneType' object has no attribute 'sendall'   
> >>>>> Domain installation does not appear to have been successful.   
> >>>>> If it was, you can restart your domain by running:   
> >>>>>   virsh --connect xen:/// start sled-12-fcs-64-pv-nnm-nete5250fdfecc94f922b5a21a   
> >>>>> otherwise, please restart your installation.   
> >>>>>    
> >>>>> Root cause is: now we use cached fetcher, but still do cleanupLocation   
> >>>>> each time, it will cause problem when using the fetcher next time.   
> >>>>> We should only do cleanupLocation at last time.   
> >>>>>    
> >>>>> Signed-off-by: Chunyan Liu <cyliu at suse.com>   
> >>>>> ---   
> >>>>>  virtinst/distroinstaller.py | 7 +++++--   
> >>>>>  1 file changed, 5 insertions(+), 2 deletions(-)   
> >>>>>    
> >>>>    
> >>>> I can't reproduce the issue, and in fact there isn't any call to 'sendall'   
>   
> >>   
> >>>   
> >>> 'sendall' is a function in FTPLib. It's called from ftp.quit in   
> >> FTPImageFetcher.  
> >>> This problem is only happens with 'ftp'. No problem if using 'http'.  
> >>>   
> >>   
> >> Yeah I tried with an ftp link too but couldn't reproduce (I can't use the   
> >> link  
> >> you provided because it's not a public URL). I still can't see in the code   
>  
> >> how  
> >  
> > Interesting. I did use virt-manager.git, and I tried another ftp link too, 
> > also has this issue.  
> >  
> >> cleanupLocation() can be called without a matching prepareLocation so not   
> >> sure  
> >> how this problem comes up in upstream code.  
> >  
> > Yes, prepareLocation and cleanupLocation appear in pair. That's the  
> problem. 
> > Now preparelocation will use cached ftp directly, from the debugging, after 
> > the 1st cleanupLocation, ftp.sock becomes None, so when 2nd time use that 
> > ftp, will cross problem as reported. 
> >  
>  
> I just tried reproducing again and indeed I can reproduce :/ I grabbed what 
> appeared to be an ftp:// URL out of tests/test_urls.py before, but several  
> of 
> them are actually ftp sites with http:// tacked on the front... I think I  
> just 
> saw the 'ftp' in the URL. Sorry about the confusion and thanks for following  
> up 
>  
> I pushed this patch which fixes things for me, can you try virt-manager.git 
> and report back? 

Tried, it works.

- Chunyan 

>  
> Thanks, 
> Cole 
>  
> commit 590f5a525bccded6866461b4ff3e1bb7adae086b 
> Author: Cole Robinson <crobinso at redhat.com> 
> Date:   Mon Sep 14 10:34:19 2015 -0400 
>  
>     urlfetcher: Clear cached ftp connection on cleanupLocation 
>  
>     Reported-by: Chun Yan Liu <cyliu at suse.com> 
>  
> diff --git a/virtinst/urlfetcher.py b/virtinst/urlfetcher.py 
> index 40f7c05..cda67a2 100644 
> --- a/virtinst/urlfetcher.py 
> +++ b/virtinst/urlfetcher.py 
> @@ -167,6 +167,7 @@ class _FTPImageFetcher(_URIImageFetcher): 
>          except: 
>              logging.debug("Error quitting ftp connection", exc_info=True) 
>  
> +        self.ftp = None 
>  
>      def hasFile(self, filename): 
>          path = self._make_path(filename) 
>  
>  
>