| Message ID | 20240402100322.17673-1-dwagner@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | add blkdev type environment variable | expand |
On Tue, Apr 02, 2024 at 12:03:19PM +0200, Daniel Wagner wrote:
> The nvme/028 test with file backend is failing in my setup
^^^^^^^^^^^^^
block device backend
On Apr 02, 2024 / 12:03, Daniel Wagner wrote: > During the last batch of refactoring I noticed that we could reduce the number > of tests a bit. There are tests which are almost identically except how the > target is configured, file vs block device backend. > > By introducing a configure knob, we can drop the duplicates and even make some > of the tests a bit more versatile. Not all tests exists in file and block device > backend version. Thus we increase the test coverage with this series. And not > really surprising, there is a fallout. The nvme/028 test with file backend is > failing in my setup but I was not able to figure out where things go wrong yet. > I'll provide some logs later. Hi Daniel, I find this series reduces code duplications further, and it expands test coverage with small test script changes. Good. On the other hand, I see that the series has a couple of drawbacks: 1) When blktests users run with the default knob only, the test coverage will be smaller. To keep the current test coverage, the users need to run the check script twice: nvmet_blkdev_type=file and nvmet_blkdev_type=device. Some users may not do it and lose the test coverage. And some users, e.g., CKI project, need to adjust their script for this change. 2) When the users run the check script twice to keep the test coverage, some test cases are executed twice under the exact same test conditions. This will waste some of the test run effort. To avoid the drawbacks, how about this? - Do not provide nvmet_blkdev_type as a knob for users. Keep it as just a global variable in tests/nvme/rc. (It should be renamed to clarify that it is not for users.) - Introduce a helper function to do the same test twice for nvmet_blkdev_type= file and nvmet_blkdev_type=device. Call this helper function from a single test case to cover both the blkdev types. I attach a patch at the end of this email to show the ideas above. It applies the idea to nvme/006 as an example. What do you think? diff --git a/tests/nvme/006 b/tests/nvme/006 index 65f2a0d..6241961 100755 --- a/tests/nvme/006 +++ b/tests/nvme/006 @@ -15,14 +15,18 @@ requires() { _require_nvme_trtype_is_fabrics } -test() { - echo "Running ${TEST_NAME}" - +do_test() { _setup_nvmet _nvmet_target_setup _nvmet_target_cleanup +} + +test() { + echo "Running ${TEST_NAME}" + + _nvmet_run_for_each_blkdev_type do_test echo "Test complete" } diff --git a/tests/nvme/rc b/tests/nvme/rc index 5fa1871..4155411 100644 --- a/tests/nvme/rc +++ b/tests/nvme/rc @@ -996,6 +996,15 @@ _nvmet_passthru_target_cleanup() { _remove_nvmet_host "${def_hostnqn}" } +_nvmet_run_for_each_blkdev_type() { + local blkdev_type + + for blkdev_type in device file; do + nvmet_blkdev_type=$blkdev_type + eval "$1" + done +} + _discovery_genctr() { _nvme_discover "${nvme_trtype}" | sed -n -e 's/^.*Generation counter \([0-9]\+\).*$/\1/p'
Hi Shinichiro, On Wed, Apr 03, 2024 at 04:54:28AM +0000, Shinichiro Kawasaki wrote: > On the other hand, I see that the series has a couple of drawbacks: > > 1) When blktests users run with the default knob only, the test coverage will be > smaller. To keep the current test coverage, the users need to run the check > script twice: nvmet_blkdev_type=file and nvmet_blkdev_type=device. Some users > may not do it and lose the test coverage. And some users, e.g., CKI project, > need to adjust their script for this change. > > 2) When the users run the check script twice to keep the test coverage, some > test cases are executed twice under the exact same test conditions. This > will waste some of the test run effort. Yes, I agree. These drawbacks should be addressed somehow. > To avoid the drawbacks, how about this? > > - Do not provide nvmet_blkdev_type as a knob for users. Keep it as just a global > variable in tests/nvme/rc. (It should be renamed to clarify that it is not for > users.) > > - Introduce a helper function to do the same test twice for nvmet_blkdev_type= > file and nvmet_blkdev_type=device. Call this helper function from a single > test case to cover both the blkdev types. > > I attach a patch at the end of this email to show the ideas above. It applies > the idea to nvme/006 as an example. What do you think? Ideally we don't have to introduce additional common setup logic into each test. Also for debugging purpose it might sense to run a test only in one configuration. So it might make sense still to have user visible environment variable nvmet_blkdev_types="file device" as default. > -test() { > - echo "Running ${TEST_NAME}" > - > +do_test() { > _setup_nvmet > > _nvmet_target_setup > > _nvmet_target_cleanup > +} > + > +test() { > + echo "Running ${TEST_NAME}" > + > + _nvmet_run_for_each_blkdev_type do_test I was wondering if the nvmet_run_for_each_blkdev_type logic could be in common code, so we don't have to add a do_test function. Basically having a common code for a bunch of configuration variables (matrix tests). This could also be useful for nvmet_trtype etc. The generic setup could be requested via the require hook. requires() = { _nvmet_setup_target } What do you think about this idea? Thanks, Daniel
Shinichiro/Daniel, On 4/3/24 02:00, Daniel Wagner wrote: > Hi Shinichiro, > > On Wed, Apr 03, 2024 at 04:54:28AM +0000, Shinichiro Kawasaki wrote: >> On the other hand, I see that the series has a couple of drawbacks: >> >> 1) When blktests users run with the default knob only, the test coverage will be >> smaller. To keep the current test coverage, the users need to run the check >> script twice: nvmet_blkdev_type=file and nvmet_blkdev_type=device. Some users >> may not do it and lose the test coverage. And some users, e.g., CKI project, >> need to adjust their script for this change. reducing code is always encouraged, but please don't change the test coverage and make user add additional steps to run the tests, IOW ./check nvme should run all the tests case by default as it dose today, this will keep your changes backward compatible .. I believe that is not too hard to achieve ? -ck
On Apr 03, 2024 / 11:00, Daniel Wagner wrote: > Hi Shinichiro, > > On Wed, Apr 03, 2024 at 04:54:28AM +0000, Shinichiro Kawasaki wrote: > > On the other hand, I see that the series has a couple of drawbacks: > > > > 1) When blktests users run with the default knob only, the test coverage will be > > smaller. To keep the current test coverage, the users need to run the check > > script twice: nvmet_blkdev_type=file and nvmet_blkdev_type=device. Some users > > may not do it and lose the test coverage. And some users, e.g., CKI project, > > need to adjust their script for this change. > > > > 2) When the users run the check script twice to keep the test coverage, some > > test cases are executed twice under the exact same test conditions. This > > will waste some of the test run effort. > > Yes, I agree. These drawbacks should be addressed somehow. > > > To avoid the drawbacks, how about this? > > > > - Do not provide nvmet_blkdev_type as a knob for users. Keep it as just a global > > variable in tests/nvme/rc. (It should be renamed to clarify that it is not for > > users.) > > > > - Introduce a helper function to do the same test twice for nvmet_blkdev_type= > > file and nvmet_blkdev_type=device. Call this helper function from a single > > test case to cover both the blkdev types. > > > > I attach a patch at the end of this email to show the ideas above. It applies > > the idea to nvme/006 as an example. What do you think? > > Ideally we don't have to introduce additional common setup logic into > each test. Also for debugging purpose it might sense to run a test only > in one configuration. So it might make sense still to have user visible > environment variable > > nvmet_blkdev_types="file device" > > as default. > > > -test() { > > - echo "Running ${TEST_NAME}" > > - > > +do_test() { > > _setup_nvmet > > > > _nvmet_target_setup > > > > _nvmet_target_cleanup > > +} > > + > > +test() { > > + echo "Running ${TEST_NAME}" > > + > > + _nvmet_run_for_each_blkdev_type do_test > > I was wondering if the nvmet_run_for_each_blkdev_type logic could be in > common code, so we don't have to add a do_test function. Basically > having a common code for a bunch of configuration variables (matrix > tests). Actually, similar feature is implemented in the common code so that some test case can be run twice, once for regular the block device, and one more time for the zoned block device. You can find test cases with CAN_BE_ZONED=1 flag. They are run twice when RUN_ZONED_TESTS is set in the config. To be precise, this applies to the test cases with test() function. CAN_BE_ZONED has different meaning for test cases with test_device(). Now we want to run some of test cases twice for the two nvmet block device types. This is essentially common feature as the repeated runs for the CAN_BE_ZONED test cases. I think it's time to generalize these two uses cases and support "repeated test case runs with different test conditions". > This could also be useful for nvmet_trtype etc. Yes, when I run for all of nvmet_trtype variations, I see some test cases are repeated with same condition. Waste of time. > > The generic setup could be requested via the require hook. > > requires() = { > > _nvmet_setup_target > } Hmm, I think this abuses the hook. IMO, it's the better to introduce a new hook. > > What do you think about this idea? It sounds an interesting idea :) I prototyped the common code change based on the idea and shared it on GitHub [*]. It introduces two new config arrays NVMET_BLKDEV_TYPES and NVMET_TR_TYPES. When these two are set in config file as follows, NVMET_BLKDEV_TYPES=(device file) NVMET_TR_TYPES=(loop rdma tcp) it will run a single test case as follows. 2 x 3 = 6 times repeptitions. $ sudo ./check nvme/006 nvme/006(nvmet dev=device tr=loop)(create an NVMeOF target) [passed] runtime 0.090s ... 0.091s nvme/006(nvmet dev=device tr=rdma)(create an NVMeOF target) [passed] runtime 0.310s ... 0.305s nvme/006(nvmet dev=device tr=tcp)(create an NVMeOF target) [passed] runtime 0.149s ... 0.153s nvme/006(nvmet dev=file tr=loop)(create an NVMeOF target) [passed] runtime 0.138s ... 0.135s nvme/006(nvmet dev=file tr=rdma)(create an NVMeOF target) [passed] runtime 0.300s ... 0.305s nvme/006(nvmet dev=file tr=tcp)(create an NVMeOF target) [passed] runtime 0.141s ... 0.147s I hope this meets your needs. [*] https://github.com/kawasaki/blktests/tree/conditions
On Wed, Apr 03, 2024 at 10:27:48PM +0000, Chaitanya Kulkarni wrote: > reducing code is always encouraged, but please don't change the test > coverage and This series increases the test coverage. > make user add additional steps to run the tests, IOW ./check nvme should > run all > the tests case by default as it dose today, this will keep your changes > backward > compatible .. Well, that is not really true right now. E.g. nvme_trtype is loop by default. And you need to specify the additional transports manually. This is also why I suggest we think about some sort of generic way to do this types of 'matrix configuration'(*) out of the box. (* don't know how to name this better)
On Thu, Apr 04, 2024 at 07:43:28AM +0000, Shinichiro Kawasaki wrote: > Actually, similar feature is implemented in the common code so that some > test case can be run twice, once for regular the block device, and one more > time for the zoned block device. You can find test cases with CAN_BE_ZONED=1 > flag. They are run twice when RUN_ZONED_TESTS is set in the config. > > To be precise, this applies to the test cases with test() function. > CAN_BE_ZONED has different meaning for test cases with test_device(). > > Now we want to run some of test cases twice for the two nvmet block device > types. This is essentially common feature as the repeated runs for the > CAN_BE_ZONED test cases. I think it's time to generalize these two uses cases > and support "repeated test case runs with different test conditions". Sounds reasonable. > > requires() = { > > > > _nvmet_setup_target > > } > > Hmm, I think this abuses the hook. IMO, it's the better to introduce a new hook. Indeed :) > > > > What do you think about this idea? > > It sounds an interesting idea :) I prototyped the common code change based on > the idea and shared it on GitHub [*]. It introduces two new config arrays > NVMET_BLKDEV_TYPES and NVMET_TR_TYPES. When these two are set in config file as > follows, > > NVMET_BLKDEV_TYPES=(device file) > NVMET_TR_TYPES=(loop rdma tcp) > > it will run a single test case as follows. 2 x 3 = 6 times repeptitions. > > $ sudo ./check nvme/006 > nvme/006(nvmet dev=device tr=loop)(create an NVMeOF target) [passed] > runtime 0.090s ... 0.091s > nvme/006(nvmet dev=device tr=rdma)(create an NVMeOF target) [passed] > runtime 0.310s ... 0.305s > nvme/006(nvmet dev=device tr=tcp)(create an NVMeOF target) [passed] > runtime 0.149s ... 0.153s > nvme/006(nvmet dev=file tr=loop)(create an NVMeOF target) [passed] > runtime 0.138s ... 0.135s > nvme/006(nvmet dev=file tr=rdma)(create an NVMeOF target) [passed] > runtime 0.300s ... 0.305s > nvme/006(nvmet dev=file tr=tcp)(create an NVMeOF target) [passed] > runtime 0.141s ... 0.147s > > I hope this meets your needs. Yes, this is very useful. > [*] https://github.com/kawasaki/blktests/tree/conditions I quickly looked into the changes. The only thing I'd say it looks a bit hard to extend if we have yet another variable. But maybe I'd make it too complex. I don't think we have to be too future proof here, because we can change this part without problems, it is all 'under the hood' and doesn't change the 'user interface'. Great stuff!
On Apr 04, 2024 / 10:29, Daniel Wagner wrote: > On Thu, Apr 04, 2024 at 07:43:28AM +0000, Shinichiro Kawasaki wrote: ... > > It sounds an interesting idea :) I prototyped the common code change based on > > the idea and shared it on GitHub [*]. It introduces two new config arrays > > NVMET_BLKDEV_TYPES and NVMET_TR_TYPES. When these two are set in config file as > > follows, > > > > NVMET_BLKDEV_TYPES=(device file) > > NVMET_TR_TYPES=(loop rdma tcp) > > > > it will run a single test case as follows. 2 x 3 = 6 times repeptitions. > > > > $ sudo ./check nvme/006 > > nvme/006(nvmet dev=device tr=loop)(create an NVMeOF target) [passed] > > runtime 0.090s ... 0.091s > > nvme/006(nvmet dev=device tr=rdma)(create an NVMeOF target) [passed] > > runtime 0.310s ... 0.305s > > nvme/006(nvmet dev=device tr=tcp)(create an NVMeOF target) [passed] > > runtime 0.149s ... 0.153s > > nvme/006(nvmet dev=file tr=loop)(create an NVMeOF target) [passed] > > runtime 0.138s ... 0.135s > > nvme/006(nvmet dev=file tr=rdma)(create an NVMeOF target) [passed] > > runtime 0.300s ... 0.305s > > nvme/006(nvmet dev=file tr=tcp)(create an NVMeOF target) [passed] > > runtime 0.141s ... 0.147s > > > > I hope this meets your needs. > > Yes, this is very useful. > > > [*] https://github.com/kawasaki/blktests/tree/conditions > > I quickly looked into the changes. The only thing I'd say it looks a > bit hard to extend if we have yet another variable. But maybe I'd make > it too complex. I don't think we have to be too future proof here, > because we can change this part without problems, it is all 'under the > hood' and doesn't change the 'user interface'. > > Great stuff! Okay, thanks for the positive comment. I will brush up the patches and post for review. Let me have several days.
During the last batch of refactoring I noticed that we could reduce the number of tests a bit. There are tests which are almost identically except how the target is configured, file vs block device backend. By introducing a configure knob, we can drop the duplicates and even make some of the tests a bit more versatile. Not all tests exists in file and block device backend version. Thus we increase the test coverage with this series. And not really surprising, there is a fallout. The nvme/028 test with file backend is failing in my setup but I was not able to figure out where things go wrong yet. I'll provide some logs later. Daniel Wagner (3): nvme/rc: add blkdev type environment variable nvme/{007,009,011,013,015,020,024}: drop duplicate nvmet blkdev type tests nvme/{021,022,025,026,027,028}: do not hard code target blkdev type Documentation/running-tests.md | 2 ++ tests/nvme/006 | 5 ++-- tests/nvme/007 | 28 -------------------- tests/nvme/007.out | 2 -- tests/nvme/008 | 4 +-- tests/nvme/009 | 36 ------------------------- tests/nvme/009.out | 3 --- tests/nvme/010 | 4 +-- tests/nvme/011 | 39 --------------------------- tests/nvme/011.out | 3 --- tests/nvme/012 | 4 +-- tests/nvme/013 | 43 ------------------------------ tests/nvme/013.out | 3 --- tests/nvme/014 | 4 +-- tests/nvme/015 | 48 ---------------------------------- tests/nvme/015.out | 4 --- tests/nvme/019 | 4 +-- tests/nvme/020 | 40 ---------------------------- tests/nvme/020.out | 4 --- tests/nvme/021 | 6 ++--- tests/nvme/022 | 6 ++--- tests/nvme/023 | 4 +-- tests/nvme/024 | 40 ---------------------------- tests/nvme/024.out | 2 -- tests/nvme/025 | 6 ++--- tests/nvme/026 | 6 ++--- tests/nvme/027 | 6 ++--- tests/nvme/028 | 6 ++--- tests/nvme/rc | 3 ++- 29 files changed, 36 insertions(+), 329 deletions(-) delete mode 100755 tests/nvme/007 delete mode 100644 tests/nvme/007.out delete mode 100755 tests/nvme/009 delete mode 100644 tests/nvme/009.out delete mode 100755 tests/nvme/011 delete mode 100644 tests/nvme/011.out delete mode 100755 tests/nvme/013 delete mode 100644 tests/nvme/013.out delete mode 100755 tests/nvme/015 delete mode 100644 tests/nvme/015.out delete mode 100755 tests/nvme/020 delete mode 100644 tests/nvme/020.out delete mode 100755 tests/nvme/024 delete mode 100644 tests/nvme/024.out