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@xxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
