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

[Cole Robinson]

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?

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)