[virt-manager PATCH 0/2] Couple of bhyve related improvements

Cole Robinson crobinso at redhat.com
Wed Feb 10 17:31:10 UTC 2021


On 2/9/21 10:51 AM, Roman Bogorodskiy wrote:
>   Cole Robinson wrote:
> 
>> On 2/6/21 8:18 AM, Roman Bogorodskiy wrote:
>>> Roman Bogorodskiy (2):
>>>   virtinst: prefer SATA bus for bhyve
>>>   virtinst: bhyve: properly configure loader
>>>
>>>  virtinst/connection.py   |  2 ++
>>>  virtinst/devices/disk.py |  3 +++
>>>  virtinst/domain/os.py    | 16 ++++++++++++++++
>>>  3 files changed, 21 insertions(+)
>>>
>>
>> Thanks for the patches. We need to add test coverage though, which since
>> these will be the first bhyve tests will take some plumbing.
>>
>> * Drop bhyve `virsh capabilities` in tests/data/capabilities/bhyve.xml
>> * Drop bhyve `virsh domcapabilities` in
>> tests/data/capabilities/bhyve-domcaps.xml
>> * In tests/utils.py:_URIs, add self.bhyve = _m("bhyve:///") +
>> _caps("bhyve.xml") + _domcaps("bhyve-domcaps.xml")
>> * in tests/test_cli.py add something like this:
>>
>> ```
>> ########################
>>
>> # bhyve specific tests #
>>
>> ########################
>>
>> c = vinst.add_category("bhyve", "--name foobhyve --noautoconsole
>> --connect " + utils.URIs.bhyve)
>> c.add_compare("--os-variant fedora27", "bhyve-default-f27")
>> ```
>>
>> * Run the test suite, verify the generated
>> ./tests/data/cli/compare/virt-install-bhyve-default-f27.xml looks sane.
>>
>>
>> That will all be the first commit. The next patches should cause test
>> output changes, use `pytest --regenerate-output` to refresh the test XML
>> and commit the difference.
>>
>> Also check `pytest --cov` doesn't regress
>>
>> Thanks,
>> Cole
>>
>>
>>
>> - Cole
>>
> 
> Thanks for the detailed instruction, will do that.
> 
> BTW, I noticed a strange issue with test_cli.py which I was hoping to
> fix later, but apparently need to figure out first for smooth testing:
> 
> Many tests are failing with the following trace:
> 
> Traceback (most recent call last):                                                                                                                                                                                  
>   File "/usr/home/novel/code/virt-manager/tests/test_cli.py", line 228, in _launch_command                                                                                                                          
>     ret = virtinstall.main(conn=conn)                                                                                                                                                                               
>   File "/usr/home/novel/code/virt-manager/virtinst/virtinstall.py", line 1145, in main                    
>     guest, installer = build_guest_instance(conn, options)                                                                                                                                                          
>   File "/usr/home/novel/code/virt-manager/virtinst/virtinstall.py", line 598, in build_guest_instance                                                                                                               
>     cli.validate_disk(disk)                                                                                                                                                                                         
>   File "/usr/home/novel/code/virt-manager/virtinst/cli.py", line 377, in validate_disk                    
>     check_inuse_conflict()                                                                                                                                                                                          
>   File "/usr/home/novel/code/virt-manager/virtinst/cli.py", line 359, in check_inuse_conflict                                                                                                                       
>     names = dev.is_conflict_disk()                                                                        
>   File "/usr/home/novel/code/virt-manager/virtinst/devices/disk.py", line 832, in is_conflict_disk        
>     read_only=self.read_only)                                                                                                                                                                                       
>   File "/usr/home/novel/code/virt-manager/virtinst/devices/disk.py", line 322, in path_in_use_by          
>     checkpath = disk.get_source_path()                                                                                                                                                                              
>   File "/usr/home/novel/code/virt-manager/virtinst/devices/disk.py", line 629, in get_source_path                                                                                                                   
>     self._resolve_storage_backend()                                                                                                                                                                                 
>   File "/usr/home/novel/code/virt-manager/virtinst/devices/disk.py", line 616, in _resolve_storage_backend 
>     self.conn, path)                                                                                                                                                                                                
>   File "/usr/home/novel/code/virt-manager/virtinst/diskbackend.py", line 143, in manage_path              
>     if not conn.support.conn_storage():                                                                                                                                                                             
> ReferenceError: weakly-referenced object no longer exists 
> 
> Traceback is always similar. I tried to find a point where it loses the
> reference using pdb and it happens to be here:
> 
>   File "/usr/home/novel/code/virt-manager/virtinst/devices/disk.py", line 322, in path_in_use_by          
>     checkpath = disk.get_source_path()                                                                                                                                                                              
>   File "/usr/home/novel/code/virt-manager/virtinst/devices/disk.py", line 629, in get_source_path                                                                                                                   
>     self._resolve_storage_backend()                                                                                                                                                                                 
> 
> In path_in_use_by() I can still access 'conn', but in get_source_path()
> self.conn is already lost. I was able to add a workaround like:
> 
> diff --git a/virtinst/devices/disk.py b/virtinst/devices/disk.py
> index a8971581..7e609914 100644
> --- a/virtinst/devices/disk.py
> +++ b/virtinst/devices/disk.py
> @@ -319,6 +319,7 @@ class DeviceDisk(Device):
>                      continue
> 
>              for disk in vm.devices.disk:
> +                disk.conn = conn
>                  checkpath = disk.get_source_path()
>                  if checkpath in vols and vm.name not in ret:
>                      # VM uses the path indirectly via backing store
> 
> But that doesn't seem like a proper fix.
> Also wondering why it doesn't show up for others? Doesn't seem to have
> anything FreeBSD specific happening here.
> 

Hmm interesting. I've never seen that error before. I will poke at the
code and try to determine what is happening.

Are the errors deterministic? Can you pastebin a full `pytest` run
either way?

No clue why freebsd would trigger things differently here but maybe it's
a garbage collection timing issue or something

Would also be interesting to know if this change works around the issue,
test suite caching speedups could be responsible:

diff --git a/tests/utils.py b/tests/utils.py
index 16ba26b4..0935f338 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -145,6 +145,7 @@ class _URIs(object):
         # an option to test the stock code
         if uri == self.test_default:
             return conn
+        return conn

         if uri not in self._conn_cache:
             conn.fetch_all_domains()

- Cole




More information about the virt-tools-list mailing list