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@xxxxxxxxxx> > > > > > > --- > > > > > > 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
