[libvirt PATCH 4/4] gitlab-ci: Introduce a new test 'integration' pipeline stage

[Andrea Bolognani]

On Mon, Jan 31, 2022 at 07:01:01PM +0100, Erik Skultety wrote:
> +++ b/.gitlab-ci-integration.yml
> @@ -0,0 +1,116 @@
> +.tests:
> +  stage: integration
> +  before_script:
> +    - mkdir "$SCRATCH_DIR"
> +    - sudo dnf install -y libvirt-rpms/* libvirt-perl-rpms/*
> +    - sudo pip3 install --prefix=/usr avocado-framework
> +    - source /etc/os-release  # in order to query the vendor-provided variables
> +    - if test "$ID" == "centos" && test "$VERSION_ID" -lt 9 ||
> +         test "$ID" == "fedora" && test "$VERSION_ID" -lt 35;

Using == with test is a bashism, please stick to the portable version
even though it's very likely that the script will ultimately run
under bash.

> +    - for daemon in $DAEMONS;
> +      do
> +        sudo sed -Ei "s/^(#)?(log_outputs=).*/\2'1:file:\/var\/log\/libvirt\/${daemon}.log'/" /etc/libvirt/${daemon}.conf;
> +        sudo sed -Ei "s/^(#)?(log_filters=).*/\2'4:*object* 4:*json* 4:*event* 4:*rpc* 4:daemon.remote 4:util.threadjob 4:*access* 1:*'/" /etc/libvirt/${daemon}.conf;
> +        sudo systemctl --quiet stop ${daemon}.service;
> +        sudo systemctl restart ${daemon}.socket;
> +      done

I suggest changing this to something along the lines of

  - for daemon in $DAEMONS;
    do
      log_outputs="file:/var/log/libvirt/${daemon}.log"
      log_filters="3:remote 4:event 3:util.json 3:util.object
3:util.dbus 3:util.netlink 3:node_device 3:rpc 3:access 1:*"
      sed -Ei -e "s;^#*\\s*log_outputs\\s*=.*$;log_outputs=\"$log_outputs\";g" \
              -e "s;^#*\\s*log_filters\\s*=.*$;log_filters=\"$log_filters\";g" \
              "src/remote/${daemon}.conf.in"
      # ...
    done

Advantages:

  * saves one call to sed;
  * doesn't need the awkward quoting for slashes in paths;
  * produces key="value" which is consistent with the existing
    contents of the configuration files (even though the parser will
    accept single quotes too);
  * handles slight, semantically-irrelevant changes to the contents
    of the configuration files such as whitespace being added;
  * doesn't unnecessarily use captures;
  * avoids excessively long lines.

Note that I have adjusted the value of log_filters to match what is
recommended in

  https://libvirt.org/kbase/debuglogs.html#less-verbose-logging-for-qemu-vms

but maybe there's a reason you had picked a different set of filters.

Also note that I haven't actually tested that the above works
correctly O:-)

> +    - sudo virsh net-start default &>/dev/null || true;
> +
> +  script:

Unnecessary empty line here.

> +  after_script:
> +    - if test -e "$SCRATCH_DIR"/avocado;
> +      then
> +        sudo mv "$SCRATCH_DIR"/avocado/latest/test-results logs/avocado;
> +      fi
> +    - sudo mv /var/log/libvirt logs/libvirt
> +    - sudo chown -R $(whoami):$(whoami) logs

Same as in the previous patch, I'd like for names of directories to
end with a slash.

> +libvirt-perl-bindings:
> +  stage: bindings
> +  trigger:
> +    project: eskultety/libvirt-perl
> +    branch: multi-project-ci
> +    strategy: depend
> +
> +
> +centos-stream-8-tests:
> +  extends: .tests
> +  needs:
> +    - libvirt-perl-bindings
> +    - pipeline: $PARENT_PIPELINE_ID
> +      job: x86_64-centos-stream-8
> +    - project: eskultety/libvirt-perl
> +      job: x86_64-centos-stream-8
> +      ref: multi-project-ci
> +      artifacts: true

IIUC from the documentation and from reading around, using
strategy:depend will cause the local job to reflect the status of the
triggered pipeline. So far so good.

What I am unclear about is, is there any guarantee that using
artifact:true with a job from an external project's pipeline will
expose the artifacts from the job that was executed as part of the
specific pipeline that we've just triggered ourselves as opposed to
some other pipeline that might have already been completed in the
past of might have completed in the meantime?

Taking a step back, why exactly are we triggering a rebuild of
libvirt-perl in the first place? Once we change that project's
pipeline so that RPMs are published as artifacts, can't we just grab
the ones from the latest successful pipeline? Maybe you've already
explained why you did things this way and I just missed it!

> +  variables:
> +    DISTRO: centos-stream-8

This variable doesn't seem to be used anywhere, so I assume it's a
leftover from development. Maybe you tried to implement the .test
template so that using it didn't require as much repetition and
unfortunately it didn't work out?

> +  tags:
> +    - centos-stream-vm

IIUC this is used both by the GitLab scheduler to pick suitable nodes
on which to execute the job (our own hardware in this case) and also
by the runner to decide which template to use for the VM.

So shouldn't this be more specific? I would expect something like

  tags:
    - centos-stream-8-vm

or

  tags:
    - centos-stream-8
    - vm

> +++ b/.gitlab-ci.yml
> @@ -4,6 +4,7 @@ variables:
>  stages:
>    - containers
>    - builds
> +  - test
>    - sanity_checks

Unlike Dan, I actually think it makes sense to have these as a
separate stage. I'd call it integration_tests and list it last
though. But admittedly this is entirely in bikeshedding territory and
I'm fine with the current solution or the one Dan proposed too :)

-- 
Andrea Bolognani / Red Hat / Virtualization