| Message ID | 20240411111228.2290407-6-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | support test case repeat by different conditions | expand |
On Thu, Apr 11, 2024 at 08:12:22PM +0900, Shin'ichiro Kawasaki wrote: > Some of the test cases in nvme test group can be run under various nvme > target transport types. The configuration parameter nvme_trtype > specifies the transport to use. But this configuration method has two > drawbacks. Firstly, the blktests check script needs to be invoked > multiple times to cover multiple transport types. Secondly, the test > cases irrelevant to the transport types are executed exactly same > conditions in the multiple blktests runs. > > To avoid the drawbacks, introduce new configuration parameter > NVMET_TR_TYPES. This is an array, and multiple transport types can > be set like: > > NVMET_TR_TYPES=(loop tcp) > > Also introduce _nvmet_set_nvme_trtype() which can be called from the > set_conditions() hook of the transport type dependent test cases. > Blktests will repeat the test case as many as the number of elements in > NVMET_TR_TYPES, and set nvme_trtype for each test case run. I suggest to keep the naming of the variable consistent, e.g. NVMET_TRTYPES.
On Apr 11, 2024 / 20:41, Daniel Wagner wrote: > On Thu, Apr 11, 2024 at 08:12:22PM +0900, Shin'ichiro Kawasaki wrote: > > Some of the test cases in nvme test group can be run under various nvme > > target transport types. The configuration parameter nvme_trtype > > specifies the transport to use. But this configuration method has two > > drawbacks. Firstly, the blktests check script needs to be invoked > > multiple times to cover multiple transport types. Secondly, the test > > cases irrelevant to the transport types are executed exactly same > > conditions in the multiple blktests runs. > > > > To avoid the drawbacks, introduce new configuration parameter > > NVMET_TR_TYPES. This is an array, and multiple transport types can > > be set like: > > > > NVMET_TR_TYPES=(loop tcp) > > > > Also introduce _nvmet_set_nvme_trtype() which can be called from the > > set_conditions() hook of the transport type dependent test cases. > > Blktests will repeat the test case as many as the number of elements in > > NVMET_TR_TYPES, and set nvme_trtype for each test case run. > > I suggest to keep the naming of the variable consistent, e.g. > NVMET_TRTYPES. I see. I can think of NVME_TRTYPES and NVMET_TRTYPES, and I'm fine with both. If there is no other voice, I will rename to NVMET_TRTYPES.
On Apr 11, 2024 / 20:12, Shin'ichiro Kawasaki wrote: > Some of the test cases in nvme test group can be run under various nvme > target transport types. The configuration parameter nvme_trtype > specifies the transport to use. But this configuration method has two > drawbacks. Firstly, the blktests check script needs to be invoked > multiple times to cover multiple transport types. Secondly, the test > cases irrelevant to the transport types are executed exactly same > conditions in the multiple blktests runs. > > To avoid the drawbacks, introduce new configuration parameter > NVMET_TR_TYPES. This is an array, and multiple transport types can > be set like: > > NVMET_TR_TYPES=(loop tcp) > > Also introduce _nvmet_set_nvme_trtype() which can be called from the > set_conditions() hook of the transport type dependent test cases. > Blktests will repeat the test case as many as the number of elements in > NVMET_TR_TYPES, and set nvme_trtype for each test case run. > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > Documentation/running-tests.md | 6 +++++- > tests/nvme/rc | 30 +++++++++++++++++++++++++++++- > 2 files changed, 34 insertions(+), 2 deletions(-) > > diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md > index ae80860..ede3a81 100644 > --- a/Documentation/running-tests.md > +++ b/Documentation/running-tests.md > @@ -102,8 +102,12 @@ RUN_ZONED_TESTS=1 > > The NVMe tests can be additionally parameterized via environment variables. > > +- NVMET_TR_TYPES (array) > + Set up NVME target backends with the specified transport. > + Valid elements are 'loop', 'tcp', 'rdma' and 'fc'. Default value is '(loop)'. > - nvme_trtype: 'loop' (default), 'tcp', 'rdma' and 'fc' > - Run the tests with the given transport. > + Run the tests with the given transport. This parameter is still usable but > + replaced with NVMET_TR_TYPES. Use NVMET_TR_TYPES instead. I noticed that nvme_trtype is still useful. nvmet_trtypes can be set in both in the config file and the command line. But NVMET_TRTYPES can be set in the config file only, because bash does not support setting arrays in the command line. # nvme_trtypes=rdma ./check nvme/006 ... works # NVMET_TRTYPES=(rdma) ./check nvme/006 ... does not work I will modify the descriptions above in the v2 series to note that both nvme_trtype and NVMET_TRTYPES are supported and usable. > - nvme_img_size: '1G' (default) > Run the tests with given image size in bytes. 'm', 'M', 'g' > and 'G' postfix are supported. > diff --git a/tests/nvme/rc b/tests/nvme/rc > index 1f5ff44..df6bf77 100644 > --- a/tests/nvme/rc > +++ b/tests/nvme/rc > @@ -18,10 +18,38 @@ def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349" > def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}" > export def_subsysnqn="blktests-subsystem-1" > export def_subsys_uuid="91fdba0d-f87b-4c25-b80f-db7be1418b9e" > -nvme_trtype=${nvme_trtype:-"loop"} > nvme_img_size=${nvme_img_size:-"1G"} > nvme_num_iter=${nvme_num_iter:-"1000"} > > +# Check consistency of NVMET_TR_TYPES and nvme_trtype configurations. > +# If neither is configured, set the default value. > +first_call=${first_call:-1} > +if ((first_call)); then > + if [[ -n $nvme_trtype ]]; then > + if [[ -n $NVMET_TR_TYPES ]]; then > + echo "Both nvme_trtype and NVMET_TR_TYPES are specified" > + exit 1 > + fi > + NVMET_TR_TYPES=("$nvme_trtype") > + elif [[ -z ${NVMET_TR_TYPES[*]} ]]; then > + nvme_trtype="loop" > + NVMET_TR_TYPES=("$nvme_trtype") > + fi > + first_call=0 > +fi > + > +_set_nvme_trtype() { > + local index=$1 > + > + if [[ -z $index ]]; then > + echo ${#NVMET_TR_TYPES[@]} > + return > + fi > + > + nvme_trtype=${NVMET_TR_TYPES[index]} > + COND_DESC="nvmet tr=${nvme_trtype}" > +} > + > # TMPDIR can not be referred out of test() or test_device() context. Instead of > # global variable def_flie_path, use this getter function. > _nvme_def_file_path() { > -- > 2.44.0 >
On Apr 16, 2024 / 05:20, Shinichiro Kawasaki wrote: > On Apr 11, 2024 / 20:12, Shin'ichiro Kawasaki wrote: > > Some of the test cases in nvme test group can be run under various nvme > > target transport types. The configuration parameter nvme_trtype > > specifies the transport to use. But this configuration method has two > > drawbacks. Firstly, the blktests check script needs to be invoked > > multiple times to cover multiple transport types. Secondly, the test > > cases irrelevant to the transport types are executed exactly same > > conditions in the multiple blktests runs. > > > > To avoid the drawbacks, introduce new configuration parameter > > NVMET_TR_TYPES. This is an array, and multiple transport types can > > be set like: > > > > NVMET_TR_TYPES=(loop tcp) > > > > Also introduce _nvmet_set_nvme_trtype() which can be called from the > > set_conditions() hook of the transport type dependent test cases. > > Blktests will repeat the test case as many as the number of elements in > > NVMET_TR_TYPES, and set nvme_trtype for each test case run. > > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > --- > > Documentation/running-tests.md | 6 +++++- > > tests/nvme/rc | 30 +++++++++++++++++++++++++++++- > > 2 files changed, 34 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md > > index ae80860..ede3a81 100644 > > --- a/Documentation/running-tests.md > > +++ b/Documentation/running-tests.md > > @@ -102,8 +102,12 @@ RUN_ZONED_TESTS=1 > > > > The NVMe tests can be additionally parameterized via environment variables. > > > > +- NVMET_TR_TYPES (array) > > + Set up NVME target backends with the specified transport. > > + Valid elements are 'loop', 'tcp', 'rdma' and 'fc'. Default value is '(loop)'. > > - nvme_trtype: 'loop' (default), 'tcp', 'rdma' and 'fc' > > - Run the tests with the given transport. > > + Run the tests with the given transport. This parameter is still usable but > > + replaced with NVMET_TR_TYPES. Use NVMET_TR_TYPES instead. > > I noticed that nvme_trtype is still useful. nvmet_trtypes can be set in both in > the config file and the command line. But NVMET_TRTYPES can be set in the config > file only, because bash does not support setting arrays in the command line. > > # nvme_trtypes=rdma ./check nvme/006 ... works > # NVMET_TRTYPES=(rdma) ./check nvme/006 ... does not work > > I will modify the descriptions above in the v2 series to note that both > nvme_trtype and NVMET_TRTYPES are supported and usable. I rethought this. Now I think it is bad that NVMET_TRTYPES can not be specified in command lines. To avoid this drawback, I think it's the better to change NVME_TRTYPES from an array to a variable with multiple items separated with spaces. For example, three types can be specified to NVMET_TRTYPES like this: NVMET_TRTYPES="loop tcp rdma" NVMET_BLKDEV_TYPES has the same restriction then I will change it also from an array to a variable in same manner. I will send out v2 soon with this change. Daniel, I assume this change is fine for your use case. If it is not the case, please let me know.
On Tue, Apr 16, 2024 at 10:28:49AM +0000, Shinichiro Kawasaki wrote: > > # nvme_trtypes=rdma ./check nvme/006 ... works > > # NVMET_TRTYPES=(rdma) ./check nvme/006 ... does not work > > > > I will modify the descriptions above in the v2 series to note that both > > nvme_trtype and NVMET_TRTYPES are supported and usable. > > I rethought this. Now I think it is bad that NVMET_TRTYPES can not be specified > in command lines. To avoid this drawback, I think it's the better to change > NVME_TRTYPES from an array to a variable with multiple items separated with > spaces. For example, three types can be specified to NVMET_TRTYPES like this: > > NVMET_TRTYPES="loop tcp rdma" > > NVMET_BLKDEV_TYPES has the same restriction then I will change it also from an > array to a variable in same manner. I will send out v2 soon with this change. > > Daniel, > > I assume this change is fine for your use case. If it is not the case, please > let me know. Yes, it's nice that all the configure variables are of the same type. On this topic, I am a bit confused about the naming scheme. We have the lower case ones, e.g. 'nvme_trtypes' and now the upper case ones NVMET_TRYPES. I assume you prefer the upper case to mark them they are injected from the environment and the lower case ones are globals variables in the framework. Should we retire the lower case ones and replace them with upper case ones?
On 4/16/2024 9:23 AM, Daniel Wagner wrote: > On Tue, Apr 16, 2024 at 10:28:49AM +0000, Shinichiro Kawasaki wrote: >>> # nvme_trtypes=rdma ./check nvme/006 ... works >>> # NVMET_TRTYPES=(rdma) ./check nvme/006 ... does not work >>> >>> I will modify the descriptions above in the v2 series to note that both >>> nvme_trtype and NVMET_TRTYPES are supported and usable. >> >> I rethought this. Now I think it is bad that NVMET_TRTYPES can not be specified >> in command lines. To avoid this drawback, I think it's the better to change >> NVME_TRTYPES from an array to a variable with multiple items separated with >> spaces. For example, three types can be specified to NVMET_TRTYPES like this: >> >> NVMET_TRTYPES="loop tcp rdma" >> >> NVMET_BLKDEV_TYPES has the same restriction then I will change it also from an >> array to a variable in same manner. I will send out v2 soon with this change. >> >> Daniel, >> >> I assume this change is fine for your use case. If it is not the case, please >> let me know. > > Yes, it's nice that all the configure variables are of the same type. > > On this topic, I am a bit confused about the naming scheme. We have the > lower case ones, e.g. 'nvme_trtypes' and now the upper case ones > NVMET_TRYPES. I assume you prefer the upper case to mark them they are > injected from the environment and the lower case ones are globals > variables in the framework. Should we retire the lower case ones and > replace them with upper case ones? can we please keep the small letter similar to nvme_trtype ? -ck
On Apr 16, 2024 / 18:43, Chaitanya Kulkarni wrote: > On 4/16/2024 9:23 AM, Daniel Wagner wrote: > > On Tue, Apr 16, 2024 at 10:28:49AM +0000, Shinichiro Kawasaki wrote: > >>> # nvme_trtypes=rdma ./check nvme/006 ... works > >>> # NVMET_TRTYPES=(rdma) ./check nvme/006 ... does not work > >>> > >>> I will modify the descriptions above in the v2 series to note that both > >>> nvme_trtype and NVMET_TRTYPES are supported and usable. > >> > >> I rethought this. Now I think it is bad that NVMET_TRTYPES can not be specified > >> in command lines. To avoid this drawback, I think it's the better to change > >> NVME_TRTYPES from an array to a variable with multiple items separated with > >> spaces. For example, three types can be specified to NVMET_TRTYPES like this: > >> > >> NVMET_TRTYPES="loop tcp rdma" > >> > >> NVMET_BLKDEV_TYPES has the same restriction then I will change it also from an > >> array to a variable in same manner. I will send out v2 soon with this change. > >> > >> Daniel, > >> > >> I assume this change is fine for your use case. If it is not the case, please > >> let me know. > > > > Yes, it's nice that all the configure variables are of the same type. > > > > On this topic, I am a bit confused about the naming scheme. We have the > > lower case ones, e.g. 'nvme_trtypes' and now the upper case ones > > NVMET_TRYPES. I assume you prefer the upper case to mark them they are > > injected from the environment and the lower case ones are globals > > variables in the framework. Should we retire the lower case ones and > > replace them with upper case ones? Yes, "mark them they are injected from the environment" was the one reason to have the parameters in upper cases. The other reason was the consistency across the all parameters described in Documentation/running-tests.md. > > can we please keep the small letter similar to nvme_trtype ? I'm fine to have small letter, lower cases for the new parameter, but I would like to clarify the reason to have lower cases. Do you mean to indicate that "the parameters are test group local" using the lower cases?
On Wed, Apr 17, 2024 at 12:58:53AM +0000, Shinichiro Kawasaki wrote: > Yes, "mark them they are injected from the environment" was the one reason to > have the parameters in upper cases. The other reason was the consistency across > the all parameters described in Documentation/running-tests.md. > > > can we please keep the small letter similar to nvme_trtype ? > > I'm fine to have small letter, lower cases for the new parameter, but I would > like to clarify the reason to have lower cases. Do you mean to indicate that > "the parameters are test group local" using the lower cases? Lower cased environment variables are not very common, in fact POSIX.1-2017 mandates upper cased environment variables [1]. Also only the nvme part of the framework is using the lower case ones, thus I agree with Shinichiro to streamline these nvme variables to upper cased versions. [1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
On 4/16/24 23:06, Daniel Wagner wrote: > On Wed, Apr 17, 2024 at 12:58:53AM +0000, Shinichiro Kawasaki wrote: >> Yes, "mark them they are injected from the environment" was the one reason to >> have the parameters in upper cases. The other reason was the consistency across >> the all parameters described in Documentation/running-tests.md. >> >>> can we please keep the small letter similar to nvme_trtype ? >> I'm fine to have small letter, lower cases for the new parameter, but I would >> like to clarify the reason to have lower cases. Do you mean to indicate that >> "the parameters are test group local" using the lower cases? what I meant that for nvme we have lowecase global variables, in case we add new upper case variables they will create confusion ... > Lower cased environment variables are not very common, in fact > POSIX.1-2017 mandates upper cased environment variables [1]. Also only > the nvme part of the framework is using the lower case ones, thus I > agree with Shinichiro to streamline these nvme variables to upper cased > versions. > > [1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html agree they should be uppercase not denying that, the only reason I asked since existing variables are lowercase, it'd be very confusing to have some variables in nvme category with lower case and some upper case ... -ck
On Thu, Apr 18, 2024 at 06:12:40AM +0000, Chaitanya Kulkarni wrote: > agree they should be uppercase not denying that, the only reason I > asked since existing variables are lowercase, it'd be very confusing > to have some variables in nvme category with lower case and some > upper case ... Same here, so let's fix this and add upper case versions of the existing variables incl documentation. I suggest we keep the lower case ones around for awhile (maybe incl a warning?) before we drop the support. What do you think?
On 4/17/24 23:36, Daniel Wagner wrote: > On Thu, Apr 18, 2024 at 06:12:40AM +0000, Chaitanya Kulkarni wrote: >> agree they should be uppercase not denying that, the only reason I >> asked since existing variables are lowercase, it'd be very confusing >> to have some variables in nvme category with lower case and some >> upper case ... > Same here, so let's fix this and add upper case versions of the existing > variables incl documentation. I suggest we keep the lower case ones > around for awhile (maybe incl a warning?) before we drop the support. > > What do you think? that should work but let's be very careful about removing lowercase options since it might break bunch of setups .... -ck
On Apr 18, 2024 / 07:18, Chaitanya Kulkarni wrote: > On 4/17/24 23:36, Daniel Wagner wrote: > > On Thu, Apr 18, 2024 at 06:12:40AM +0000, Chaitanya Kulkarni wrote: > >> agree they should be uppercase not denying that, the only reason I > >> asked since existing variables are lowercase, it'd be very confusing > >> to have some variables in nvme category with lower case and some > >> upper case ... > > Same here, so let's fix this and add upper case versions of the existing > > variables incl documentation. I suggest we keep the lower case ones > > around for awhile (maybe incl a warning?) before we drop the support. > > > > What do you think? > > that should work but let's be very careful about removing lowercase > options since it might break bunch of setups .... I agree with this approach. I've just sent out v3 to which I added the patches to introduce the uppercase options below: nvme_img_size -> NVME_IMG_SIZE nvme_num_iter -> NVME_NUM_ITER use_rxe -> USE_RXE Both uppercase and lowercase options will work, but it is guided to use the uppercase option.
diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md index ae80860..ede3a81 100644 --- a/Documentation/running-tests.md +++ b/Documentation/running-tests.md @@ -102,8 +102,12 @@ RUN_ZONED_TESTS=1 The NVMe tests can be additionally parameterized via environment variables. +- NVMET_TR_TYPES (array) + Set up NVME target backends with the specified transport. + Valid elements are 'loop', 'tcp', 'rdma' and 'fc'. Default value is '(loop)'. - nvme_trtype: 'loop' (default), 'tcp', 'rdma' and 'fc' - Run the tests with the given transport. + Run the tests with the given transport. This parameter is still usable but + replaced with NVMET_TR_TYPES. Use NVMET_TR_TYPES instead. - nvme_img_size: '1G' (default) Run the tests with given image size in bytes. 'm', 'M', 'g' and 'G' postfix are supported. diff --git a/tests/nvme/rc b/tests/nvme/rc index 1f5ff44..df6bf77 100644 --- a/tests/nvme/rc +++ b/tests/nvme/rc @@ -18,10 +18,38 @@ def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349" def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}" export def_subsysnqn="blktests-subsystem-1" export def_subsys_uuid="91fdba0d-f87b-4c25-b80f-db7be1418b9e" -nvme_trtype=${nvme_trtype:-"loop"} nvme_img_size=${nvme_img_size:-"1G"} nvme_num_iter=${nvme_num_iter:-"1000"} +# Check consistency of NVMET_TR_TYPES and nvme_trtype configurations. +# If neither is configured, set the default value. +first_call=${first_call:-1} +if ((first_call)); then + if [[ -n $nvme_trtype ]]; then + if [[ -n $NVMET_TR_TYPES ]]; then + echo "Both nvme_trtype and NVMET_TR_TYPES are specified" + exit 1 + fi + NVMET_TR_TYPES=("$nvme_trtype") + elif [[ -z ${NVMET_TR_TYPES[*]} ]]; then + nvme_trtype="loop" + NVMET_TR_TYPES=("$nvme_trtype") + fi + first_call=0 +fi + +_set_nvme_trtype() { + local index=$1 + + if [[ -z $index ]]; then + echo ${#NVMET_TR_TYPES[@]} + return + fi + + nvme_trtype=${NVMET_TR_TYPES[index]} + COND_DESC="nvmet tr=${nvme_trtype}" +} + # TMPDIR can not be referred out of test() or test_device() context. Instead of # global variable def_flie_path, use this getter function. _nvme_def_file_path() {
Some of the test cases in nvme test group can be run under various nvme target transport types. The configuration parameter nvme_trtype specifies the transport to use. But this configuration method has two drawbacks. Firstly, the blktests check script needs to be invoked multiple times to cover multiple transport types. Secondly, the test cases irrelevant to the transport types are executed exactly same conditions in the multiple blktests runs. To avoid the drawbacks, introduce new configuration parameter NVMET_TR_TYPES. This is an array, and multiple transport types can be set like: NVMET_TR_TYPES=(loop tcp) Also introduce _nvmet_set_nvme_trtype() which can be called from the set_conditions() hook of the transport type dependent test cases. Blktests will repeat the test case as many as the number of elements in NVMET_TR_TYPES, and set nvme_trtype for each test case run. Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- Documentation/running-tests.md | 6 +++++- tests/nvme/rc | 30 +++++++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-)