| Message ID | 503559b438cd67b865d32d7d0577afc7ee15f32c.1508257445.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/17/2017 11:31 AM, Jeff Cody wrote: > For bash tests, this allows 'check' to reap all launch protocol > servers / processes, after a test has finished running. Nice stuff! > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > tests/qemu-iotests/check | 13 +++++++ > tests/qemu-iotests/common.rc | 93 +++++++++++++++++++++++++++++--------------- > 2 files changed, 75 insertions(+), 31 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On 17/10/2017 18:31, Jeff Cody wrote: > ) > else > + # Do this in a sub-shell, so we are operating on the right > + # TEST_DIR / QEMU_TEST_DIR > ( > export TEST_DIR=$TEST_DIR_SEQ > cd "$source_iotests"; Where is the missing ")"? > @@ -837,6 +841,15 @@ do > fi > fi > > + # Do this in a sub-shell, so we are operating on the right > + # TEST_DIR / QEMU_TEST_DIR > + ( > + export TEST_DIR=$TEST_DIR_SEQ > + . "$source_iotests/common.config" > + . "$source_iotests/common.rc" > + > + _cleanup_protocols "check" wasn't including common.rc before this patch, and most of the content of that file doesn't apply to "check". So if we want to move cleanup code to "check" we should remove it from common.rc too. In general, I think we should strive to have a better separation between tests and harness. This for example could let us write a different harness for the same tests and integrate the tests better into Avocado. Hence I see one advantage and one disadvantage in your series: * by adding more functionality to "check", it shows that the current separation may fail with a more sophisticated harness (such as the one you are creating here) * it adds a lot more knowledge of QEMU (especially protocols) in "check", but there is still some unbalance: tests create the images and the protocol servers, but the harness cleans it up. The visible result of this unbalance is for example how multi-process protocol tests can fail when multiple tests try to bind the same address. I think it's the right direction, but it feels like it's not there yet... Sorry---I know this is not very constructive, but I hope it helps anyway. Maybe we should actually rewrite "check" in Python. That would force us to think more about the design. Paolo
On Wed, Oct 18, 2017 at 04:46:20PM +0200, Paolo Bonzini wrote: > On 17/10/2017 18:31, Jeff Cody wrote: > > ) > > else > > + # Do this in a sub-shell, so we are operating on the right > > + # TEST_DIR / QEMU_TEST_DIR > > ( > > export TEST_DIR=$TEST_DIR_SEQ > > cd "$source_iotests"; > > Where is the missing ")"? It's part of the diff context, not a change itself... it's still there, just not shown in the patch. > > > @@ -837,6 +841,15 @@ do > > fi > > fi > > > > + # Do this in a sub-shell, so we are operating on the right > > + # TEST_DIR / QEMU_TEST_DIR > > + ( > > + export TEST_DIR=$TEST_DIR_SEQ > > + . "$source_iotests/common.config" > > + . "$source_iotests/common.rc" > > + > > + _cleanup_protocols > > "check" wasn't including common.rc before this patch, and most of the > content of that file doesn't apply to "check". So if we want to move > cleanup code to "check" we should remove it from common.rc too. > > In general, I think we should strive to have a better separation between > tests and harness. This for example could let us write a different > harness for the same tests and integrate the tests better into Avocado. > Hence I see one advantage and one disadvantage in your series: > > * by adding more functionality to "check", it shows that the current > separation may fail with a more sophisticated harness (such as the one > you are creating here) > > * it adds a lot more knowledge of QEMU (especially protocols) in > "check", but there is still some unbalance: tests create the images and > the protocol servers, but the harness cleans it up. The visible result > of this unbalance is for example how multi-process protocol tests can > fail when multiple tests try to bind the same address. > > I think it's the right direction, but it feels like it's not there > yet... Sorry---I know this is not very constructive, but I hope it > helps anyway. > > Maybe we should actually rewrite "check" in Python. That would force us > to think more about the design. > I think writing a more sophisticated harness in another language, such as Python, has merit. But to clarify, do you mean that as a 'nack' to this series, or as something to be done later, after this series? If this series improves the existing harness, I don't see why to exclude it because a new re-write could be superior (which I don't dispute).
On 10/18/2017 09:46 AM, Paolo Bonzini wrote: > On 17/10/2017 18:31, Jeff Cody wrote: >> ) >> else >> + # Do this in a sub-shell, so we are operating on the right >> + # TEST_DIR / QEMU_TEST_DIR >> ( >> export TEST_DIR=$TEST_DIR_SEQ >> cd "$source_iotests"; > > Where is the missing ")"? This hunk is just adding comments, not '(', so it is already well-paired.
On 18/10/2017 17:03, Jeff Cody wrote: > On Wed, Oct 18, 2017 at 04:46:20PM +0200, Paolo Bonzini wrote: >> On 17/10/2017 18:31, Jeff Cody wrote: >>> ) >>> else >>> + # Do this in a sub-shell, so we are operating on the right >>> + # TEST_DIR / QEMU_TEST_DIR >>> ( >>> export TEST_DIR=$TEST_DIR_SEQ >>> cd "$source_iotests"; >> Where is the missing ")"? > It's part of the diff context, not a change itself... it's still there, just > not shown in the patch. > >>> @@ -837,6 +841,15 @@ do >>> fi >>> fi >>> >>> + # Do this in a sub-shell, so we are operating on the right >>> + # TEST_DIR / QEMU_TEST_DIR >>> + ( >>> + export TEST_DIR=$TEST_DIR_SEQ >>> + . "$source_iotests/common.config" >>> + . "$source_iotests/common.rc" >>> + >>> + _cleanup_protocols >> "check" wasn't including common.rc before this patch, and most of the >> content of that file doesn't apply to "check". So if we want to move >> cleanup code to "check" we should remove it from common.rc too. >> >> In general, I think we should strive to have a better separation between >> tests and harness. This for example could let us write a different >> harness for the same tests and integrate the tests better into Avocado. >> Hence I see one advantage and one disadvantage in your series: >> >> * by adding more functionality to "check", it shows that the current >> separation may fail with a more sophisticated harness (such as the one >> you are creating here) >> >> * it adds a lot more knowledge of QEMU (especially protocols) in >> "check", but there is still some unbalance: tests create the images and >> the protocol servers, but the harness cleans it up. The visible result >> of this unbalance is for example how multi-process protocol tests can >> fail when multiple tests try to bind the same address. >> >> I think it's the right direction, but it feels like it's not there >> yet... Sorry---I know this is not very constructive, but I hope it >> helps anyway. >> >> Maybe we should actually rewrite "check" in Python. That would force us >> to think more about the design. > > I think writing a more sophisticated harness in another language, such as > Python, has merit. But to clarify, do you mean that as a 'nack' to this > series, or as something to be done later, after this series? If this series > improves the existing harness, I don't see why to exclude it because a new Well, I have no idea (hence the "not very constructive" part). I'm only "nacking" the sourcing of common.rc in the check script. The series improves the harness, but it also sets a very different separation between the tests and the harness (especially WRT the tests cleaning up after themselves). The level of separation would at least be clearer if check didn't include common.rc. Paolo
On Wed, Oct 18, 2017 at 05:16:44PM +0200, Paolo Bonzini wrote: > On 18/10/2017 17:03, Jeff Cody wrote: > > On Wed, Oct 18, 2017 at 04:46:20PM +0200, Paolo Bonzini wrote: > >> On 17/10/2017 18:31, Jeff Cody wrote: > >>> ) > >>> else > >>> + # Do this in a sub-shell, so we are operating on the right > >>> + # TEST_DIR / QEMU_TEST_DIR > >>> ( > >>> export TEST_DIR=$TEST_DIR_SEQ > >>> cd "$source_iotests"; > >> Where is the missing ")"? > > It's part of the diff context, not a change itself... it's still there, just > > not shown in the patch. > > > >>> @@ -837,6 +841,15 @@ do > >>> fi > >>> fi > >>> > >>> + # Do this in a sub-shell, so we are operating on the right > >>> + # TEST_DIR / QEMU_TEST_DIR > >>> + ( > >>> + export TEST_DIR=$TEST_DIR_SEQ > >>> + . "$source_iotests/common.config" > >>> + . "$source_iotests/common.rc" > >>> + > >>> + _cleanup_protocols > >> "check" wasn't including common.rc before this patch, and most of the > >> content of that file doesn't apply to "check". So if we want to move > >> cleanup code to "check" we should remove it from common.rc too. > >> > >> In general, I think we should strive to have a better separation between > >> tests and harness. This for example could let us write a different > >> harness for the same tests and integrate the tests better into Avocado. > >> Hence I see one advantage and one disadvantage in your series: > >> > >> * by adding more functionality to "check", it shows that the current > >> separation may fail with a more sophisticated harness (such as the one > >> you are creating here) > >> > >> * it adds a lot more knowledge of QEMU (especially protocols) in > >> "check", but there is still some unbalance: tests create the images and > >> the protocol servers, but the harness cleans it up. The visible result > >> of this unbalance is for example how multi-process protocol tests can > >> fail when multiple tests try to bind the same address. > >> > >> I think it's the right direction, but it feels like it's not there > >> yet... Sorry---I know this is not very constructive, but I hope it > >> helps anyway. > >> > >> Maybe we should actually rewrite "check" in Python. That would force us > >> to think more about the design. > > > > I think writing a more sophisticated harness in another language, such as > > Python, has merit. But to clarify, do you mean that as a 'nack' to this > > series, or as something to be done later, after this series? If this series > > improves the existing harness, I don't see why to exclude it because a new > > Well, I have no idea (hence the "not very constructive" part). I'm only > "nacking" the sourcing of common.rc in the check script. > > The series improves the harness, but it also sets a very different > separation between the tests and the harness (especially WRT the tests > cleaning up after themselves). The level of separation would at least > be clearer if check didn't include common.rc. > I can get rid of the common.rc includes prior to running the tests, but this series really requires including common.rc in the spot you mentioned, for automatically cleaning up protocol and QEMU processes. That auto-cleanup is arguably a big improvement, as it has been relatively common to run across tests that leave processes running in the background. I agree that it sets up different expectations, but that is at least partly intentional. I don't really want to have to rely on individually written tests to clean up properly. That is ~200 chances (and growing) for a mistake; instead, this series moves that responsibility into a single place to maintain.
On 18/10/2017 17:34, Jeff Cody wrote: >> Well, I have no idea (hence the "not very constructive" part). I'm only >> "nacking" the sourcing of common.rc in the check script. >> >> The series improves the harness, but it also sets a very different >> separation between the tests and the harness (especially WRT the tests >> cleaning up after themselves). The level of separation would at least >> be clearer if check didn't include common.rc. >> > I can get rid of the common.rc includes prior to running the tests, but this > series really requires including common.rc in the spot you mentioned, for > automatically cleaning up protocol and QEMU processes. Understood, but does it have to be common.rc? Can it be a different file? That at least would still make it clear what check is doing (for example it is not launching qemu, which is part of common.rc). > That auto-cleanup is arguably a big improvement, as it has been relatively > common to run across tests that leave processes running in the background. > > I agree that it sets up different expectations, but that is at least partly > intentional. I don't really want to have to rely on individually written > tests to clean up properly. That is ~200 chances (and growing) for a > mistake; instead, this series moves that responsibility into a single place > to maintain. Understood, that's also why I'm all but nacking the entire series! Paolo
On Wed, Oct 18, 2017 at 04:46:20PM +0200, Paolo Bonzini wrote: > Maybe we should actually rewrite "check" in Python. That would force us > to think more about the design. I think that would be great. Any shell script that's more than 5 lines long is a shell script that should not exist. It is a terrible language for doing anything remotely complex, and the iotest harness easily falls into category. Regards, Daniel
On Wed, Oct 18, 2017 at 05:39:40PM +0200, Paolo Bonzini wrote: > On 18/10/2017 17:34, Jeff Cody wrote: > >> Well, I have no idea (hence the "not very constructive" part). I'm only > >> "nacking" the sourcing of common.rc in the check script. > >> > >> The series improves the harness, but it also sets a very different > >> separation between the tests and the harness (especially WRT the tests > >> cleaning up after themselves). The level of separation would at least > >> be clearer if check didn't include common.rc. > >> > > I can get rid of the common.rc includes prior to running the tests, but this > > series really requires including common.rc in the spot you mentioned, for > > automatically cleaning up protocol and QEMU processes. > > Understood, but does it have to be common.rc? Can it be a different > file? That at least would still make it clear what check is doing (for > example it is not launching qemu, which is part of common.rc). > Here is what we need from common.rc for this series: _rm_test_img _cleanup_nbd _cleanup_vxhs _cleanup_rbd _cleanup_sheepdog _cleanup_protocols _cleanup_test_img They all have a common theme (cleanup), so I could move them all to a common.cleanup (naming suggestion?) file (which would need to be included by common.rc, as well). Would this be a strong enough delineation to overcome your concerns? > > That auto-cleanup is arguably a big improvement, as it has been relatively > > common to run across tests that leave processes running in the background. > > > > I agree that it sets up different expectations, but that is at least partly > > intentional. I don't really want to have to rely on individually written > > tests to clean up properly. That is ~200 chances (and growing) for a > > mistake; instead, this series moves that responsibility into a single place > > to maintain. > > Understood, that's also why I'm all but nacking the entire series! > > Paolo
On 18/10/2017 17:50, Jeff Cody wrote: > Here is what we need from common.rc for this series: > > _rm_test_img > _cleanup_nbd > _cleanup_vxhs > _cleanup_rbd > _cleanup_sheepdog > _cleanup_protocols > _cleanup_test_img > > > They all have a common theme (cleanup), so I could move them all to a > common.cleanup (naming suggestion?) file (which would need to be included by > common.rc, as well). > > Would this be a strong enough delineation to overcome your concerns? A great start. Which of these are actually needed by the tests (and hence by common.rc) and why? Paolo
On Wed, Oct 18, 2017 at 05:51:53PM +0200, Paolo Bonzini wrote: > On 18/10/2017 17:50, Jeff Cody wrote: > > Here is what we need from common.rc for this series: > > > > _rm_test_img > > _cleanup_nbd > > _cleanup_vxhs > > _cleanup_rbd > > _cleanup_sheepdog > > _cleanup_protocols > > _cleanup_test_img > > > > > > They all have a common theme (cleanup), so I could move them all to a > > common.cleanup (naming suggestion?) file (which would need to be included by > > common.rc, as well). > > > > Would this be a strong enough delineation to overcome your concerns? > > A great start. Which of these are actually needed by the tests (and > hence by common.rc) and why? > Some tests are written such that they do intermediate cleanups between multiple internal sub-tests for varying reasons, and so use those cleanup functions as part of their testing. The function _cleanup_test_img effectively calls all the other functions I listed, so they are really all required for the tests, if they choose to call _cleanup_test_img. And for 'check' to tear everything down to a clean state, it also needs to use the cleanup functions for everything that is not just a file/directory. -Jeff
On 18/10/2017 18:19, Jeff Cody wrote: >>> Here is what we need from common.rc for this series: >>> >>> _rm_test_img >>> _cleanup_nbd >>> _cleanup_vxhs >>> _cleanup_rbd >>> _cleanup_sheepdog >>> _cleanup_protocols >>> _cleanup_test_img >>> >>> They all have a common theme (cleanup), so I could move them all to a >>> common.cleanup (naming suggestion?) file (which would need to be included by >>> common.rc, as well). >>> >>> Would this be a strong enough delineation to overcome your concerns? >> >> A great start. Which of these are actually needed by the tests (and >> hence by common.rc) and why? > > Some tests are written such that they do intermediate cleanups between > multiple internal sub-tests for varying reasons, and so use those cleanup > functions as part of their testing. The function _cleanup_test_img > effectively calls all the other functions I listed, so they are really all > required for the tests, if they choose to call _cleanup_test_img. > > And for 'check' to tear everything down to a clean state, it also needs to > use the cleanup functions for everything that is not just a file/directory. Do these tests really need the "cleanup protocols" part, because the few that have more than one _cleanup_test_img (059, 066, 070, 084, 146, 171) are either file-only or non-raw, so they should be able to just rebuild the format on top of the same image. Maybe that's where the separation lies---protocol vs. format, where cleaning up the "file" protocol need not do anything because it's done when removing the test directory. If that's the case, it'd be nice because it might also make it much easier to tackle the issue with parallel tests. Paolo
On Wed, Oct 18, 2017 at 06:39:26PM +0200, Paolo Bonzini wrote: > On 18/10/2017 18:19, Jeff Cody wrote: > >>> Here is what we need from common.rc for this series: > >>> > >>> _rm_test_img > >>> _cleanup_nbd > >>> _cleanup_vxhs > >>> _cleanup_rbd > >>> _cleanup_sheepdog > >>> _cleanup_protocols > >>> _cleanup_test_img > >>> > >>> They all have a common theme (cleanup), so I could move them all to a > >>> common.cleanup (naming suggestion?) file (which would need to be included by > >>> common.rc, as well). > >>> > >>> Would this be a strong enough delineation to overcome your concerns? > >> > >> A great start. Which of these are actually needed by the tests (and > >> hence by common.rc) and why? > > > > Some tests are written such that they do intermediate cleanups between > > multiple internal sub-tests for varying reasons, and so use those cleanup > > functions as part of their testing. The function _cleanup_test_img > > effectively calls all the other functions I listed, so they are really all > > required for the tests, if they choose to call _cleanup_test_img. > > > > And for 'check' to tear everything down to a clean state, it also needs to > > use the cleanup functions for everything that is not just a file/directory. > > Do these tests really need the "cleanup protocols" part, because the few > that have more than one _cleanup_test_img (059, 066, 070, 084, 146, 171) > are either file-only or non-raw, so they should be able to just rebuild > the format on top of the same image. > Maybe not, but that could just be the nature of what bugs we are testing for at this time. These specific tests may not need to, but it is not inconceivable to have a test that involves bringing up and tearing down a protocol based server some arbitrary number times, to test that specific behavior. > Maybe that's where the separation lies---protocol vs. format, where > cleaning up the "file" protocol need not do anything because it's done > when removing the test directory. If that's the case, it'd be nice > because it might also make it much easier to tackle the issue with > parallel tests. > 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.
On 18/10/2017 19:27, Jeff Cody wrote: > On Wed, Oct 18, 2017 at 06:39:26PM +0200, Paolo Bonzini wrote: >> On 18/10/2017 18:19, Jeff Cody wrote: >>>>> Here is what we need from common.rc for this series: >>>>> >>>>> _rm_test_img >>>>> _cleanup_nbd >>>>> _cleanup_vxhs >>>>> _cleanup_rbd >>>>> _cleanup_sheepdog >>>>> _cleanup_protocols >>>>> _cleanup_test_img >>>>> >>>>> They all have a common theme (cleanup), so I could move them all to a >>>>> common.cleanup (naming suggestion?) file (which would need to be included by >>>>> common.rc, as well). >>>>> >>>>> Would this be a strong enough delineation to overcome your concerns? >>>> >>>> A great start. Which of these are actually needed by the tests (and >>>> hence by common.rc) and why? >>> >>> Some tests are written such that they do intermediate cleanups between >>> multiple internal sub-tests for varying reasons, and so use those cleanup >>> functions as part of their testing. The function _cleanup_test_img >>> effectively calls all the other functions I listed, so they are really all >>> required for the tests, if they choose to call _cleanup_test_img. >>> >>> And for 'check' to tear everything down to a clean state, it also needs to >>> use the cleanup functions for everything that is not just a file/directory. >> >> Do these tests really need the "cleanup protocols" part, because the few >> that have more than one _cleanup_test_img (059, 066, 070, 084, 146, 171) >> are either file-only or non-raw, so they should be able to just rebuild >> the format on top of the same image. >> > > Maybe not, but that could just be the nature of what bugs we are testing for > at this time. These specific tests may not need to, but it is not > inconceivable to have a test that involves bringing up and tearing down > a protocol based server some arbitrary number times, to test that specific > behavior. Right, but such a test should probably be protocol-specific; it can just use "file" and invoke QEMU_NBD on its own for example, similar to what tests 058 and 162 already do. For example, it's very different if you delete and recreate a raw image _and_ rerun qemu-nbd, or only do the latter. >> Maybe that's where the separation lies---protocol vs. format, where >> cleaning up the "file" protocol need not do anything because it's done >> when removing the test directory. If that's the case, it'd be nice >> because it might also make it much easier to tackle the issue with >> parallel tests. > > 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: - 083 and 143 for example are very much NBD-specific; 083 uses "_supported_proto nbd", while 143 probably should be using either "file" or "nbd". - only 058 and 162 are running qemu-nbd manually, and they actually start a _new_ NBD protocol server In addition not many tests do not use _make_test_img, as seen with "git grep -LE '_make_test_img|bin/env python' [0-9][0-9][0-9]". They are: - 064, 070 and 135 which use the sample image and thus _should_ be using file - 049, 082, 111 which is creating images with "qemu-img create"; 049 and 111 might actually use nfs too. 150 is using "qemu-img convert", which is also creation more or less - 083 is NBD-specific because it uses the fault injector - 113 probably should be more generic than just NBD - 128 is pretty unique in that it creates a Linux device mapper device (!) - 143 probably should be using either "file" or "nbd". - 162 is also a bit unique in that it tests null-co:// and disregards IMGFMT/IMGPROTO So it does seem that handling the protocol in "check" is a good approximation. And by the way, the only existing uses of "_supported_proto" are _supported_proto file _supported_proto file nfs _supported_proto file sheepdog rbd nfs _supported_proto generic _supported_proto nbd _supported_proto nfs so another thing to do might be to change _supported_proto to a feature-based test: - file/sheepdog/rbd/nfs are those that support resizing (aka "truncate") - file/nfs are generally those that support "qemu-img create" (plus others that should be "generic" actually, including 090, 103, 107); 150 is "file" because it needs "map" in addition to "create". - nfs is used in test 173 to test protocol-based images with relative filename backing strings; it probably can run on file too, anyway that's a third important discriminating feature - nbd is used in a bunch of random tests that can be made more generic (094, 113, 119, 123). Only 083 is NBD-specific because it uses the NBD fault injector, and it actually doesn't use the protocol that is created by the caller. Since you are doing a lot of whole-directory moves, "_supported" tags could become a separate configuration file, e.g. ---- # all generic, no need to include it #[001] [025] fmt=raw qcow2 qed # specify feature proto=+resize [143] fmt=generic proto=nbd start_protocol=no [150] fmt=raw qcow2 proto=+create +map ---- The few tests using start_protocol=no would have to ensure parallel-safe operation themselves. This would also enable a more consistent handling across shell and Python tests. So, this is why I was wondering whether patches 3/4 kinda paint ourselves in the corner. 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". - 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 (Python can do the same by setting preexec_fn=os.setpgrp when calling Subprocess.popen; then you can kill the entire group with os.killpg) - 10 depends on everything before. SAD! ;) It's a pity to lose the cleanup in patch 4, but I think it's better in the long term if we have a clear idea of the tests' tasks vs. the harness's. Thanks, Paolo
On Thu, Oct 19, 2017 at 12:23:39PM +0200, Paolo Bonzini wrote: > On 18/10/2017 19:27, Jeff Cody wrote: > > On Wed, Oct 18, 2017 at 06:39:26PM +0200, Paolo Bonzini wrote: > >> On 18/10/2017 18:19, Jeff Cody wrote: > >>>>> Here is what we need from common.rc for this series: > >>>>> > >>>>> _rm_test_img > >>>>> _cleanup_nbd > >>>>> _cleanup_vxhs > >>>>> _cleanup_rbd > >>>>> _cleanup_sheepdog > >>>>> _cleanup_protocols > >>>>> _cleanup_test_img > >>>>> > >>>>> They all have a common theme (cleanup), so I could move them all to a > >>>>> common.cleanup (naming suggestion?) file (which would need to be included by > >>>>> common.rc, as well). > >>>>> > >>>>> Would this be a strong enough delineation to overcome your concerns? > >>>> > >>>> A great start. Which of these are actually needed by the tests (and > >>>> hence by common.rc) and why? > >>> > >>> Some tests are written such that they do intermediate cleanups between > >>> multiple internal sub-tests for varying reasons, and so use those cleanup > >>> functions as part of their testing. The function _cleanup_test_img > >>> effectively calls all the other functions I listed, so they are really all > >>> required for the tests, if they choose to call _cleanup_test_img. > >>> > >>> And for 'check' to tear everything down to a clean state, it also needs to > >>> use the cleanup functions for everything that is not just a file/directory. > >> > >> Do these tests really need the "cleanup protocols" part, because the few > >> that have more than one _cleanup_test_img (059, 066, 070, 084, 146, 171) > >> are either file-only or non-raw, so they should be able to just rebuild > >> the format on top of the same image. > >> > > > > Maybe not, but that could just be the nature of what bugs we are testing for > > at this time. These specific tests may not need to, but it is not > > inconceivable to have a test that involves bringing up and tearing down > > a protocol based server some arbitrary number times, to test that specific > > behavior. > > Right, but such a test should probably be protocol-specific; it can just > use "file" and invoke QEMU_NBD on its own for example, similar to what > tests 058 and 162 already do. > > For example, it's very different if you delete and recreate a raw image > _and_ rerun qemu-nbd, or only do the latter. > > >> Maybe that's where the separation lies---protocol vs. format, where > >> cleaning up the "file" protocol need not do anything because it's done > >> when removing the test directory. If that's the case, it'd be nice > >> because it might also make it much easier to tackle the issue with > >> parallel tests. > > > > 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. > - 083 and 143 for example are very much NBD-specific; 083 uses > "_supported_proto nbd", while 143 probably should be using either "file" > or "nbd". > > - only 058 and 162 are running qemu-nbd manually, and they actually > start a _new_ NBD protocol server > > > In addition not many tests do not use _make_test_img, as seen with "git > grep -LE '_make_test_img|bin/env python' [0-9][0-9][0-9]". They are: > > - 064, 070 and 135 which use the sample image and thus _should_ be using > file > > - 049, 082, 111 which is creating images with "qemu-img create"; 049 and > 111 might actually use nfs too. 150 is using "qemu-img convert", which > is also creation more or less > > - 083 is NBD-specific because it uses the fault injector > > - 113 probably should be more generic than just NBD > > - 128 is pretty unique in that it creates a Linux device mapper device (!) > > - 143 probably should be using either "file" or "nbd". > > - 162 is also a bit unique in that it tests null-co:// and disregards > IMGFMT/IMGPROTO > > > So it does seem that handling the protocol in "check" is a good > approximation. > > And by the way, the only existing uses of "_supported_proto" are > > _supported_proto file > _supported_proto file nfs > _supported_proto file sheepdog rbd nfs > _supported_proto generic > _supported_proto nbd > _supported_proto nfs > > so another thing to do might be to change _supported_proto to a > feature-based test: > > - file/sheepdog/rbd/nfs are those that support resizing (aka "truncate") > > - file/nfs are generally those that support "qemu-img create" (plus > others that should be "generic" actually, including 090, 103, 107); 150 > is "file" because it needs "map" in addition to "create". > > - nfs is used in test 173 to test protocol-based images with relative > filename backing strings; it probably can run on file too, anyway that's > a third important discriminating feature > > - nbd is used in a bunch of random tests that can be made more generic > (094, 113, 119, 123). Only 083 is NBD-specific because it uses the NBD > fault injector, and it actually doesn't use the protocol that is created > by the caller. > > > Since you are doing a lot of whole-directory moves, "_supported" tags > could become a separate configuration file, e.g. > > ---- > # all generic, no need to include it > #[001] > > [025] > fmt=raw qcow2 qed > # specify feature > proto=+resize > > [143] > fmt=generic > proto=nbd > start_protocol=no > > [150] > fmt=raw qcow2 > proto=+create +map > ---- > > The few tests using start_protocol=no would have to ensure parallel-safe > operation themselves. This would also enable a more consistent handling > across shell and Python tests. > > 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. I'm advocating that this series keep the same test APIs intact, and remove the need for tests to perform exit cleanup. (Patch 10 is just some icing, since patches 1-9 make it fairly easy to add) 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. 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] > 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. > (Python can do the same by setting preexec_fn=os.setpgrp when calling > Subprocess.popen; then you can kill the entire group with os.killpg) > Yeah - a new check rewrite in a language like python would make process control significantly easier. > - 10 depends on everything before. SAD! ;) > > It's a pity to lose the cleanup in patch 4, but I think it's better in > the long term if we have a clear idea of the tests' tasks vs. the harness's. > This is the crux of it all, I suppose. [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". Jeff
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
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check index 5ae34bf..b4ab646 100755 --- a/tests/qemu-iotests/check +++ b/tests/qemu-iotests/check @@ -757,6 +757,8 @@ do fi export OUTPUT_DIR=$PWD if $debug; then + # Do this in a sub-shell, so we are operating on the right + # TEST_DIR / QEMU_TEST_DIR ( export TEST_DIR=$TEST_DIR_SEQ cd "$source_iotests"; @@ -766,6 +768,8 @@ do $run_command -d 2>&1 | tee $tmp.out ) else + # Do this in a sub-shell, so we are operating on the right + # TEST_DIR / QEMU_TEST_DIR ( export TEST_DIR=$TEST_DIR_SEQ cd "$source_iotests"; @@ -837,6 +841,15 @@ do fi fi + # Do this in a sub-shell, so we are operating on the right + # TEST_DIR / QEMU_TEST_DIR + ( + export TEST_DIR=$TEST_DIR_SEQ + . "$source_iotests/common.config" + . "$source_iotests/common.rc" + + _cleanup_protocols + ) rm -rf "$TEST_DIR_SEQ" fi diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index 0e8a33c..a345ffd 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -101,7 +101,7 @@ _qemu_nbd_wrapper() _qemu_vxhs_wrapper() { ( - echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid" + echo $BASHPID > "${QEMU_TEST_DIR}/qemu-vxhs.pid" exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@" ) } @@ -248,7 +248,7 @@ _make_test_img() # Start QNIO server on image directory for vxhs protocol if [ $IMGPROTO = "vxhs" ]; then - eval "$QEMU_VXHS -d $TEST_DIR > /dev/null &" + eval "$QEMU_VXHS -d $QEMU_TEST_DIR > /dev/null &" sleep 1 # Wait for server to come up. fi } @@ -264,29 +264,64 @@ _rm_test_img() rm -f "$img" } +_cleanup_nbd() +{ + if [ -f "${QEMU_TEST_DIR}/qemu-nbd.pid" ]; then + local QEMU_NBD_PID + read QEMU_NBD_PID < "${QEMU_TEST_DIR}/qemu-nbd.pid" + rm -f "${QEMU_TEST_DIR}/qemu-nbd.pid" + kill ${QEMU_NBD_PID} >/dev/null 2>&1 + fi +} + +_cleanup_vxhs() +{ + if [ -f "${QEMU_TEST_DIR}/qemu-vxhs.pid" ]; then + local QEMU_VXHS_PID + read QEMU_VXHS_PID < "${QEMU_TEST_DIR}/qemu-vxhs.pid" + rm -f "${QEMU_TEST_DIR}/qemu-vxhs.pid" + kill ${QEMU_VXHS_PID} >/dev/null 2>&1 + fi +} + +_cleanup_rbd() +{ + rbd --no-progress rm "$QEMU_TEST_DIR/t.$IMGFMT" > /dev/null +} + +_cleanup_sheepdog() +{ + collie vdi delete "$QEMU_TEST_DIR/t.$IMGFMT" +} + + +_cleanup_protocols() +{ + # Some tests (e.g. 058) start some protocols + # even though the protocol was not specified when running + # check. If the wrappers create pidfiles, go ahead and clean + # up without checking $IMGPROTO. + _cleanup_nbd + _cleanup_vxhs + + case "$IMGPROTO" in + + rbd) + _cleanup_rbd + ;; + + sheepdog) + _cleanup_sheepdog + ;; + + esac +} + _cleanup_test_img() { + _cleanup_protocols + case "$IMGPROTO" in - - nbd) - if [ -f "${QEMU_TEST_DIR}/qemu-nbd.pid" ]; then - local QEMU_NBD_PID - read QEMU_NBD_PID < "${QEMU_TEST_DIR}/qemu-nbd.pid" - kill ${QEMU_NBD_PID} - rm -f "${QEMU_TEST_DIR}/qemu-nbd.pid" - fi - rm -f "$TEST_IMG_FILE" - ;; - vxhs) - if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then - local QEMU_VXHS_PID - read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid" - kill ${QEMU_VXHS_PID} >/dev/null 2>&1 - rm -f "${TEST_DIR}/qemu-vxhs.pid" - fi - rm -f "$TEST_IMG_FILE" - ;; - file) _rm_test_img "$TEST_DIR/t.$IMGFMT" _rm_test_img "$TEST_DIR/t.$IMGFMT.orig" @@ -298,16 +333,12 @@ _cleanup_test_img() TEST_IMG="$ORIG_TEST_IMG" fi ;; - - rbd) - rbd --no-progress rm "$TEST_DIR/t.$IMGFMT" > /dev/null - ;; - - sheepdog) - collie vdi delete "$TEST_DIR/t.$IMGFMT" - ;; - esac + + if [ -n "$TEST_IMG_FILE" ] + then + rm -f "$TEST_IMG_FILE" + fi } _check_test_img()
For bash tests, this allows 'check' to reap all launch protocol servers / processes, after a test has finished running. Signed-off-by: Jeff Cody <jcody@redhat.com> --- tests/qemu-iotests/check | 13 +++++++ tests/qemu-iotests/common.rc | 93 +++++++++++++++++++++++++++++--------------- 2 files changed, 75 insertions(+), 31 deletions(-)