[v4,2/4] Jobs based on custom runners: build environment docs and playbook

To run basic jobs on custom runners, the environment needs to be
properly set up.  The most common requirement is having the right
packages installed.

The playbook introduced here covers a number of different Linux
distributions and FreeBSD, and are intended to provide a reproducible
environment.

Signed-off-by: Cleber Rosa <crosa@redhat.com>
---
 docs/devel/ci.rst                      |  32 ++++
 scripts/ci/setup/build-environment.yml | 233 +++++++++++++++++++++++++
 scripts/ci/setup/inventory             |   2 +
 3 files changed, 267 insertions(+)
 create mode 100644 scripts/ci/setup/build-environment.yml
 create mode 100644 scripts/ci/setup/inventory

...

> diff --git a/scripts/ci/setup/inventory b/scripts/ci/setup/inventory
> new file mode 100644
> index 0000000000..8bb7ba6b33
> --- /dev/null
> +++ b/scripts/ci/setup/inventory
> @@ -0,0 +1,2 @@
> +[local]

Nit pick, is a group for localhost actually needed?

Regards,
Erik

On Mon, Oct 19, 2020 at 12:27:41PM +0200, Erik Skultety wrote:
> ...
> 
> > diff --git a/scripts/ci/setup/inventory b/scripts/ci/setup/inventory
> > new file mode 100644
> > index 0000000000..8bb7ba6b33
> > --- /dev/null
> > +++ b/scripts/ci/setup/inventory
> > @@ -0,0 +1,2 @@
> > +[local]
> 
> Nit pick, is a group for localhost actually needed?
>

You're right, it's not needed... I just thought it gave the
"localhost" entry some "shelter" and "context". :)

And, I think a mostly "ini-like" file without a section triggers an OCD
reaction in me.  I can remove it if it does something similar to you! :)

Thanks!
- Cleber.

> Regards,
> Erik

On Mon, Oct 19, 2020 at 04:25:31PM -0400, Cleber Rosa wrote:
> On Mon, Oct 19, 2020 at 12:27:41PM +0200, Erik Skultety wrote:
> > ...
> > 
> > > diff --git a/scripts/ci/setup/inventory b/scripts/ci/setup/inventory
> > > new file mode 100644
> > > index 0000000000..8bb7ba6b33
> > > --- /dev/null
> > > +++ b/scripts/ci/setup/inventory
> > > @@ -0,0 +1,2 @@
> > > +[local]
> > 
> > Nit pick, is a group for localhost actually needed?
> >
> 
> You're right, it's not needed... I just thought it gave the
> "localhost" entry some "shelter" and "context". :)
> 
> And, I think a mostly "ini-like" file without a section triggers an OCD
> reaction in me.  I can remove it if it does something similar to you! :)
> 
> Thanks!
> - Cleber.

I understand, but even though it may be an ini-like config, "sections" have a
very distinct meaning in Ansible and that is grouping, so unless there are
hosts to group, I'd keep the inventory in a form of a simple list of individual
hosts.

Regards,
Erik

On Sun, 2020-10-18 at 21:50 -0400, Cleber Rosa wrote:
> +++ b/scripts/ci/setup/build-environment.yml
> @@ -0,0 +1,233 @@
> +---
> +- name: Installation of basic packages to build QEMU
> +  hosts: all
> +  tasks:

My Ansible-fu is a bit rusty at the moment, so please double-check my
claims and apologies in advance for the ones that I will get wrong O:-)

> +    - name: Install basic packages to build QEMU on Ubuntu 18.04/20.04
> +      apt:
> +        update_cache: yes
> +        # Originally from tests/docker/dockerfiles/ubuntu1804.docker
> +        pkg:
> +          - ccache
> +          - clang

Instead of using the 'apt' module here, and the equivalent module
further down, you could just do

  package:
    name:
      - pkg1
      - pkg2
        ...
    state: present

every single time and let Ansible take care of the differences for
you.

