On 2/11/21 6:34 AM, Roman Bogorodskiy wrote:
> Cole Robinson wrote:
>
>> 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?
>
> Errors are almost deterministic. "Almost" in a sense that a couple of
> times I got successful runs. E.g. I was updating master branch today,
> then executed `pytest tests/test_cli.py` and it passed. I was surprised,
> restarted, and it failed like before. Once it starts failing, it fails
> the same way.
>
> Sample run:
>
> https://people.freebsd.org/~novel/misc/virt-manager-test_cli_weakref_error.txt
>
Strange. Not really sure how to figure it out. I'll probably need to get
my hands on a freebsd setup.
>> 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:
>
> This change fixes the issue.
>
Thanks for checking
- Cole
