[libvirt-ci PATCH 11/12] config: Move the virt-install settings from install.yml to the config

[Erik Skultety]

On Thu, May 07, 2020 at 06:56:44PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-05-06 at 14:06 +0200, Erik Skultety wrote:
> > +++ b/guests/config.yaml
> > @@ -15,6 +15,23 @@ install:
> >    # instead. (Mandatory)
> >    #root_password:
> >  
> > +  # Settings mapping to the virt-install options - see virt-install(1).
> > +  # It is strongly recommended that you keep the following at their default
> > +  # values to produce machines which conform to the upstream libvirt standard,
> > +  # unless you have a reason to do otherwise.
> > +  #
> > +  # Sizes are expressed in GiB.
> > +  #
> > +  virt_type: kvm
> > +  arch: x86_64
> > +  machine: pc
> > +  cpu_model: host-passthrough
> > +  vcpus: 2
> > +  memory_size: 2
> > +  disk_size: 15
> > +  storage_pool: default
> > +  network: default
> 
> Now is a great time to delete group_vars/all/install.yml, since
> you've just made it obsolete and we don't want to have the same
> information stored in two places.
> 
> Note that there's one use of install_* variables that would be broken
> by doing so in playbooks/update/templates/bashrc.j2, but I've already
> posted a patch[1] that takes care of that, so as long as that goes in
> before your series we don't have to worry about it :)

Would it? I knew about the MAKEFLAGS thing, but I expected that to simply work
once I pass the extra variables, nevertheless the patch you posted makes
complete sense and is even in sync in what we do in gitlab CI.

> 
> >      def _action_install(self, args):
> >          base = Util.get_base()
> > +        config = self._config
> >  
> >          for host in self._inventory.expand_pattern(args.hosts):
> >              facts = self._inventory.get_facts(host)
> >  
> > -            # Both memory size and disk size are stored as GiB in the
> > -            # inventory, but virt-install expects the disk size in GiB
> > -            # and the memory size in *MiB*, so perform conversion here
> > -            memory_arg = str(int(facts["install_memory_size"]) * 1024)
> > -
> > -            vcpus_arg = str(facts["install_vcpus"])
> 
> Please leave these type conversion bits here where they can easily
> be spotted, instead of hiding them in the jungle of creating the
> virt-install command line.

Okay.

> 
> 
> With the changes mentioned above,
> 
>   Reviewed-by: Andrea Bolognani <abologna at redhat.com>
> 
> 
> [1] https://www.redhat.com/archives/libvir-list/2020-May/msg00330.html
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

-- 
Erik Skultety