> +      when: "ansible_facts['distribution'] == 'Ubuntu'"

Quoting the entire condition is not necessary, you can just have

  when: ansible_facts['distribution'] == 'Ubuntu'

or, my preference,

  when:
    - ansible_facts['distribution'] == 'Ubuntu'

which results in a nicer diff when you add/remove conditions.

> +    - name: Install packages to build QEMU on Ubuntu 18.04/20.04 on non-s390x
> +      apt:
> +        update_cache: yes
> +        pkg:
> +         - libspice-server-dev
> +         - libxen-dev

Indentation of list items is inconsistent here.

> +    - name: Install basic packages to build QEMU on FreeBSD 12.x
> +      pkgng:
> +        # Originally from packages on .cirrus.yml under the freebsd_12_task
> +        name: bash,curl,cyrus-sasl,git,glib,gmake,gnutls,gsed,nettle,ninja,perl5,pixman,pkgconf,png,usbredir

See above for 'pkgng' vs 'package', but at the very least this should
be

  pkgng:
    name:
      - bash
      - curl
        ...

or each time the list is touched the resulting diff is going to be
unmanageable.

> +    - name: Enable PowerTools repo on CentOS 8
> +      ini_file:
> +        path: /etc/yum.repos.d/CentOS-PowerTools.repo
> +        section: PowerTools
> +        option: enabled
> +        value: "1"
> +      when:
> +        - "ansible_facts['distribution'] == 'CentOS'"
> +        - "ansible_facts['distribution_major_version'] == '8'"

Another option would be to use

  command: 'dnf config-manager --set-enabled Stream-PowerTools -y'
  args:
    warn: no

but I have to admit the way you're doing it is very clever ;)

On 19/10/2020 03.50, Cleber Rosa wrote:
> To run basic jobs on custom runners, the environment needs to be
> properly set up.  The most common requirement is having the right
> packages installed.
> 
> The playbook introduced here covers a number of different Linux
> distributions and FreeBSD, and are intended to provide a reproducible
> environment.
> 
> Signed-off-by: Cleber Rosa <crosa@redhat.com>
> ---
[...]
> +    - name: Enable PowerTools repo on CentOS 8
> +      ini_file:
> +        path: /etc/yum.repos.d/CentOS-PowerTools.repo
> +        section: PowerTools
> +        option: enabled
> +        value: "1"
> +      when:
> +        - "ansible_facts['distribution'] == 'CentOS'"
> +        - "ansible_facts['distribution_major_version'] == '8'"
> +
> +    - name: Install basic packages to build QEMU on CentOS 8
> +      dnf:
> +        # Originally from tests/docker/dockerfiles/centos8.docker
> +        name:
> +          - SDL-devel

We do not support SDL1 in QEMU anymore, so this should be SDL2-devel now.
Yes, we've also got it wrong in the docker files ... I'll send a patch to
fix it there.

 Thomas

On Tue, Oct 20, 2020 at 08:18:54AM +0200, Erik Skultety wrote:
> On Mon, Oct 19, 2020 at 04:25:31PM -0400, Cleber Rosa wrote:
> > On Mon, Oct 19, 2020 at 12:27:41PM +0200, Erik Skultety wrote:
> > > ...
> > > 
> > > > diff --git a/scripts/ci/setup/inventory b/scripts/ci/setup/inventory
> > > > new file mode 100644
> > > > index 0000000000..8bb7ba6b33
> > > > --- /dev/null
> > > > +++ b/scripts/ci/setup/inventory
> > > > @@ -0,0 +1,2 @@
> > > > +[local]
> > > 
> > > Nit pick, is a group for localhost actually needed?
> > >
> > 
> > You're right, it's not needed... I just thought it gave the
> > "localhost" entry some "shelter" and "context". :)
> > 
> > And, I think a mostly "ini-like" file without a section triggers an OCD
> > reaction in me.  I can remove it if it does something similar to you! :)
> > 
> > Thanks!
> > - Cleber.
> 
> I understand, but even though it may be an ini-like config, "sections" have a
> very distinct meaning in Ansible and that is grouping, so unless there are
> hosts to group, I'd keep the inventory in a form of a simple list of individual
> hosts.
>

