[libvirt PATCH v2 0/8] Extract the integration job commands to a shell script
Using shell scripts rather than inlining shell commands to YAML feels more natural, more readable, and will keep all different variations of execution consistent. Essentially the only disadvantage is that we won't see each command listed one-by-one in gitlab's log output (unless we set -x that is), but given that shell would complain if something was wrong with the script, it's fairly easy to identify the problem. Here's a test pipeline after the change: https://gitlab.com/eskultety/libvirt/-/pipelines/759277200 Since v1: - 3/7 - reworded commit message as requested - 4/7 was dropped - point the SCRATCH_DIR to /var/tmp instead of /tmp to not be limited by the size of ramdisk mounted in there Erik Skultety (8): syntax-check: Drop the shell's 'check for minus' rule ci: Move the SCRATCH_DIR from /tmp ci: integration: Extract several hidden job definitions to a script ci: integration: Drop the 'install-deps' hidden job and reference ci: integration-template: Drop the '-lt Fedora 35' check ci: integration.sh: Add/Rewrite/Reformat commentaries ci: integration.sh: Replace 'test' with '[' operator ci: integration.sh: Define the SCRATCH_DIR variable for local execution build-aux/syntax-check.mk | 9 -------- ci/integration-template.yml | 44 +++-------------------------------- ci/integration.sh | 46 +++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 50 deletions(-) create mode 100644 ci/integration.sh -- 2.39.1
Apparently we've only had it because the -[ao] options weren't portable at the time, but according to https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html both are defined in POSIX.1-2017 revision which is old enough for all our supported platforms to have adopted it already. Therefore, drop the rule. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- build-aux/syntax-check.mk | 9 --------- 1 file changed, 9 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 96d322ee04..7b07fd4712 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1183,15 +1183,6 @@ sc_prohibit_double_semicolon: halt="Double semicolon detected" \ $(_sc_search_regexp) -_ptm1 = use "test C1 && test C2", not "test C1 -''a C2" -_ptm2 = use "test C1 || test C2", not "test C1 -''o C2" -# Using test's -a and -o operators is not portable. -# We prefer test over [, since the latter is spelled [[ in configure.ac. -sc_prohibit_test_minus_ao: - @prohibit='(\<test| \[+) .+ -[ao] ' \ - halt='$(_ptm1); $(_ptm2)' \ - $(_sc_search_regexp) - # Avoid a test bashism. sc_prohibit_test_double_equal: @prohibit='(\<test| \[+) .+ == ' \ -- 2.39.1
In case /tmp is mounted as an in-memory file system, we're fairly storage limited. This might pose a problem in VM scenarios if we build multiple projects in there or download some data needed for testing. Therefore, move the scratch to /home. Also, take the opportunity and rename scratch to 'vroot' as primarily we're building other projects in this directory, the fact that we also store other data is only a secondary use case for it. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration-template.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/integration-template.yml b/ci/integration-template.yml index 9141d5cedf..15c36d7a29 100644 --- a/ci/integration-template.yml +++ b/ci/integration-template.yml @@ -91,7 +91,7 @@ - test "$CI_JOB_STATUS" = "success" && exit 0; - *collect-logs variables: - SCRATCH_DIR: "/tmp/scratch" + SCRATCH_DIR: "/var/tmp/scratch" artifacts: name: logs expire_in: 1 day -- 2.39.1
This will allow us to re-use the script in other scenarios like local executions where up until now one had to copy-paste the recipe line-by-line. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration-template.yml | 36 +----------------------------------- ci/integration.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 35 deletions(-) create mode 100644 ci/integration.sh diff --git a/ci/integration-template.yml b/ci/integration-template.yml index 15c36d7a29..802cba9420 100644 --- a/ci/integration-template.yml +++ b/ci/integration-template.yml @@ -27,34 +27,6 @@ .install-deps: &install-deps - sudo dnf install -y libvirt-rpms/* libvirt-perl-rpms/* libvirt-python-rpms/* - # Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now - - sudo pip3 install --prefix=/usr avocado-framework==98.0 - - -.enable-core-dumps: &enable-core-dumps - - sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally - - sudo systemctl daemon-reexec # need to reexec systemd after changing config - - -.enable-libvirt-debugging: &enable-libvirt-debugging - - 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; - then - DAEMONS="libvirtd virtlockd virtlogd"; - else - DAEMONS="virtinterfaced virtlockd virtlogd virtnetworkd virtnodedevd virtnwfilterd virtproxyd virtqemud virtsecretd virtstoraged"; - fi - - for daemon in $DAEMONS; - do - LOG_OUTPUTS="1: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:*"; - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" &>/dev/null; - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" &>/dev/null; - sudo systemctl --quiet stop ${daemon}.service; - sudo systemctl restart ${daemon}.socket; - done - .collect-logs: &collect-logs - set +e @@ -79,14 +51,8 @@ before_script: - mkdir "$SCRATCH_DIR" - *install-deps - - *enable-core-dumps - - *enable-libvirt-debugging - - sudo virsh net-start default &>/dev/null || true; script: - - cd "$SCRATCH_DIR" - - git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git - - cd libvirt-tck - - sudo avocado --config avocado.config run --job-results-dir "$SCRATCH_DIR"/avocado + - chmod +x ci/integration.sh && ci/integration.sh after_script: - test "$CI_JOB_STATUS" = "success" && exit 0; - *collect-logs diff --git a/ci/integration.sh b/ci/integration.sh new file mode 100644 index 0000000000..21795518c3 --- /dev/null +++ b/ci/integration.sh @@ -0,0 +1,32 @@ +#!/bin/sh + +# Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now +sudo pip3 install --prefix=/usr avocado-framework==98.0 + +sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally +sudo systemctl daemon-reexec # need to reexec systemd after changing config + +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 +then + DAEMONS="libvirtd virtlockd virtlogd" +else + DAEMONS="virtinterfaced virtlockd virtlogd virtnetworkd virtnodedevd virtnwfilterd virtproxyd virtqemud virtsecretd virtstoraged" +fi +for daemon in $DAEMONS +do + LOG_OUTPUTS="1: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:*" + sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" &>/dev/null + sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" &>/dev/null + sudo systemctl --quiet stop ${daemon}.service + sudo systemctl restart ${daemon}.socket +done + +sudo virsh net-start default &>/dev/null || true + +cd "$SCRATCH_DIR" +git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git +cd libvirt-tck +sudo avocado --config avocado.config run --job-results-dir "$SCRATCH_DIR"/avocado -- 2.39.1
Since the section now only consists of a single command, we can happily move the command to the main integration template job body. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration-template.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ci/integration-template.yml b/ci/integration-template.yml index 802cba9420..1425510d4f 100644 --- a/ci/integration-template.yml +++ b/ci/integration-template.yml @@ -24,10 +24,6 @@ - sudo make install -.install-deps: &install-deps - - sudo dnf install -y libvirt-rpms/* libvirt-perl-rpms/* libvirt-python-rpms/* - - .collect-logs: &collect-logs - set +e - shopt -s nullglob @@ -50,7 +46,7 @@ stage: integration_tests before_script: - mkdir "$SCRATCH_DIR" - - *install-deps + - sudo dnf install -y libvirt-rpms/* libvirt-perl-rpms/* libvirt-python-rpms/* script: - chmod +x ci/integration.sh && ci/integration.sh after_script: -- 2.39.1
We no longer support Fedora 35 or older. While at it, we can also tailor the rest of the check to CentOS Stream 8 only since all other supported Red Hat like platforms have moved onto modular daemons setup already. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ci/integration.sh b/ci/integration.sh index 21795518c3..160aecf00d 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -7,8 +7,7 @@ sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explic sudo systemctl daemon-reexec # need to reexec systemd after changing config 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 +if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" else -- 2.39.1
Because of the nature of writing inline shell commands to YAML, most of the commentaries where inlined with the command not to hinder YAML readability any further. Since we moved the logic to a standalone script, we can now do whatever formatting & readability adjustments we want. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration.sh | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/ci/integration.sh b/ci/integration.sh index 160aecf00d..cbf2b0f1b4 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -3,10 +3,15 @@ # Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now sudo pip3 install --prefix=/usr avocado-framework==98.0 -sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally -sudo systemctl daemon-reexec # need to reexec systemd after changing config +# Explicitly allow storing cores globally +sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" + +# Need to reexec systemd after changing config +sudo systemctl daemon-reexec + +# Source the os-release file to query the vendor-provided variables +source /etc/os-release -source /etc/os-release # in order to query the vendor-provided variables if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" @@ -23,6 +28,7 @@ do sudo systemctl restart ${daemon}.socket done +# Make sure the default network is started on all platforms sudo virsh net-start default &>/dev/null || true cd "$SCRATCH_DIR" -- 2.39.1
Since we now have a standalone script instead of inlined shell commands in a YAML, we can adopt the common syntax for if-then-else clauses which involve the '[' test command alias rather than keep using 'test' explicitly. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/integration.sh b/ci/integration.sh index cbf2b0f1b4..7bf3623809 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -12,7 +12,7 @@ sudo systemctl daemon-reexec # Source the os-release file to query the vendor-provided variables source /etc/os-release -if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 +if [ "$ID" = "centos" -a "$VERSION_ID" -eq 8 ] then DAEMONS="libvirtd virtlockd virtlogd" else -- 2.39.1
Running outside of GitLab will likely not have the variable set and hence the execution would fail. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ci/integration.sh b/ci/integration.sh index 7bf3623809..e1f461a441 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -31,6 +31,15 @@ done # Make sure the default network is started on all platforms sudo virsh net-start default &>/dev/null || true +# If we're running outside of GitLab, this variable will likely not exist, so +# we need to define it and create the scratch directory +if [ -z "$SCRATCH_DIR" ] +then + SCRATCH_DIR="/var/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null +fi + +rm -rf "$SCRATCH_DIR/*" cd "$SCRATCH_DIR" git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git cd libvirt-tck -- 2.39.1
On Fri, Jan 27, 2023 at 10:26:48AM +0100, Erik Skultety wrote:
Using shell scripts rather than inlining shell commands to YAML feels more natural, more readable, and will keep all different variations of execution consistent. Essentially the only disadvantage is that we won't see each command listed one-by-one in gitlab's log output (unless we set -x that is), but given that shell would complain if something was wrong with the script, it's fairly easy to identify the problem.
Here's a test pipeline after the change: https://gitlab.com/eskultety/libvirt/-/pipelines/759277200
Since v1: - 3/7 - reworded commit message as requested - 4/7 was dropped - point the SCRATCH_DIR to /var/tmp instead of /tmp to not be limited by the size of ramdisk mounted in there
Ping.
participants (1)
-
Erik Skultety