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.0On 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. > > 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 :|
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
... > > 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 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
© 2016 - 2026 Red Hat, Inc.