Fair enough.  Changing it for v5.

Thanks!
- Cleber.

> Regards,
> Erik

On Tue, Oct 20, 2020 at 07:52:43PM +0200, Andrea Bolognani wrote:
> On Sun, 2020-10-18 at 21:50 -0400, Cleber Rosa wrote:
> > +++ b/scripts/ci/setup/build-environment.yml
> > @@ -0,0 +1,233 @@
> > +---
> > +- name: Installation of basic packages to build QEMU
> > +  hosts: all
> > +  tasks:
> 
> My Ansible-fu is a bit rusty at the moment, so please double-check my
> claims and apologies in advance for the ones that I will get wrong O:-)
> 
> > +    - name: Install basic packages to build QEMU on Ubuntu 18.04/20.04
> > +      apt:
> > +        update_cache: yes
> > +        # Originally from tests/docker/dockerfiles/ubuntu1804.docker
> > +        pkg:
> > +          - ccache
> > +          - clang
> 
> Instead of using the 'apt' module here, and the equivalent module
> further down, you could just do
> 
>   package:
>     name:
>       - pkg1
>       - pkg2
>         ...
>     state: present
> 
> every single time and let Ansible take care of the differences for
> you.
>

I'm almost sure that this was a conscious decision.  I remeber it had
to do with not being able to set `update_cache`, and failures on
recently installed systems and containers that did not update the APT
cache.  There may be something else, but I'll have to give it another
round of testing.

FIY, under the hood, package is not really a module, but an action
plugin that forwards these very limited options to the set or detected
package manager, so it brings uniformity in the playbook, but limits
the control too.  IMO, it's very low impact to leave it AS IS.

> > +      when: "ansible_facts['distribution'] == 'Ubuntu'"
> 
> Quoting the entire condition is not necessary, you can just have
> 
>   when: ansible_facts['distribution'] == 'Ubuntu'
> 
> or, my preference,
> 
>   when:
>     - ansible_facts['distribution'] == 'Ubuntu'
>

Yep, I've used the explicit lists when there was more than one
condition, but having a standard style is better indeed.

> which results in a nicer diff when you add/remove conditions.
> 
> > +    - name: Install packages to build QEMU on Ubuntu 18.04/20.04 on non-s390x
> > +      apt:
> > +        update_cache: yes
> > +        pkg:
> > +         - libspice-server-dev
> > +         - libxen-dev
> 
> Indentation of list items is inconsistent here.
>

True.  Fixed, thanks!

> > +    - name: Install basic packages to build QEMU on FreeBSD 12.x
> > +      pkgng:
> > +        # Originally from packages on .cirrus.yml under the freebsd_12_task
> > +        name: bash,curl,cyrus-sasl,git,glib,gmake,gnutls,gsed,nettle,ninja,perl5,pixman,pkgconf,png,usbredir
>
> See above for 'pkgng' vs 'package', but at the very least this should
> be
> 
>   pkgng:
>     name:
>       - bash
>       - curl
>         ...
> 
> or each time the list is touched the resulting diff is going to be
> unmanageable.
>

The documentation suggests a comma separated list of package names:

   https://docs.ansible.com/ansible/2.8/modules/pkgng_module.html#pkgng-module

And the reason is that this module is not as smart as others, and will
run one separate command for each individual package name value:

   https://github.com/ansible/ansible/blob/v2.10.3/test/support/integration/plugins/modules/pkgng.py#L214

It's a tradeoff indeed, but I think we're aligned with the docs.

