[virt-tools-list] [virt-manager PATCH 1/2] guest: Default to libosinfo recommended resources

[Pavel Hrdina]

On Fri, Dec 07, 2018 at 03:39:53PM +0100, Pavel Hrdina wrote:
> On Wed, Dec 05, 2018 at 09:15:18AM +0100, Fabiano Fidêncio wrote:
> > Let's create a new method that defaults to libosinfo's recommended
> > resources (when they're available) for memory and vcpus.
> > 
> > It'll help us to avoid erroring out whenever virt-install is called
> > without specifying the memory amount, as the recommended amount of
> > memory would come from libosinfo.
> > 
> > Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> > ---
> >  virtinst/guest.py | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virtinst/guest.py b/virtinst/guest.py
> > index eeb40cb6..ced8f259 100644
> > --- a/virtinst/guest.py
> > +++ b/virtinst/guest.py
> > @@ -475,11 +475,11 @@ class Guest(XMLBuilder):
> >      def set_defaults(self, _guest):
> >          if not self.uuid:
> >              self.uuid = util.generate_uuid(self.conn)
> > -        if not self.vcpus:
> > -            self.vcpus = 1
> >  
> >          self.set_capabilities_defaults()
> >  
> > +        self._set_default_resources()
> > +
> 
> Nitpick: no need for the empty line.
> 
> >          self._set_default_machine()
> >          self._set_default_uefi()
> >  
> > @@ -510,6 +510,18 @@ class Guest(XMLBuilder):
> >      # Private xml routines #
> >      ########################
> >  
> > +    def _set_default_resources(self):
> > +        res = self.osinfo.get_recommended_resources(self)
> > +        if not res:
> > +            return

Also this check breaks a lot of test cases where we don't have any
resources available, the old code defaults to 1 vcpu every single time.

> > +
> > +        if not self.memory and res.get('ram') > 0:
> > +            self.memory = res['ram'] // 1024
> 
> The .get() method for dictionary can have default value as second
> parameter and if there is no second parameter the default is None.
> 
> So in this case it's not correct, I'm sure that if there is any 'ram'
> value stored in osinfo-db it will never be '0' and the only case when
> the 'res.get('ram') > 0' would fail is if there is no 'ram' value and in
> that case python would complain about comparing None and Int.
> 
> It should be only:
> 
>     if not self.memory and res.get('ram')
> 
> > +
> > +        if not self.vcpus:
> > +            self.vcpus = res.get('n-cpus') if res.get('n-cpus') > 0 else 1
> > +
> 
> Here we can use the default parameter for .get() so:
> 
>     self.vcpus = res.get('n-cpus', 1)

So this will not work as well :/ for some reason when I tested it I got
-1 in some test cases. Sigh, so we will need to check whether it's
defined and more then 0.

Pavel

> 
> > +
> >      def _set_default_machine(self):
> >          if self.os.machine:
> >              return
> > -- 
> > 2.19.1
> 
> This patch breaks tests, be sure to run all of these before posting
> patches:
> 
> ./setup.py build
> ./setup.py test
> ./setup.py pylint
> 
> and if you feel like it and have codespell installed
> 
> ./setup.py codespell
> 
> For the test data, you can use './setup.py test --regenerate-output'
> to update all XMLs, but review all the changes if it's correct.
> 
> Pavel
-------------- 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/20181207/d2cd2168/attachment.sig>