Libguestfs - Sep 2018 - Re: [PATCH nbdkit 4/4] tests: Add a helper function which waits for nbdkit to start up.

On 9/11/18 1:47 PM, Richard W.M. Jones wrote:
> This assumes bashisms, but bash is required to run the tests.
> 
> This is mostly simple refactoring.  Except for the test-memory*.sh
> tests where nbdkit used to run in the foreground, but that seems to be
> a consequence of some left over debugging.
> ---
> +++ b/tests/functions.sh.in
> @@ -32,6 +32,41 @@
>   # OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   # SUCH DAMAGE.
>   
> +# start_nbdkit args...
> +#
> +# Run nbdkit with args and wait for it to start up.  If it fails
> +# to start up, exit with an error message.
> +start_nbdkit ()
> +{
> +    # Find the -P <pidfile> argument.
> +    pidfile> +    for i in `seq 1 $#`; do
Is seq universally available, even on BSD?  It's not in POSIX.  Since 
you're already using bashisms, you could instead do

for (( i=1; i <= $#; i++)); do

or even:

for i in {1..$#}; do

and drop the need for seq or even a fork (the former works regardless of 
$#, the latter has poor scaling effects if $# is large since it is 
expanded in advance - but then again, none of our tests pass that large 
of $#)
> +        if [ "${!i}" = "-P" ]; then
Unspecified behavior if ${!i} evaluates to "!" (which none of the
tests
do).  Since you're already using bashisms, you could instead do

if [[ ${!i} == -P ]]; then
> +            j=$((i+1))
> +            pidfile="${!j}"
> +        fi
> +    done
This parse loop requires tests to spell it "-P /path/to/file" as two 
separate args, rather than also permitting "-P/path/to/file".  A 
reasonable restriction but might be worth a comment.
> +    if [ "$pidfile" = "" ]; then
> +        echo "$0: start_nbdkit: -P option not present on nbdkit
command line"
> +        exit 1
> +    fi
> +
> +    # Run nbdkit.
> +    nbdkit "$@"
You could also change the contract of this function to REQUIRE the pid 
filename be first, so that you don't have to hunt over $# where -P 
lives, something like:

start_nbdkit()
{
   pidfile=$1
   shift
   nbdkit -P $pidfile "$@"

and change callers to look like:

start_nbdkit foo.pid --myoption1 myplugin ...
> +
> +    # Wait for the pidfile to appear.
> +    for i in `seq 1 10`; do
I guess we were already relying on seq.  Could also be spelled {1..10}
> +        if test -f "$pidfile"; then
> +            break
> +        fi
> +        sleep 1
> +    done
> +    if ! test -f "$pidfile"; then
> +        echo "$0: start_nbdkit: PID file $pidfile was not
created"
> +        exit 1
> +    fi
> +}
> +
> +++ b/tests/test-blocksize.sh
> @@ -31,6 +31,7 @@
>   # OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   # SUCH DAMAGE.
>   
> +source functions.sh
>   set -e
More need for source ./functions.sh throughout the patch
>   
>   files="blocksize1.img blocksize1.log blocksize1.sock blocksize1.pid
> @@ -72,27 +73,13 @@ cleanup ()
>   trap cleanup INT QUIT TERM EXIT ERR
>   
>   # Run two parallel nbdkit; to compare the logs and see what changes.
> -nbdkit -P blocksize1.pid -U blocksize1.sock \
> +start_nbdkit -P blocksize1.pid -U blocksize1.sock \
>          --filter=log file logfile=blocksize1.log blocksize1.img
> -nbdkit -P blocksize2.pid -U blocksize2.sock --filter=blocksize \
> +start_nbdkit -P blocksize2.pid -U blocksize2.sock --filter=blocksize \
>          --filter=log file logfile=blocksize2.log blocksize2.img \
>          minblock=1024 maxdata=512k maxlen=1M
If the first process starts but the second fails, then you have not set 
pid1=, and leak the first process.  This is a regression from the old 
code, which managed to capture pid1 before triggering the trap to 
cleanup(), and thus killed the successful nbdkit.  Several tests are 
impacted.

Less importantly, the old code performed startup in parallel (kick off 
both nbdkit processes, then start the loop; if a pid file was not 
produced by either one of the nbdkit processes, the test determined that 
within 10 seconds); now startup is serial (you don't start a second 
nbdkit until the first one has succeeded). For a test with two nbdkit 
processes, that could expand typical test time from 1 second to 2 (if 
the pid file creation is not instantaneous, but the first sleep 1 was 
sufficient for both processes to be ready); and expand worst-case 
failure detection under heavy load from 10 into 20 seconds (if the first 
succeeded on the last iteration, but the second timed out).  I don't see 
anything wrong with the change, although it might be worth a mention in 
the commit message as intentional.

I'm also seeing a common pattern of assigning pid=$(cat file.pid) right 
after calling start_nbdkt.  Would it be worth changing the signature of 
start_nbdkit() to also populate a pid variable on success, as in:

start_nbdkit blocksize1.pid pid1 myarg1

start_nbdkit()
{
   pidfile=$1
   pidvar=$2
   shift 2
   ...
   pid=$(cat $pidfile)
   eval $pidvar=$pid

which would also have the added benefit of avoiding the regression of 
pid1 not being set if the second nbdkit process doesn't start?

But overall, I like the idea of refactoring the common code for less 
repetition.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Tue, Sep 11, 2018 at 03:02:47PM -0500, Eric Blake wrote:
[Lots of issues]

I'll fix all these, thanks.
> >  files="blocksize1.img blocksize1.log blocksize1.sock
blocksize1.pid
> >@@ -72,27 +73,13 @@ cleanup ()
> >  trap cleanup INT QUIT TERM EXIT ERR
> >  # Run two parallel nbdkit; to compare the logs and see what changes.
> >-nbdkit -P blocksize1.pid -U blocksize1.sock \
> >+start_nbdkit -P blocksize1.pid -U blocksize1.sock \
> >         --filter=log file logfile=blocksize1.log blocksize1.img
> >-nbdkit -P blocksize2.pid -U blocksize2.sock --filter=blocksize \
> >+start_nbdkit -P blocksize2.pid -U blocksize2.sock --filter=blocksize \
> >         --filter=log file logfile=blocksize2.log blocksize2.img \
> >         minblock=1024 maxdata=512k maxlen=1M
> 
> If the first process starts but the second fails, then you have not
> set pid1=, and leak the first process.  This is a regression from
> the old code, which managed to capture pid1 before triggering the
> trap to cleanup(), and thus killed the successful nbdkit.  Several
> tests are impacted.
So one other idea I had is to have start_nbdkit create a trap function
to clean up the nbdkit process (we'd still need trap functions in the
main program for deleting temporary files).  I don't know if this
would work, but if it does it would fix the above problem.
> Less importantly, the old code performed startup in parallel (kick
> off both nbdkit processes, then start the loop; if a pid file was
> not produced by either one of the nbdkit processes, the test
> determined that within 10 seconds); now startup is serial (you don't
> start a second nbdkit until the first one has succeeded). For a test
> with two nbdkit processes, that could expand typical test time from
> 1 second to 2 (if the pid file creation is not instantaneous, but
> the first sleep 1 was sufficient for both processes to be ready);
> and expand worst-case failure detection under heavy load from 10
> into 20 seconds (if the first succeeded on the last iteration, but
> the second timed out).  I don't see anything wrong with the change,
> although it might be worth a mention in the commit message as
> intentional.
I'm not too worried about this since it should cause only a minor
increase in time.
> I'm also seeing a common pattern of assigning pid=$(cat file.pid)
> right after calling start_nbdkt.  Would it be worth changing the
> signature of start_nbdkit() to also populate a pid variable on
> success, as in:
> 
> start_nbdkit blocksize1.pid pid1 myarg1
> 
> start_nbdkit()
> {
>   pidfile=$1
>   pidvar=$2
>   shift 2
>   ...
>   pid=$(cat $pidfile)
>   eval $pidvar=$pid
It was nice that start_nbdkit has the same "signature" as
a regular nbdkit command.
> which would also have the added benefit of avoiding the regression
> of pid1 not being set if the second nbdkit process doesn't start?
> 
> But overall, I like the idea of refactoring the common code for less
> repetition.
I'll have a think about the trap function stuff, but most
likely tomorrow.

Thanks!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 9/11/18 4:14 PM, Richard W.M. Jones wrote:
>> If the first process starts but the second fails, then you have not
>> set pid1=, and leak the first process.  This is a regression from
>> the old code, which managed to capture pid1 before triggering the
>> trap to cleanup(), and thus killed the successful nbdkit.  Several
>> tests are impacted.
> 
> So one other idea I had is to have start_nbdkit create a trap function
> to clean up the nbdkit process (we'd still need trap functions in the
> main program for deleting temporary files).  I don't know if this
> would work, but if it does it would fix the above problem.
Calling 'trap' more than once overwrites the previous version of the 
trap; it's rather awkward to query trap to see what is already installed 
in order to append to that before reinstalling a new trap.  But you 
COULD have sourcing the common stuff unconditionally install a generic 
trap handler:

declare -a cleanup_hook
trap cleanup INT QUIT TERM EXIT ERR
cleanup()
{
   for (( i=0; i < ${#cleanup_hook[@]}; i++ )); do
     ${cleanup_hook[i]}
   done
}
# Appends all arguments as a new command on the list of actions run at 
script exit
cleanup_append()
{
   cleanup_hook[${#cleanup_hook[@]}]="$@"
}

then have the various tests do:

cleanup_append myfunc arg

without having to call trap again.  The arguments to cleanup_append will 
undergo word splitting during the cleanup function execution, but it's 
not a full eval so you can't rely on other shell metacharacters.  Then 
again, because you can append shell function names with arguments as one 
of the commands on the list, it's fairly easy to encapsulate any 
test-specific cleanup in a function.

In the case of start_nbdkit, it would mean you set up a secondary 
function that takes a pid file as a name, then call 'cleanup_append 
helper $pidfile' at the appropriate point, where only the helper 
function has to cat $pidfile if it exists.  Then the test files won't 
have to track pid1= variables.

So that looks even nicer than my idea of passing a variable name:
> 
>> I'm also seeing a common pattern of assigning pid=$(cat file.pid)
>> right after calling start_nbdkt.  Would it be worth changing the
>> signature of start_nbdkit() to also populate a pid variable on
>> success, as in:
>>
>> start_nbdkit blocksize1.pid pid1 myarg1
>>
>> start_nbdkit()
>> {
>>    pidfile=$1
>>    pidvar=$2
>>    shift 2
>>    ...
>>    pid=$(cat $pidfile)
>>    eval $pidvar=$pid
> 
> It was nice that start_nbdkit has the same "signature" as
> a regular nbdkit command.
True, you do have that aspect, so continuing to hunt for -P may still be 
worthwhile, especially if the cleanup hook idea means you don't need to 
track a pid= variable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org