> > +    - name: Enable PowerTools repo on CentOS 8
> > +      ini_file:
> > +        path: /etc/yum.repos.d/CentOS-PowerTools.repo
> > +        section: PowerTools
> > +        option: enabled
> > +        value: "1"
> > +      when:
> > +        - "ansible_facts['distribution'] == 'CentOS'"
> > +        - "ansible_facts['distribution_major_version'] == '8'"
> 
> Another option would be to use
> 
>   command: 'dnf config-manager --set-enabled Stream-PowerTools -y'
>   args:
>     warn: no
> 
> but I have to admit the way you're doing it is very clever ;)
>

Yeah, that would require another package to be installed, and then the
command executed... So I think this is cheaper and eaiser indeed.

> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

Thanks for the review, I'll report on the additional points as soon as
I test them.  If appropriate, I'll put notes on the v5.

- Cleber.

On Wed, Oct 21, 2020 at 09:00:27AM +0200, Thomas Huth wrote:
> On 19/10/2020 03.50, Cleber Rosa wrote:
> > To run basic jobs on custom runners, the environment needs to be
> > properly set up.  The most common requirement is having the right
> > packages installed.
> > 
> > The playbook introduced here covers a number of different Linux
> > distributions and FreeBSD, and are intended to provide a reproducible
> > environment.
> > 
> > Signed-off-by: Cleber Rosa <crosa@redhat.com>
> > ---
> [...]
> > +    - name: Enable PowerTools repo on CentOS 8
> > +      ini_file:
> > +        path: /etc/yum.repos.d/CentOS-PowerTools.repo
> > +        section: PowerTools
> > +        option: enabled
> > +        value: "1"
> > +      when:
> > +        - "ansible_facts['distribution'] == 'CentOS'"
> > +        - "ansible_facts['distribution_major_version'] == '8'"
> > +
> > +    - name: Install basic packages to build QEMU on CentOS 8
> > +      dnf:
> > +        # Originally from tests/docker/dockerfiles/centos8.docker
> > +        name:
> > +          - SDL-devel
> 
> We do not support SDL1 in QEMU anymore, so this should be SDL2-devel now.
> Yes, we've also got it wrong in the docker files ... I'll send a patch to
> fix it there.
> 
>  Thomas

Nice catch.  I see the dockerfile changes are already merged, so
updating them here.

- Cleber.

On Mon, 2020-11-09 at 11:37 -0500, Cleber Rosa wrote:
> On Tue, Oct 20, 2020 at 07:52:43PM +0200, Andrea Bolognani wrote:
> > On Sun, 2020-10-18 at 21:50 -0400, Cleber Rosa wrote:
> > > +    - name: Install basic packages to build QEMU on FreeBSD 12.x
> > > +      pkgng:
> > > +        # Originally from packages on .cirrus.yml under the freebsd_12_task
> > > +        name: bash,curl,cyrus-sasl,git,glib,gmake,gnutls,gsed,nettle,ninja,perl5,pixman,pkgconf,png,usbredir
> > 
> > See above for 'pkgng' vs 'package', but at the very least this should
> > be
> > 
> >   pkgng:
> >     name:
> >       - bash
> >       - curl
> >         ...
> > 
> > or each time the list is touched the resulting diff is going to be
> > unmanageable.
> 
> The documentation suggests a comma separated list of package names:
> 
>    https://docs.ansible.com/ansible/2.8/modules/pkgng_module.html#pkgng-module
> 
> And the reason is that this module is not as smart as others, and will
> run one separate command for each individual package name value:
> 
>    https://github.com/ansible/ansible/blob/v2.10.3/test/support/integration/plugins/modules/pkgng.py#L214
> 
> It's a tradeoff indeed, but I think we're aligned with the docs.

I would probably take the performance hit over having to deal with an
unmaintainable blob. As a rule of thumb, reviewer time is more
valuable than machine time.

If the performance hit turns out to be unacceptably big, you could at
least do something along the lines of

  - set_fact:
      freebsd_packages:
        - bash
        - curl
          ...
  
  - pkgng:
      name: "{ freebsd_packages | join(',') }"

to keep things reviewable.