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
> 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.
> 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
>
Roman Bogorodskiy
Attachment:
signature.asc
Description: PGP signature
