[libvirt PATCH 7/7] ci: integration.sh: Define the SCRATCH_DIR variable for local execution

[Martin Kletzander]

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 at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230123/a5d6c391/attachment.sig>