[virt-tools-list] [virt-manager PATCH 1/2] refactor detection of guest type capabilities
Pavel Hrdina
phrdina at redhat.com
Thu Jul 9 12:32:33 UTC 2015
On Wed, Jul 08, 2015 at 12:28:32PM -0400, Cole Robinson wrote:
> On 07/08/2015 05:33 AM, Pavel Hrdina wrote:
> > Each guest type can have its own capabilities and we should always ask
> > only for those capabilities.
> >
> > The old approach was to get capabilities from libvirt and then for
> > example cycle trough all guests and return True, if any guest type
> > supports kvm or pae, etc.
> >
> > Now we check those capabilities only for the correct guest type
> > according to defaults and input from user.
> >
>
> $ python setup.py pylint
> ************* Module virtManager.create
> E:881, 0: invalid syntax (syntax-error)
> ************* Module virtManager.engine
> F: 42, 0: Unable to import 'create' (invalid syntax (<string>, line 881))
> (import-error)
> running pep8
> virtManager/create.py:882: [E113] unexpected indentation
Right, I forget to add a colon after the condition. Note to myself, always run
also pylint together with test.
>
> I also get this failure:
>
> ======================================================================
> FAIL: testVMX2Libvirt (tests.virtconvtest.TestVirtConv)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/crobinso/src/virt-manager/tests/virtconvtest.py", line 91, in
> testVMX2Libvirt
> self._compare_files("vmx")
> File "/home/crobinso/src/virt-manager/tests/virtconvtest.py", line 86, in
> _compare_files
> self._compare_single_file(in_path, in_type)
> File "/home/crobinso/src/virt-manager/tests/virtconvtest.py", line 75, in
> _compare_single_file
> self._convert_helper(in_path, out_path, in_type, disk_format)
> File "/home/crobinso/src/virt-manager/tests/virtconvtest.py", line 58, in
> _convert_helper
> utils.diff_compare(out_expect, outfile)
> File "/home/crobinso/src/virt-manager/tests/utils.py", line 206, in diff_compare
> raise AssertionError("Conversion outputs did not match.\n%s" % diff)
> AssertionError: Conversion outputs did not match.
> ---
> /home/crobinso/src/virt-manager/tests/virtconv-files/libvirt_output/vmx2libvirt_test-vmx-zip.libvirt
> +++ Generated Output
> @@ -12,7 +12,6 @@
> <features>
> <acpi/>
> <apic/>
> - <pae/>
> <vmport state="off"/>
> </features>
> <cpu mode="custom" match="exact">
>
I don't get that error, maybe missing dependency or something else? I'll try to
investigate it.
>
>
> Few other comments below
>
> > diff --git a/tests/xmlconfig.py b/tests/xmlconfig.py
> > index 048675d..bf41c07 100644
> > --- a/tests/xmlconfig.py
> > +++ b/tests/xmlconfig.py
> > @@ -52,6 +52,8 @@ def _make_guest(installer=None, conn=None):
> > g.os.arch = "i686"
> > g.os.os_type = "hvm"
> >
> > + g.capsinfo = conn.caps.guest_lookup(g.os.os_type, g.os.arch)
> > +
>
> Instead I'd switch this routine to use lookup_virtinst_guest
That's good idea and better. I'll use it.
>
> > g.add_default_input_device()
> > g.add_default_console_device()
> > g.add_device(virtinst.VirtualAudio(g.conn))
> > diff --git a/virtManager/create.py b/virtManager/create.py
> > index 8401a7a..45de3c5 100644
> > --- a/virtManager/create.py
> > +++ b/virtManager/create.py
> > @@ -504,7 +504,7 @@ class vmmCreate(vmmGObjectUI):
> > self.widget("startup-error-box").hide()
> > self.widget("create-forward").set_sensitive(True)
> >
> > - if not self.conn.caps.has_install_options():
> > + if not self.capsinfo.guest.has_install_options():
> > error = _("No hypervisor options were found for this "
> > "connection.")
> >
> > @@ -539,7 +539,7 @@ class vmmCreate(vmmGObjectUI):
> > self.startup_warning(error)
> >
> > elif self.conn.is_qemu():
> > - if not self.conn.caps.is_kvm_available():
> > + if not self.capsinfo.guest.is_kvm_available():
> > error = _("KVM is not available. This may mean the KVM "
> > "package is not installed, or the KVM kernel modules "
> > "are not loaded. Your virtual machines may perform poorly.")
> > @@ -875,11 +875,10 @@ class vmmCreate(vmmGObjectUI):
> > break
> >
> > capsinfo = self.conn.caps.guest_lookup(os_type=gtype, arch=arch)
> > - newg, newdom = capsinfo.get_caps_objects()
> >
> > if self.capsinfo:
> > - oldg, olddom = self.capsinfo.get_caps_objects()
> > - if oldg == newg and olddom and newdom:
> > + if (self.capsinfo.guest == capsinfo.guest and
> > + self.capsinfo.domain == capsinfo.domain)
> > return
> >
> > self.capsinfo = capsinfo
> > diff --git a/virtinst/capabilities.py b/virtinst/capabilities.py
> > index bed8596..a26b4a2 100644
> > --- a/virtinst/capabilities.py
> > +++ b/virtinst/capabilities.py
> > @@ -268,6 +268,36 @@ class _CapsGuest(XMLBuilder):
> > # Fallback, just return last item in list
> > return domains[-1]
> >
> > + def has_install_options(self):
> > + """
> > + Return True if there are any install options available
> > + """
> > + if len(self.domains) > 0:
> > + return True
> > +
> > + return False
> > +
> > + def is_kvm_available(self):
> > + """
> > + Return True if kvm guests can be installed
> > + """
> > + if self.os_type != "hvm":
> > + return False
> > +
> > + for d in self.domains:
> > + if d.hypervisor_type == "kvm":
> > + return True
> > +
> > + return False
> > +
> > + def supports_pae(self):
> > + """
> > + Return True if capabilities report support for PAE
> > + """
> > + if self.features.pae:
> > + return True
> > + return False
> > +
> >
> > ############################
> > # Main capabilities object #
> > @@ -280,23 +310,17 @@ class _CapsInfo(object):
> > """
> > def __init__(self, conn, guest, domain, requested_machine):
> > self.conn = conn
> > - self._guest = guest
> > - self._domain = domain
> > + self.guest = guest
> > + self.domain = domain
> > self._requested_machine = requested_machine
> >
> > - self.hypervisor_type = self._domain.hypervisor_type
> > - self.os_type = self._guest.os_type
> > - self.arch = self._guest.arch
> > - self.loader = self._guest.loader
> > + self.hypervisor_type = self.domain.hypervisor_type
> > + self.os_type = self.guest.os_type
> > + self.arch = self.guest.arch
> > + self.loader = self.guest.loader
> >
> > - self.emulator = self._domain.emulator
> > - self.machines = self._domain.machines[:]
> > -
> > - def get_caps_objects(self):
> > - """
> > - Return the raw backing caps objects
> > - """
> > - return self._guest, self._domain
> > + self.emulator = self.domain.emulator
> > + self.machines = self.domain.machines[:]
> >
> > def get_recommended_machine(self):
> > """
> > @@ -360,39 +384,6 @@ class Capabilities(XMLBuilder):
> > # Public API #
> > ##############
> >
> > - def has_install_options(self):
> > - """
> > - Return True if there are any install options available
> > - """
> > - for g in self.guests:
> > - if len(g.domains) > 0:
> > - return True
> > -
> > - return False
> > -
> > - def is_kvm_available(self):
> > - """
> > - Return True if kvm guests can be installed
> > - """
> > - for g in self.guests:
> > - if g.os_type != "hvm":
> > - continue
> > -
> > - for d in g.domains:
> > - if d.hypervisor_type == "kvm":
> > - return True
> > -
> > - return False
> > -
> > - def supports_pae(self):
> > - """
> > - Return True if capabilities report support for PAE
> > - """
> > - for g in self.guests:
> > - if g.features.pae:
> > - return True
> > - return False
> > -
> > def get_cpu_values(self, arch):
> > if not arch:
> > return []
> > @@ -498,6 +489,8 @@ class Capabilities(XMLBuilder):
> >
> > gobj.os.machine = capsinfo.get_recommended_machine()
> >
> > + gobj.capsinfo = capsinfo
> > +
> > return gobj
> >
> > def lookup_virtinst_guest(self, *args, **kwargs):
> > diff --git a/virtinst/guest.py b/virtinst/guest.py
> > index bf4b70b..7ff7bd5 100644
> > --- a/virtinst/guest.py
> > +++ b/virtinst/guest.py
> > @@ -844,7 +844,7 @@ class Guest(XMLBuilder):
> > if self.features.apic == "default":
> > self.features.apic = self._os_object.supports_apic(default)
> > if self.features.pae == "default":
> > - self.features.pae = self.conn.caps.supports_pae()
> > + self.features.pae = self.capsinfo.guest.supports_pae()
> >
> > if (self.features.vmport == "default" and
> > self.has_spice() and
> >
>
> Add self.capsinfo = None to Guest.__init__ with a comment that it's set via
> Capabilities.build_virtinst_guest
Good point, that's much better and safer.
Thanks for review,
Pavel
>
> Thanks,
> Cole
More information about the virt-tools-list
mailing list