Re: [PATCH v3 5/5] tests: Ensure that loopback devices and zfs devices are cleaned up

[Glenn Washburn]

On Mon, 7 Feb 2022 19:36:14 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sun, Feb 06, 2022 at 04:00:12PM -0600, Glenn Washburn wrote:
> > ZFS file systems are not unmounted using umount, but instead by exporting
> > them. So export the ZFS file system that has the same label as the one that
> > was created during the test, if such one exists. This is required to delete
> > the loopback device that uses the ZFS image file. Otherwise the added code
> > to delete all loopback devices setup during the test run will never be able
> > to finish because the loopback device can not be deleted while in use.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  tests/util/grub-fs-tester.in | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> > index 5c722209d..3bc3006de 100644
> > --- a/tests/util/grub-fs-tester.in
> > +++ b/tests/util/grub-fs-tester.in
> > @@ -14,10 +14,27 @@ tempdir=`mktemp -d "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` ||
> >  XORRISOFS_CHARSET="-input-charset UTF-8 -output-charset UTF-8"
> >
> >  MOUNTS=
> > +LODEVICES=
> >  cleanup() {
> > +    if [ -n "$fs" -a -z "${fs##*zfs*}" -a -n "$FSLABEL" ]; then
> > +   zpool list "$FSLABEL" 2>/dev/null &&
> > +   while ! zpool export "$FSLABEL" ; do
> > +       sleep 1;
> > +   done
> > +    fi
> > +
> >      for i in $MOUNTS; do
> >     umount "$i" || :
> >      done
> > +
> > +    for lodev in $LODEVICES; do
> > +   local i=600
> 
> Why 600? How did you come up with this number? Do not we have better
> mechanism to break this loop? If not please add a comment here.

600 is just 5 minutes since we're doing a 1 second sleep. I think
that's a pertty conservative number, none the less it is some what
arbitrary. Its meant to allow for unknown reasons why the loop back
device is still being used. Hopefully whatever is using it is trying to
stop use it. I can't remember if this is actually needed or if I put it
in just to allow some leeway. I'd rather wait an extra 5 minutes in
hopes that this will remove the loopback than have the script end with
a loopback device tht hasn't been cleaned up.

An alternative is to just loop forever, but that's not a god option for
automated CI where you'd rather have the test fail and move on than
have the CI system just kill the whole thing when its alotted time has
been exceeded.

I'd like to get a message to the user that the script finished with
some things not cleaned up, but don't see a great way of doing that. I
could write a message to /dev/tty, but there's a good change that will
be get in all the other output. And it wouldn't get saved to a log
file.

I'm opened to suggestions on any part of this. I'll await your response
before sending out an updated series.

> And if we want to be perfect I would do "local i" at the beginning of
> this function in the previous patch. Then we should initialize i before
> this loop.

Sure, I'll do that.

> > +   while losetup -l -O NAME | grep -q "^$lodev\$"; do
> > +       losetup -d "$lodev" || sleep 1
> > +       [ "$((i--))" = "0" ] && break
> 
> This is a number not a string. Then: [ $((i--)) -eq 0 ] && break

Sure.

Glenn