Re: [Qemu-block] [Qemu-devel] [PATCH v5 03/10] qemu-iotests: automatically clean up bash protocol servers

[Paolo Bonzini]

On 19/10/2017 16:52, Jeff Cody wrote:
> On Thu, Oct 19, 2017 at 12:23:39PM +0200, Paolo Bonzini wrote:
>> On 18/10/2017 19:27, Jeff Cody wrote:
>>> On final exit, yes, a test needs not remember to remove all of its mouse
>>> droppings.  But as far as not needing to remove images in intermediate
>>> stages of a given test, I think that assumes too much. For instance,
>>> qemu-img _should_ be able to rebuild a format on top of the same image.  But
>>> maybe a test wants to see if that specific functionality actually works as
>>> intended, and compares removing and creating an image to rebuilding on top
>>> of an image, etc.
>>
>> Right, but let's draw a line, does such a test need to support multiple
>> protocols?  For example:
>>
> This is a good question.  But, I'm not sure that this is a question this
> series is trying to answer; one goal of this series is to keep the existing
> APIs currently in use by tests unchanged.

Right, but in order to do so it's also making the line between test and
harness unclear, which is something I'd like to avoid (when I looked at
it a couple months ago, the line was surprisingly clear apart from some
confusion around searching for programs, and separating check vs.
common.rc turned out to be very easy).

>> [snip] So, this is why I was wondering whether patches 3/4 kinda paint
>> ourselves in the corner.
> 
> I think this conflates a bit how we'd like to restructure tests in a future
> harness rewrite, and what this series does.

This is true.  But this sure is not exactly keeping the test APIs
intact.  The APIs are intact, but the usage isn't---for example, for
patch 9 to work you need to _not_ use _cleanup_test_img in the tests.

> If we look at what patches 3 & 4 do:
> 
> Patch 3:
> 
>     - Code movement within common.rc, but doesn't change the API.  Tests
>       still just call _cleanup_test_img() as needed.
> 
>     - It does break apart _cleanup_test_img(), thereby technically creating
>       some new APIs available to future tests:
>          * _cleanup_nbd()
>          * _cleanup_vxhs()
>          * _cleanup_rbd()
>          * _cleanup_sheepdog()
>          * _cleanup_protocols()
> 
>       Maybe these new APIs are a sticking point?  If so, perhaps we can mark
>       them (via comments) as internal-only?
> 
>     - ./check does an extra protocol cleanup check after a test is run, via
>       the new _cleanup_protocols().
> 
>     As far as existing tests go, no changes yet.

Here I'd like to remove _cleanup_test_img as a test API even.  Most
invocations out of the "trap" are unnecessary.  Some (for VMDK) can be
changed to _rm_test_img or changed to create a file with a new name (to
make patch 9 more effective).

With that change, we can apply patch 4 with no issue.

> Patch 4:
> 
>     - Removes test exit cleanup from tests
> 
>     Now this does change test behavior, as it relies on the harness for file
>     and protocol cleanup at test exit.
> 
>     This will indeed paint us in a corner if we want a new check.py to not
>     perform the test exit cleanup, and leave test cleanup (either partially
>     or fully) as the responsibility for the tests. [1]

I think patch 9 is enough proof that check should perform the test exit
cleanup.

But again, the thing I'm worried about is mixing code between check and
tests.

>> So, looking at the patches:
>>
>> - 1, 2, 7, 8, 9 are definitely good ideas, and should be done _before_
>> an eventual/hypothetical Python rewrite of "check".
> 
> Alas, 9 requires 4 (which in turn requires 3).  Without 4, there is nothing
> to keep, as the tests try to remove it all.
> 
>> - for 5, 6 I think we should be using shell job control instead in
>> "check" ('set -m')
>>
>>   #! /bin/sh
>>   set -m
>>   # Start a job which leaves two processes behind.  By starting it
>>   # in the background, we can get the leader process's pid in $!
>>   # That pid is also the process group id of the whole job.
>>   sh -c 'echo subshell pid is $$; sleep 10 | sleep 15 &' &
>>   pgrp=$!
>>   wait
>>   echo '$! is '$pgrp', killing all processes in that group:'
>>   pgrep -g $pgrp -a
>>   kill -TERM -$pgrp
>>   sleep 1
>>   echo Leftover processes have been killed:
>>   ps axo pid,ppid,pgrp,stat,tty,comm|grep sleep
>>
> 
> Existing tests right now use _cleanup_qemu in their tests (outside of final
> cleanup): 095 109 117 130, etc.  So we can do process control differently,
> but _cleanup_qemu still needs to exist and also clean up other files (such
> as fifos, close fds, etc..), and provide the same functionality (optional
> wait-for-completion, etc.), if we are keeping the usage by tests the same.

Yes, _cleanup_qemu can stay in the tests.

> [1] So on that point: do you think individual tests should be responsible
> for cleaning up files and processes at test exit?  If that answer is a 'yes'
> to either files or processes, then 3, 4, 6 (and maybe 9) are incompatible
> with a future redesign with that assumption.  FWIW, my thought is that the
> answer should be "the harness shall perform both file and process cleanup on
> test exit".

I definitely agree on that, but I that the harness can be a little less
refined: kill the whole process group and rm -rf the whole test
directory.  Protocols may need a little bit of refinement, but
everything else should use OS services and ignore the QEMUness of
qemu-iotests (and especially the fact that tests are shell scripts).

Paolo