[libvirt PATCH 0/7] Extract the integration job commands to a shell scripts
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 successful pipeline after the change: https://gitlab.com/eskultety/libvirt/-/pipelines/753090691 Erik Skultety (7): ci: integration: Extract several hidden job definitions to a script ci: integration: Drop the 'install-deps' hidden job and reference ci: integration-template: Drop Fedora 35 check ci: integration.sh: Replace most 'sudo' usage with running as root 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 ci/integration-template.yml | 42 ++---------------------------- ci/integration.sh | 51 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 40 deletions(-) create mode 100644 ci/integration.sh -- 2.39.0
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 9141d5cedf..f9903cdbb4 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.0
On Fri, Jan 20, 2023 at 04:05:57PM +0100, Erik Skultety wrote:
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 9141d5cedf..f9903cdbb4 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
Kind of makes me wish for a command that could take a gitlab job name, and extract the shell commands from the YAML and run them. I guess that's a gitlab runner really, but taking YML directly instead of getting jobs sent from gitlab With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jan 23, 2023 at 12:06:21PM +0000, Daniel P. Berrangé wrote:
On Fri, Jan 20, 2023 at 04:05:57PM +0100, Erik Skultety wrote:
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 9141d5cedf..f9903cdbb4 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
Kind of makes me wish for a command that could take a gitlab job name, and extract the shell commands from the YAML and run them. I guess that's a gitlab runner really, but taking YML directly instead of getting jobs sent from gitlab
Yeah, I don't think that's going to happen. Then again, we'd be probably the only consumers of such thing by far, so adding an explicit script which everyone can read and run without anything else is for the best. Regards, Erik
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 f9903cdbb4..a5d0fee3d9 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.0
We no longer support Fedora 35. While at it, we can also tailor the rest of the check to CentOS Stream 8 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.0
On Fri, Jan 20, 2023 at 04:05:59PM +0100, Erik Skultety wrote:
We no longer support Fedora 35. While at it, we can also tailor the
"older than 35" based on the check below.
rest of the check to CentOS Stream 8 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.0
This is a purely cosmetic change dropping the majority of 'sudo' invocations which we need to do to make final preparations on the test environment. This patch visually divides the part where we need to run as root from the rest of the operations. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration.sh | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/ci/integration.sh b/ci/integration.sh index 160aecf00d..5cee536442 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -1,10 +1,13 @@ #!/bin/sh +# BEGIN AS ROOT +sudo su - + # 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 +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 +echo "DefaultLimitCORE=infinity" >> /etc/systemd/system.conf # Explicitly allow storing cores globally +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" -eq 8 @@ -17,13 +20,16 @@ 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 + augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" &>/dev/null + augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" &>/dev/null + systemctl --quiet stop ${daemon}.service + systemctl restart ${daemon}.socket done -sudo virsh net-start default &>/dev/null || true +virsh net-start default &>/dev/null || true + +# END AS ROOT +exit cd "$SCRATCH_DIR" git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git -- 2.39.0
On Fri, Jan 20, 2023 at 04:06:00PM +0100, Erik Skultety wrote:
This is a purely cosmetic change dropping the majority of 'sudo' invocations which we need to do to make final preparations on the test environment. This patch visually divides the part where we need to run as root from the rest of the operations.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/integration.sh | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/ci/integration.sh b/ci/integration.sh index 160aecf00d..5cee536442 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -1,10 +1,13 @@ #!/bin/sh
+# BEGIN AS ROOT +sudo su - +
This won't work, it will spawn a shell and if the input is closed it will quit again. It will not run the rest of the script as root. Unless there is some special setting somewhere which I don't know.
# 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 +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 +echo "DefaultLimitCORE=infinity" >> /etc/systemd/system.conf # Explicitly allow storing cores globally +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" -eq 8 @@ -17,13 +20,16 @@ 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 + augtool set /files/etc/libvirt/${daemon}.conf/log_filters "'$LOG_FILTERS'" &>/dev/null + augtool set /files/etc/libvirt/${daemon}.conf/log_outputs "'$LOG_OUTPUTS'" &>/dev/null + systemctl --quiet stop ${daemon}.service + systemctl restart ${daemon}.socket done
-sudo virsh net-start default &>/dev/null || true +virsh net-start default &>/dev/null || true + +# END AS ROOT +exit
This would quit the whole script then.
cd "$SCRATCH_DIR" git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git -- 2.39.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> --- ci/integration.sh | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/ci/integration.sh b/ci/integration.sh index 5cee536442..cbd55d4253 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -6,10 +6,15 @@ sudo su - # Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0 for now pip3 install --prefix=/usr avocado-framework==98.0 -echo "DefaultLimitCORE=infinity" >> /etc/systemd/system.conf # Explicitly allow storing cores globally -systemctl daemon-reexec # need to reexec systemd after changing config +# Explicitly allow storing cores globally +echo "DefaultLimitCORE=infinity" >> /etc/systemd/system.conf + +# Need to reexec systemd after changing config +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" @@ -26,6 +31,7 @@ do systemctl restart ${daemon}.socket done +# Make sure the default network is started on all platforms virsh net-start default &>/dev/null || true # END AS ROOT -- 2.39.0
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 cbd55d4253..41326d6e40 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -15,7 +15,7 @@ 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" ] && [ "$VERSION_ID" -eq 8 ] then DAEMONS="libvirtd virtlockd virtlogd" else -- 2.39.0
On Fri, Jan 20, 2023 at 04:06:02PM +0100, Erik Skultety wrote:
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 cbd55d4253..41326d6e40 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -15,7 +15,7 @@ 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" ] && [ "$VERSION_ID" -eq 8 ]
Or simply: if [ "$ID" = "centos" -a "$VERSION_ID" -eq 8 ]
then DAEMONS="libvirtd virtlockd virtlogd" else -- 2.39.0
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 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit +# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null +fi + cd "$SCRATCH_DIR" git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git cd libvirt-tck -- 2.39.0
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
+fi + cd "$SCRATCH_DIR" git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git cd libvirt-tck -- 2.39.0
On Mon, Jan 23, 2023 at 09:41:10AM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections. So, I preferred consistency here, since I wouldn't realistically expect an engineer to have /tmp/scratch prior to executing script given the main motivation here is to quickly get a throwaway test machine to run the script and THEN debug if the tests fail. So, while I agree you're right here, I don't think it's a likely scenario. Erik
On Mon, Jan 23, 2023 at 10:50:31AM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 09:41:10AM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
I don't really understand how that affects a change from: SCRATCH_DIR="/tmp/scratch" mkdir "$SCRATCH_DIR" to something like SCRATCH_DIR=$(mktemp -d) or possibly SCRATCH_DIR=$(mktemp -d "/tmp/scratch.XXX") especially when only used without the SCRATCH_DIR variable.
So, I preferred consistency here, since I wouldn't realistically expect an engineer to have /tmp/scratch prior to executing script given the main motivation here is to quickly get a throwaway test machine to run the script and THEN debug if the tests fail. So, while I agree you're right here, I don't think it's a likely scenario.
Erik
On Mon, Jan 23, 2023 at 11:42:52AM +0100, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 10:50:31AM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 09:41:10AM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
I don't really understand how that affects a change from:
SCRATCH_DIR="/tmp/scratch" mkdir "$SCRATCH_DIR"
to something like
SCRATCH_DIR=$(mktemp -d)
Simple, ^this is not consistent and results in a different environments.
or possibly
SCRATCH_DIR=$(mktemp -d "/tmp/scratch.XXX")
^This one is close enough, I'm fine doing that, but again, one expects that the directory will be in /tmp/scratch and it isn't. We can keep arguing about "you can just hit tab-tab in a shell", or "that association is obvious to anyone", or "any engineer who wishes to debug libvirt must be able to figure out what the correct directory is". My only argument was about consistent and uniform user experience. However, the deal breaker here kinda supporting your suggestion and where my original proposal fails is quite different actually - not all platforms actually clean /tmp on reboots, e.g. CentOS Stream - in this particular case it will be done with systemd-tmpfiles-clean timer and service, other platforms might employ a different mechanism, but the point is, if it's not mounted as tmpfs, the reboot guarantee isn't there and hence we could have a left-over directory from a previous run. However, having said that, rather than ending up with multiple /tmp/scratch.XXX directories where you actually have to verify which one is it that you want to interact with, I'd still suggest to preventively remove the directory and re-create it. Why? Because the proposed scenario is to actually get a throwaway VM with an "identical" (it won't be because of package versions) environment to that running in GitLab and we can even document this behaviour so that these VMs are created on-demand locally rather than re-using them which also leads to other potential problems. So, given that I document the recommendation wrt creating throwaway VMs, would you agree to: SCRATCH_DIR="/tmp/scratch" if [ -d $SCRATCH_DIR ] then rm -rf $SCRATCH_DIR fi mkdir "$SCRATCH_DIR" Erik
On Mon, Jan 23, 2023 at 12:08:23PM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 11:42:52AM +0100, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 10:50:31AM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 09:41:10AM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
I don't really understand how that affects a change from:
SCRATCH_DIR="/tmp/scratch" mkdir "$SCRATCH_DIR"
to something like
SCRATCH_DIR=$(mktemp -d)
Simple, ^this is not consistent and results in a different environments.
or possibly
SCRATCH_DIR=$(mktemp -d "/tmp/scratch.XXX")
^This one is close enough, I'm fine doing that, but again, one expects that the directory will be in /tmp/scratch and it isn't. We can keep arguing about "you can just hit tab-tab in a shell", or "that association is obvious to anyone", or "any engineer who wishes to debug libvirt must be able to figure out what the correct directory is". My only argument was about consistent and uniform user experience. However, the deal breaker here kinda supporting your suggestion and where my original proposal fails is quite different actually - not all platforms actually clean /tmp on reboots, e.g. CentOS Stream - in this particular case it will be done with systemd-tmpfiles-clean timer and service, other platforms might employ a different mechanism, but the point is, if it's not mounted as tmpfs, the reboot guarantee isn't there and hence we could have a left-over directory from a previous run.
Since running as root you might just mount tmpfs over /tmp/scratch. That is if you are fine with the RAM being used for storage, but I presume that not much is needed.
However, having said that, rather than ending up with multiple /tmp/scratch.XXX directories where you actually have to verify which one is it that you want to interact with, I'd still suggest to preventively remove the directory and re-create it. Why? Because the proposed scenario is to actually get a throwaway VM with an "identical" (it won't be because of package versions) environment to that running in GitLab and we can even document this behaviour so that these VMs are created on-demand locally rather than re-using them which also leads to other potential problems.
So, given that I document the recommendation wrt creating throwaway VMs, would you agree to:
SCRATCH_DIR="/tmp/scratch" if [ -d $SCRATCH_DIR ] then rm -rf $SCRATCH_DIR fi mkdir "$SCRATCH_DIR"
So I guess I misunderstood and I need some clarification. This script will run inside the VM used for testing and is not in any case meant to be run on a machine used for other purposes since it has side effects, right? If that's the case (and looking at it again it seems like it is) I'm fine with both solutions. And I'm guessing the /tmp/scratch is either hardcoded somewhere else or it is expected that someone can diff some outputs with the full path, then (possibly in the future)?
Erik
On Mon, Jan 23, 2023 at 12:40:01PM +0100, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 12:08:23PM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 11:42:52AM +0100, Martin Kletzander wrote:
On Mon, Jan 23, 2023 at 10:50:31AM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 09:41:10AM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
I don't really understand how that affects a change from:
SCRATCH_DIR="/tmp/scratch" mkdir "$SCRATCH_DIR"
to something like
SCRATCH_DIR=$(mktemp -d)
Simple, ^this is not consistent and results in a different environments.
or possibly
SCRATCH_DIR=$(mktemp -d "/tmp/scratch.XXX")
^This one is close enough, I'm fine doing that, but again, one expects that the directory will be in /tmp/scratch and it isn't. We can keep arguing about "you can just hit tab-tab in a shell", or "that association is obvious to anyone", or "any engineer who wishes to debug libvirt must be able to figure out what the correct directory is". My only argument was about consistent and uniform user experience. However, the deal breaker here kinda supporting your suggestion and where my original proposal fails is quite different actually - not all platforms actually clean /tmp on reboots, e.g. CentOS Stream - in this particular case it will be done with systemd-tmpfiles-clean timer and service, other platforms might employ a different mechanism, but the point is, if it's not mounted as tmpfs, the reboot guarantee isn't there and hence we could have a left-over directory from a previous run.
Since running as root you might just mount tmpfs over /tmp/scratch. That is if you are fine with the RAM being used for storage, but I presume that not much is needed.
Sure, but again, we're deviating from the consistent experience, not that many people really have access to the VMs scheduled by GitLab, so... ...
So, given that I document the recommendation wrt creating throwaway VMs, would you agree to:
SCRATCH_DIR="/tmp/scratch" if [ -d $SCRATCH_DIR ] then rm -rf $SCRATCH_DIR fi mkdir "$SCRATCH_DIR"
So I guess I misunderstood and I need some clarification. This script will run inside the VM used for testing and is not in any case meant to be run on a machine used for other purposes since it has side effects, right?
That is correct, by no chance is this script meant to be used on the host, particularly because it has side effects and hence a fresh testing environment (a VM in this case) is always recommended.
If that's the case (and looking at it again it seems like it is) I'm fine with both solutions. And I'm guessing the /tmp/scratch is either hardcoded somewhere else or it is expected that someone can diff some outputs with the full path, then (possibly in the future)?
It is hardcoded only a couple of times in a few gitlab jobs (the rest is inherited), but once the variable has been defined with a hardcoded value, then only the variable is referenced. But I guess the answer you're looking for is, yes, we're pulling test results out of this directory in case a job fails. Erik
On Mon, Jan 23, 2023 at 10:50:31AM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 09:41:10AM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
So, I preferred consistency here, since I wouldn't realistically expect an engineer to have /tmp/scratch prior to executing script given the main motivation here is to quickly get a throwaway test machine to run the script and THEN debug if the tests fail. So, while I agree you're right here, I don't think it's a likely scenario.
I'm wondering why I put it at /tmp/scratch in the first place when we started using gitlab, and struggling to come up with a justification. In fact I'm not entirely convinced we really need a SCRATCH_DIR env variable at all, since AFAICT, we only use it one place for creating $VROOT. In terms of standalone reproduction of build env, it would be saner to use VOORT=$CWD/vroot, which I think would probably work under GitLab ok too, and do away with SCRATCH_DIR. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
...
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
So, I preferred consistency here, since I wouldn't realistically expect an engineer to have /tmp/scratch prior to executing script given the main motivation here is to quickly get a throwaway test machine to run the script and THEN debug if the tests fail. So, while I agree you're right here, I don't think it's a likely scenario.
I'm wondering why I put it at /tmp/scratch in the first place when we started using gitlab, and struggling to come up with a justification.
In fact I'm not entirely convinced we really need a SCRATCH_DIR env variable at all, since AFAICT, we only use it one place for creating $VROOT.
In terms of standalone reproduction of build env, it would be saner to use VOORT=$CWD/vroot, which I think would probably work under GitLab ok too, and do away with SCRATCH_DIR.
That would be just a replacement of one var for another and we'd still have to keep clearing/removing vroot anyway - one thing I didn't mention, because it was irrelevant up until this point, is that in the future we should improve the local experience even more by following the logic we have for local container envs where we mount the git directory inside the container as a volume. Using the same mantra, I can imagine us using e.g. virtiofs to mount the git dir to the VM so that the developer can run ninja build immediately without having to wait for gitlab and test directly with their own development branch in a safe environment. The difference is that while /tmp would be cleared on VM destroy, vroot's content as you propose would remain as a side-effect after destroying the VM unless we clear it in which case it's no different to the current SCRATCH_DIR solution, so with this in mind I think we should keep it the way we have. Regards, Erik
On Mon, Jan 23, 2023 at 02:19:23PM +0100, Erik Skultety wrote:
...
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
So, I preferred consistency here, since I wouldn't realistically expect an engineer to have /tmp/scratch prior to executing script given the main motivation here is to quickly get a throwaway test machine to run the script and THEN debug if the tests fail. So, while I agree you're right here, I don't think it's a likely scenario.
I'm wondering why I put it at /tmp/scratch in the first place when we started using gitlab, and struggling to come up with a justification.
In fact I'm not entirely convinced we really need a SCRATCH_DIR env variable at all, since AFAICT, we only use it one place for creating $VROOT.
In terms of standalone reproduction of build env, it would be saner to use VOORT=$CWD/vroot, which I think would probably work under GitLab ok too, and do away with SCRATCH_DIR.
That would be just a replacement of one var for another and we'd still have to keep clearing/removing vroot anyway - one thing I didn't mention, because it was irrelevant up until this point, is that in the future we should improve the local experience even more by following the logic we have for local container envs where we mount the git directory inside the container as a volume. Using the same mantra, I can imagine us using e.g. virtiofs to mount the git dir to the VM so that the developer can run ninja build immediately without having to wait for gitlab and test directly with their own development branch in a safe environment.
The difference is that while /tmp would be cleared on VM destroy, vroot's content as you propose would remain as a side-effect after destroying the VM unless we clear it in which case it's no different to the current SCRATCH_DIR solution, so with this in mind I think we should keep it the way we have.
The /tmp directory is often a ram disk though, hence my aversion to using /tmp, as you can't be confident it has sufficient space for without impacting other aspects of the system. With $CWD/vroot you can expect access to the full extent of the user's storage. If we're passing through a host git checkout though, that's sketchy, unless we did a git clone --reflink, to get a pristine checkout for each job in an efficient way. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jan 23, 2023 at 01:27:12PM +0000, Daniel P. Berrangé wrote:
On Mon, Jan 23, 2023 at 02:19:23PM +0100, Erik Skultety wrote:
...
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
So, I preferred consistency here, since I wouldn't realistically expect an engineer to have /tmp/scratch prior to executing script given the main motivation here is to quickly get a throwaway test machine to run the script and THEN debug if the tests fail. So, while I agree you're right here, I don't think it's a likely scenario.
I'm wondering why I put it at /tmp/scratch in the first place when we started using gitlab, and struggling to come up with a justification.
In fact I'm not entirely convinced we really need a SCRATCH_DIR env variable at all, since AFAICT, we only use it one place for creating $VROOT.
In terms of standalone reproduction of build env, it would be saner to use VOORT=$CWD/vroot, which I think would probably work under GitLab ok too, and do away with SCRATCH_DIR.
That would be just a replacement of one var for another and we'd still have to keep clearing/removing vroot anyway - one thing I didn't mention, because it was irrelevant up until this point, is that in the future we should improve the local experience even more by following the logic we have for local container envs where we mount the git directory inside the container as a volume. Using the same mantra, I can imagine us using e.g. virtiofs to mount the git dir to the VM so that the developer can run ninja build immediately without having to wait for gitlab and test directly with their own development branch in a safe environment.
The difference is that while /tmp would be cleared on VM destroy, vroot's content as you propose would remain as a side-effect after destroying the VM unless we clear it in which case it's no different to the current SCRATCH_DIR solution, so with this in mind I think we should keep it the way we have.
The /tmp directory is often a ram disk though, hence my aversion to using /tmp, as you can't be confident it has sufficient space for without impacting other aspects of the system. With $CWD/vroot you can expect access to the full extent of the user's storage.
Fair enough.
If we're passing through a host git checkout though, that's sketchy, unless we did a git clone --reflink, to get a pristine checkout for each job in an efficient way.
--reflink is not at option, did you mean --local which creates hardlinks for the git objects or --reference <repo> that pulls objects from the reference repo? For local containers we're doing the former before mounting them as volumes. Regards, Erik
On Mon, Jan 23, 2023 at 12:02:44PM +0000, Daniel P. Berrangé wrote:
On Mon, Jan 23, 2023 at 10:50:31AM +0100, Erik Skultety wrote:
On Mon, Jan 23, 2023 at 09:41:10AM +0100, Martin Kletzander wrote:
On Fri, Jan 20, 2023 at 04:06:03PM +0100, Erik Skultety wrote:
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 | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/ci/integration.sh b/ci/integration.sh index 41326d6e40..ac04c46d8e 100644 --- a/ci/integration.sh +++ b/ci/integration.sh @@ -37,6 +37,14 @@ virsh net-start default &>/dev/null || true # END AS ROOT exit
+# 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="/tmp/scratch" + mkdir "$SCRATCH_DIR" 2>/dev/null
This could fail if someone has this directory already. Which is a good thing as otherwise it could override some of it. But wouldn't it be nicer to use mktemp -d and print the result?
Although an option, the main motivation here to remain consistent with how it works in GitLab. Since we define SCRATCH_DIR under the 'vars' section, IIRC you can only use a scalar value, not a command (if we can, I retract my argument) and hence we'd have to export and define the variable under each script, before_script, after_script sections.
So, I preferred consistency here, since I wouldn't realistically expect an engineer to have /tmp/scratch prior to executing script given the main motivation here is to quickly get a throwaway test machine to run the script and THEN debug if the tests fail. So, while I agree you're right here, I don't think it's a likely scenario.
I'm wondering why I put it at /tmp/scratch in the first place when we started using gitlab, and struggling to come up with a justification.
I think I may have found the answer to ^this - variables can only be static strings, so $PWD/vroot would not be an option in such case. The obvious benefit is that the variables are inherited to all jobs which extend a template, so even if we currently use SCRATCH_DIR in a single YAML file only, I'd like to keep it that way.
In fact I'm not entirely convinced we really need a SCRATCH_DIR env variable at all, since AFAICT, we only use it one place for creating $VROOT.
In terms of standalone reproduction of build env, it would be saner to use VOORT=$CWD/vroot, which I think would probably work under GitLab ok too, and do away with SCRATCH_DIR.
Like I mentioned in my previous response, seems like we'd only rename the variable and even had to re-define it in all (.*_)?script sections along with all inherited templates. So, to keep being consistent with other libvirt projects where I believe we still use SCRATCH_DIR in this form. Let's just fix the ramdisk mount issue and move it to /var/tmp - it's a static path, everyone has access there (EUID depends on how the runner was configured), and it's persistent storage Regards, Erik
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Martin Kletzander