[virt-tools-list] [PATCH 1/3] virtinst: Add vsock device type
Pavel Hrdina
phrdina at redhat.com
Mon Dec 10 11:09:18 UTC 2018
On Mon, Dec 10, 2018 at 12:00:07PM +0200, Slavomir Kaslev wrote:
> On Mon, Dec 10, 2018 at 11:46:14AM +0800, Pavel Hrdina wrote:
> > On Fri, Dec 07, 2018 at 05:02:14PM +0200, Slavomir Kaslev wrote:
> > > Signed-off-by: Slavomir Kaslev <kaslevs at vmware.com>
> > > ---
> > > virtinst/devices/__init__.py | 1 +
> > > virtinst/devices/vsock.py | 41 ++++++++++++++++++++++++++++++++++++
> > > virtinst/guest.py | 3 ++-
> > > 3 files changed, 44 insertions(+), 1 deletion(-)
> > > create mode 100644 virtinst/devices/vsock.py
> >
> > This patch fails './setup.py test'.
>
> That's interesting. This is the output of './setup.py test' on my machine:
>
> $ ./setup.py test
> running test
> ...........................................................................................................................ssss..........................................................................................................s.s..s...s.s....ss.ssss....s........................................................................................................................................................................................s
> ----------------------------------------------------------------------
> Ran 446 tests in 7.120s
>
> OK (skipped=17)
>
> Am I running it with wrong flags?
No, it's just not ideal test code that we have in virt-manager :/.
If you look into tests/checkprops.py into the testCheckProps() method
you will see that if any test failed/was skipped we will not run this
specific test case which would tell you this:
======================================================================
FAIL: testCheckProps (tests.checkprops.CheckPropsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/phrdina/work/virt-manager/tests/checkprops.py", line 51, in testCheckProps
self.fail(msg)
AssertionError: Traceback (most recent call last):
File "/home/phrdina/work/virt-manager/tests/checkprops.py", line 41, in testCheckProps
self.assertEqual([], fail)
File "/usr/lib64/python3.7/unittest/case.py", line 856, in assertEqual
assertion_func(first, second, msg=msg)
File "/usr/lib64/python3.7/unittest/case.py", line 1062, in assertListEqual
self.assertSequenceEqual(list1, list2, msg, seq_type=list)
File "/usr/lib64/python3.7/unittest/case.py", line 1044, in assertSequenceEqual
self.fail(msg)
File "/usr/lib64/python3.7/unittest/case.py", line 697, in fail
raise self.failureException(msg)
AssertionError: Lists differ: [] != [<XMLProperty ./@model 139788382535496>, <[82 chars]712>]
Second list contains 3 additional elements.
First extra element 0:
<XMLProperty ./@model 139788382535496>
- []
+ [<XMLProperty ./@model 139788382535496>,
+ <XMLProperty ./cid/@auto 139788382535592>,
+ <XMLProperty ./cid/@address 139788382064712>]
This means that there are XML properties that are
untested in the test suite. This could be caused
by a previous test suite failure, or if you added
a new property and didn't extend the test suite.
Look into extending clitest.py and/or xmlparse.py.
The reason for having skipped tests is probably that you have old
libvirt on your system.
> > Every new XML element should have
> > it's command line option for virt-install and also we require some basic
> > test to be available. So you need to extend this patch to introduce
> > command line option(s).
>
> OK, I'll add it to virt-install cli in v2. Can you point me to an existing test
> I can use as reference?
You can check this commit <eb1a67a595e6e631479376eb4aa82c680e7ae56e>
which added new options for existing command, that should help you
figure out what to do.
> >
> > > diff --git a/virtinst/devices/__init__.py b/virtinst/devices/__init__.py
> > > index 6da0766d..6120f5d0 100644
> > > --- a/virtinst/devices/__init__.py
> > > +++ b/virtinst/devices/__init__.py
> > > @@ -22,6 +22,7 @@ from .redirdev import DeviceRedirdev
> > > from .rng import DeviceRng
> > > from .tpm import DeviceTpm
> > > from .video import DeviceVideo
> > > +from .vsock import DeviceVsock
> > > from .watchdog import DeviceWatchdog
> > >
> > >
> > > diff --git a/virtinst/devices/vsock.py b/virtinst/devices/vsock.py
> > > new file mode 100644
> > > index 00000000..beb00e50
> > > --- /dev/null
> > > +++ b/virtinst/devices/vsock.py
> > > @@ -0,0 +1,41 @@
> > > +# Copyright (C) 2018 VMware, Inc.
> > > +#
> > > +# Copyright 2018
> > > +# Slavomir Kaslev <kaslevs at vmware.com>
> > > +#
> > > +# This work is licensed under the GNU GPLv2 or later.
> > > +# See the COPYING file in the top-level directory.
> > > +
> > > +from .device import Device
> > > +from ..xmlbuilder import XMLProperty
> > > +
> > > +
> > > +class DeviceVsock(Device):
> > > + XML_NAME = "vsock"
> > > +
> > > + model = XMLProperty("./@model")
> > > + auto_cid = XMLProperty("./cid/@auto", is_yesno=True)
> > > + cid = XMLProperty("./cid/@address", is_int=True)
> > > +
> > > +
> > > + ##############
> > > + # Validation #
> > > + ##############
> > > +
> > > + def validate(self):
> > > + if not self.auto_cid and (self.cid is None or self.cid < 3):
> > > + raise ValueError(_("guest CID {0} must be >= 3").format(self.cid))
> >
> > It would be better to have some const variable, for example
> > CID_MIN_GUEST_ADDRESS or something like that instead of "magic number".
>
> Makes sense. I added `VMADDR_CID_HOST = 2` as constant to make it clearer.
>
> >
> > > +
> > > +
> > > + ##################
> > > + # Default config #
> > > + ##################
> > > +
> > > + def set_defaults(self, guest):
> > > + if not self.model:
> > > + self.model = "virtio"
> > > +
> > > + if self.auto_cid is None and self.cid is None:
> > > + self.auto_cid = True
> > > + if self.cid is None and not self.auto_cid:
> > > + self.cid = 3
> >
> > I think that it would be better not to have any default CID. We can
> > default to set cid auto to True if nothing is provided and if cid auto
> > is set to False we should require cid address.
>
> I don't quite follow. If auto_cid is not True and no CID is provided, should
> set_defaults raise an exception (OTOH no other Device.set_defaults raises) or
> just ignore it here and let libvirt catch it and complain?
Set defaults should do nothing in that case and the validate method
should handle the error and it will print it if you add CLI option and
use it like this:
virt-install .... --vsock cid_auto=no
I just tried libvirt and it will complain only when you try to start
guest with device like this:
<vsock model='virtio'>
<cid auto='no'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
</vsock>
sigh, it should complain while defining such guest :/.
So it would be nice if virt-install/virt-xml would complain if you try
to use that configuration.
Pavel
> Thank you,
>
> -- Slavi
>
> >
> > Otherwise looks good.
> >
> > Pavel
> >
> > > diff --git a/virtinst/guest.py b/virtinst/guest.py
> > > index eeb40cb6..9acff3b9 100644
> > > --- a/virtinst/guest.py
> > > +++ b/virtinst/guest.py
> > > @@ -29,7 +29,7 @@ class _DomainDevices(XMLBuilder):
> > > 'smartcard', 'serial', 'parallel', 'console', 'channel',
> > > 'input', 'tpm', 'graphics', 'sound', 'video', 'hostdev',
> > > 'redirdev', 'watchdog', 'memballoon', 'rng', 'panic',
> > > - 'memory']
> > > + 'memory', 'vsock']
> > >
> > >
> > > disk = XMLChildProperty(DeviceDisk)
> > > @@ -53,6 +53,7 @@ class _DomainDevices(XMLBuilder):
> > > rng = XMLChildProperty(DeviceRng)
> > > panic = XMLChildProperty(DevicePanic)
> > > memory = XMLChildProperty(DeviceMemory)
> > > + vsock = XMLChildProperty(DeviceVsock)
> > >
> > > def get_all(self):
> > > retlist = []
> > > --
> > > 2.19.1
> > >
> > > _______________________________________________
> > > virt-tools-list mailing list
> > > virt-tools-list at redhat.com
> > > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
>
>
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20181210/397d45cc/attachment.sig>
More information about the virt-tools-list
mailing list