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, so we can drop the check. However, the above has also marked -[ao] as obsolescent stating that: "[OB] Obsolescent The functionality described may be removed in a future version of this volume of POSIX.1-2017. Strictly Conforming POSIX Applications and Strictly Conforming XSI Applications shall not use obsolescent features." It is however unlikely that the shell implementations would drop support for -[ao] despite POSIX potentially removing them. Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@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 d7cf109fbd..5718768193 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1201,15 +1201,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.41.0

All supported versions of Fedora and CentOS Stream 9 default to modular setup, it's probably better if we cosmetically adjust the CentOS Stream version check to make it explicit that monolithic daemon services ought to be started only on Stream 8. Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- ci/jobs.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 2bc6f86e45..adb798d127 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -89,7 +89,7 @@ run_integration() { 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 + if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" else -- 2.41.0

We needed v98.0 in commit c9a65eb8 due to a bug in Avocado in the past and have been installing the latest Avocado for a while since commit 91774931, yet we kept the comment by a mistake. Besides, looks like v98.0 ignores the avocado.config file in the TCK repo instructing it to run the test suite sequentially leading to test stability issues, so abandoning the v98.0 in commit 91774931 was a good thing in the end. Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- ci/jobs.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index adb798d127..04e776e9be 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -82,7 +82,6 @@ run_website_build() { } run_integration() { - # Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now sudo pip3 install --prefix=/usr avocado-framework sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Explicitly allow storing cores globally -- 2.41.0

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> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- ci/jobs.sh | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 75ea638580..37bca452fa 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -84,10 +84,14 @@ run_website_build() { run_integration() { sudo pip3 install --prefix=/usr avocado-framework - 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" - source /etc/os-release # in order to query the vendor-provided variables + # 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 if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" @@ -104,6 +108,11 @@ run_integration() { sudo systemctl restart ${daemon}.socket done + # Make sure the default network is started on all platforms + # The reason for the '|| true' here is solely that GitLab executes all + # Shell scripts with -e by default and virsh returns an error if one tries + # to start a machine/network that is already active which is both fine and + # should also be a non-fatal error sudo virsh --quiet net-start default &>/dev/null || true cd "$SCRATCH_DIR" -- 2.41.0

We've started using the run_cmd helper function to log what kind of command is being executed as well as actually executing the command. The problem however is doing I/O redirections for commands which we don't wish to see any output for whatever reason. Now, if the redirection is applied at parameter passing to run_cmd it's going to be applied to the debug print as well. Let's introduce another helper, run_cmd_quiet which takes care of the I/O redirection and executes the command completely silently. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/jobs.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ci/jobs.sh b/ci/jobs.sh index b52e45cc8c..8ae3b87228 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -18,6 +18,11 @@ run_cmd() { "$@" } +run_cmd_quiet() { + printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*" + "$@" 1>/dev/null 2>&1 +} + run_meson_setup() { run_cmd meson setup build --error -Dsystem=true $MESON_OPTS $MESON_ARGS || \ (cat "${GIT_ROOT}/build/meson-logs/meson-log.txt" && exit 1) -- 2.41.0

Unfortunately, once we go down the line of running our own scripts as part of GitLab CI jobs rather than open coding Shell in YAML, we lose the benefit of seeing each line the script executes. The downside of the default YAML however is that we have to maintain the same piece of code on 2 places in that case. Let's adopt what we use with other container jobs and prefix each shell command with 'run_cmd' or 'run_cmd_quiet' which will dump it in the logs before executing. Flow control expressions and structures are a problem though in this regard, so let's just print some important values for debugging purposes. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/jobs.sh | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/ci/jobs.sh b/ci/jobs.sh index 8ae3b87228..861e561039 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -87,16 +87,16 @@ run_website_build() { } run_integration() { - sudo pip3 install --prefix=/usr avocado-framework + run_cmd sudo pip3 install --prefix=/usr avocado-framework # Explicitly allow storing cores globally - sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" + run_cmd sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf" # Need to reexec systemd after changing config - sudo systemctl daemon-reexec + run_cmd sudo systemctl daemon-reexec # Source the os-release file to query the vendor-provided variables - . /etc/os-release + run_cmd . /etc/os-release if test "$ID" = "centos" && test "$VERSION_ID" -eq 8 then DAEMONS="libvirtd virtlockd virtlogd" @@ -107,10 +107,10 @@ run_integration() { 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'" 1>/dev/null 2>&1 - sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" 1>/dev/null 2>&1 - sudo systemctl --quiet stop ${daemon}.service - sudo systemctl restart ${daemon}.socket + run_cmd_quiet sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" + run_cmd_quiet sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" + run_cmd_quiet sudo systemctl --quiet stop ${daemon}.service + run_cmd_quiet sudo systemctl restart ${daemon}.socket done # Make sure the default network is started on all platforms @@ -118,10 +118,10 @@ run_integration() { # Shell scripts with -e by default and virsh returns an error if one tries # to start a machine/network that is already active which is both fine and # should also be a non-fatal error - sudo virsh --quiet net-start default 1>/dev/null 2>&1 || true + run_cmd_quiet sudo virsh --quiet net-start default || 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 + run_cmd cd "$SCRATCH_DIR" + run_cmd git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git + run_cmd cd libvirt-tck + run_cmd sudo avocado --config avocado.config run --job-results-dir "$SCRATCH_DIR"/avocado } -- 2.41.0

One advantage that GitLab's YAML has with Shell commands is that every single line is printed out as is, including control structures. In order to see whether the logic did the same thing and the tests are going to operate on the right set of daemons (monolithic vs modular), lets print the DAEMONS variable that we set depending on the distro we execute the tests on. Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- ci/jobs.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/jobs.sh b/ci/jobs.sh index 861e561039..2e6888232f 100644 --- a/ci/jobs.sh +++ b/ci/jobs.sh @@ -103,6 +103,8 @@ run_integration() { else DAEMONS="virtinterfaced virtlockd virtlogd virtnetworkd virtnodedevd virtnwfilterd virtproxyd virtqemud virtsecretd virtstoraged" fi + + echo "DAEMONS=$DAEMONS" for daemon in $DAEMONS do LOG_OUTPUTS="1:file:/var/log/libvirt/${daemon}.log" -- 2.41.0