[PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[Xiao Yang]

In _have_kernel_option(), SKIP_REASON = "kernel option NVME_MULTIPATH
has not been enabled" is expected but all nvmeof-mp tests are skipped
due to the SKIP_REASON. For example:
-----------------------------------------------------
./check nvmeof-mp/001
nvmeof-mp/***                                                [not run]
    kernel option NVME_MULTIPATH has not been enabled
-----------------------------------------------------

Avoid the issue by unsetting the SKIP_REASON.

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 tests/nvmeof-mp/rc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
index dcb2e3c..9c91f8c 100755
--- a/tests/nvmeof-mp/rc
+++ b/tests/nvmeof-mp/rc
@@ -24,6 +24,11 @@ and multipathing has been enabled in the nvme_core kernel module"
 		return
 	fi
 
+	# In _have_kernel_option(), SKIP_REASON = "kernel option
+	# NVME_MULTIPATH has not been enabled" is expected so
+	# avoid skipping tests by unsetting the SKIP_REASON
+	unset SKIP_REASON
+
 	_have_configfs || return
 	required_modules=(
 		dm_multipath
-- 
2.34.1

Re: [PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[Shinichiro Kawasaki]

On Jun 24, 2022 / 15:50, Xiao Yang wrote:
> In _have_kernel_option(), SKIP_REASON = "kernel option NVME_MULTIPATH
> has not been enabled" is expected but all nvmeof-mp tests are skipped
> due to the SKIP_REASON. For example:
> -----------------------------------------------------
> ./check nvmeof-mp/001
> nvmeof-mp/***                                                [not run]
>     kernel option NVME_MULTIPATH has not been enabled
> -----------------------------------------------------
> 
> Avoid the issue by unsetting the SKIP_REASON.
> 
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>

Good catch. Thanks!

This issue was triggered by the commit 7ae143852f6c ("common/rc: don't unset
previous SKIP_REASON in _have_kernel_option()"). So let's add a "Fixes" tag to
note it.

> ---
>  tests/nvmeof-mp/rc | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
> index dcb2e3c..9c91f8c 100755
> --- a/tests/nvmeof-mp/rc
> +++ b/tests/nvmeof-mp/rc
> @@ -24,6 +24,11 @@ and multipathing has been enabled in the nvme_core kernel module"
>  		return
>  	fi
>  
> +	# In _have_kernel_option(), SKIP_REASON = "kernel option
> +	# NVME_MULTIPATH has not been enabled" is expected so
> +	# avoid skipping tests by unsetting the SKIP_REASON

Can we have shorter comment? Like:

        # Avoid test skip due to SKIP_REASON set by _have_kernel_option().

> +	unset SKIP_REASON
> +

The change above looks good to me, and I confirmed it fixies the issue.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[Li, Zhijian]

> On Jun 24, 2022 / 15:50, Xiao Yang wrote:
> > In _have_kernel_option(), SKIP_REASON = "kernel option NVME_MULTIPATH > has not been enabled" is expected but all nvmeof-mp tests are 
> skipped > due to the SKIP_REASON. For example: > 
> ----------------------------------------------------- > ./check 
> nvmeof-mp/001 > nvmeof-mp/*** [not run] > kernel option NVME_MULTIPATH 
> has not been enabled > 
> ----------------------------------------------------- > > Avoid the 
> issue by unsetting the SKIP_REASON. > > Signed-off-by: Xiao Yang 
> <yangx.jy@fujitsu.com>
> Good catch. Thanks!
>
> This issue was triggered by the commit 7ae143852f6c ("common/rc: don't unset
> previous SKIP_REASON in _have_kernel_option()"). So let's add a "Fixes" tag to
> note it.
>
> > --- > tests/nvmeof-mp/rc | 5 +++++ > 1 file changed, 5 insertions(+) > > 
> diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc > index 
> dcb2e3c..9c91f8c 100755 > --- a/tests/nvmeof-mp/rc > +++ 
> b/tests/nvmeof-mp/rc > @@ -24,6 +24,11 @@ and multipathing has been 
> enabled in the nvme_core kernel module" > return > fi > > + # In 
> _have_kernel_option(), SKIP_REASON = "kernel option > + # 
> NVME_MULTIPATH has not been enabled" is expected so > + # avoid 
> skipping tests by unsetting the SKIP_REASON
> Can we have shorter comment? Like:
>
>          # Avoid test skip due to SKIP_REASON set by _have_kernel_option().
>
> > +	unset SKIP_REASON

Well, IMO it's not always correct to unsetSKIP_REASON, for example, if 
the OS didn't have kernel config file, we should report the test as 'not 
run'

Thanks

Zhijian


> > +
> The change above looks good to me, and I confirmed it fixies the issue.
>
> -- 
> Shin'ichiro Kawasaki

Re: [PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[Shinichiro Kawasaki]

On Jun 24, 2022 / 17:59, Li, Zhijian wrote:
> > On Jun 24, 2022 / 15:50, Xiao Yang wrote:
> > > In _have_kernel_option(), SKIP_REASON = "kernel option NVME_MULTIPATH
> > > has not been enabled" is expected but all nvmeof-mp tests are skipped
> > > due to the SKIP_REASON. For example: >
> > ----------------------------------------------------- > ./check
> > nvmeof-mp/001 > nvmeof-mp/*** [not run] > kernel option NVME_MULTIPATH
> > has not been enabled >
> > ----------------------------------------------------- > > Avoid the
> > issue by unsetting the SKIP_REASON. > > Signed-off-by: Xiao Yang
> > <yangx.jy@fujitsu.com>
> > Good catch. Thanks!
> > 
> > This issue was triggered by the commit 7ae143852f6c ("common/rc: don't unset
> > previous SKIP_REASON in _have_kernel_option()"). So let's add a "Fixes" tag to
> > note it.
> > 
> > > --- > tests/nvmeof-mp/rc | 5 +++++ > 1 file changed, 5 insertions(+) >
> > > diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc > index
> > dcb2e3c..9c91f8c 100755 > --- a/tests/nvmeof-mp/rc > +++
> > b/tests/nvmeof-mp/rc > @@ -24,6 +24,11 @@ and multipathing has been
> > enabled in the nvme_core kernel module" > return > fi > > + # In
> > _have_kernel_option(), SKIP_REASON = "kernel option > + # NVME_MULTIPATH
> > has not been enabled" is expected so > + # avoid skipping tests by
> > unsetting the SKIP_REASON
> > Can we have shorter comment? Like:
> > 
> >          # Avoid test skip due to SKIP_REASON set by _have_kernel_option().
> > 
> > > +	unset SKIP_REASON
> 
> Well, IMO it's not always correct to unsetSKIP_REASON, for example, if the
> OS didn't have kernel config file, we should report the test as 'not run'

Actually, this group_requires() in tests/nvmeof-mp/rc has another
_have_kernel_option() call for DM_UEVENT. It will catch and report the "no
kernel config "case. So, I think it is ok to apply Xiao's solution to fix
the current issue.

I think Li's point is still valid, but let's take action for it later. One more
point I want to mention is that "unset SKIP_REASON" is not a good practice. To
seek for the best shape, I can think of following changes:

1) Introduce _check_kernel_option(), which checks the specified kernel option
   is defined, but does not set SKIP_RESAON. Using this, "unset SKIP_REASON" of
   group_requires() in tests/nvme-of/rc (and tests/nvme/039) can be avoided.

2) Introduce _have_kernel_config_file() which sets SKIP_REASON when neither
   /boot/config* nor /proc/config.gz is available. It can be called from the
   group_requires() in tests/nvme-of/rc before _check_kernel_option() to ensure
   the kernel option check is valid.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[Shinichiro Kawasaki]

On Jun 24, 2022 / 08:20, Shinichiro Kawasaki wrote:
> On Jun 24, 2022 / 15:50, Xiao Yang wrote:
> > In _have_kernel_option(), SKIP_REASON = "kernel option NVME_MULTIPATH
> > has not been enabled" is expected but all nvmeof-mp tests are skipped
> > due to the SKIP_REASON. For example:
> > -----------------------------------------------------
> > ./check nvmeof-mp/001
> > nvmeof-mp/***                                                [not run]
> >     kernel option NVME_MULTIPATH has not been enabled
> > -----------------------------------------------------
> > 
> > Avoid the issue by unsetting the SKIP_REASON.
> > 
> > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> 
> Good catch. Thanks!
> 
> This issue was triggered by the commit 7ae143852f6c ("common/rc: don't unset
> previous SKIP_REASON in _have_kernel_option()"). So let's add a "Fixes" tag to
> note it.
> 
> > ---
> >  tests/nvmeof-mp/rc | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
> > index dcb2e3c..9c91f8c 100755
> > --- a/tests/nvmeof-mp/rc
> > +++ b/tests/nvmeof-mp/rc
> > @@ -24,6 +24,11 @@ and multipathing has been enabled in the nvme_core kernel module"
> >  		return
> >  	fi
> >  
> > +	# In _have_kernel_option(), SKIP_REASON = "kernel option
> > +	# NVME_MULTIPATH has not been enabled" is expected so
> > +	# avoid skipping tests by unsetting the SKIP_REASON
> 
> Can we have shorter comment? Like:
> 
>         # Avoid test skip due to SKIP_REASON set by _have_kernel_option().
> 
> > +	unset SKIP_REASON
> > +
> 
> The change above looks good to me, and I confirmed it fixies the issue.

I've applied this patch with the edits above. Thanks!

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[Li, Zhijian]

on 6/24/2022 8:17 PM, Shinichiro Kawasaki wrote:
> On Jun 24, 2022 / 17:59, Li, Zhijian wrote:
>>> On Jun 24, 2022 / 15:50, Xiao Yang wrote:
>>>> In _have_kernel_option(), SKIP_REASON = "kernel option NVME_MULTIPATH
>>>> has not been enabled" is expected but all nvmeof-mp tests are skipped
>>>> due to the SKIP_REASON. For example: >
>>> ----------------------------------------------------- > ./check
>>> nvmeof-mp/001 > nvmeof-mp/*** [not run] > kernel option NVME_MULTIPATH
>>> has not been enabled >
>>> ----------------------------------------------------- > > Avoid the
>>> issue by unsetting the SKIP_REASON. > > Signed-off-by: Xiao Yang
>>> <yangx.jy@fujitsu.com>
>>> Good catch. Thanks!
>>>
>>> This issue was triggered by the commit 7ae143852f6c ("common/rc: don't unset
>>> previous SKIP_REASON in _have_kernel_option()"). So let's add a "Fixes" tag to
>>> note it.
>>>
>>>> --- > tests/nvmeof-mp/rc | 5 +++++ > 1 file changed, 5 insertions(+) >
>>>> diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc > index
>>> dcb2e3c..9c91f8c 100755 > --- a/tests/nvmeof-mp/rc > +++
>>> b/tests/nvmeof-mp/rc > @@ -24,6 +24,11 @@ and multipathing has been
>>> enabled in the nvme_core kernel module" > return > fi > > + # In
>>> _have_kernel_option(), SKIP_REASON = "kernel option > + # NVME_MULTIPATH
>>> has not been enabled" is expected so > + # avoid skipping tests by
>>> unsetting the SKIP_REASON
>>> Can we have shorter comment? Like:
>>>
>>>           # Avoid test skip due to SKIP_REASON set by _have_kernel_option().
>>>
>>>> +	unset SKIP_REASON
>> Well, IMO it's not always correct to unsetSKIP_REASON, for example, if the
>> OS didn't have kernel config file, we should report the test as 'not run'
> Actually, this group_requires() in tests/nvmeof-mp/rc has another
> _have_kernel_option() call for DM_UEVENT. It will catch and report the "no
> kernel config "case. So, I think it is ok to apply Xiao's solution to fix
> the current issue.
>
> I think Li's point is still valid, but let's take action for it later. One more
> point I want to mention is that "unset SKIP_REASON" is not a good practice. To
> seek for the best shape, I can think of following changes:

below changes sound good :)


>
> 1) Introduce _check_kernel_option(), which checks the specified kernel option
>     is defined, but does not set SKIP_RESAON. Using this, "unset SKIP_REASON" of
>     group_requires() in tests/nvme-of/rc (and tests/nvme/039) can be avoided.
>
> 2) Introduce _have_kernel_config_file() which sets SKIP_REASON when neither
>     /boot/config* nor /proc/config.gz is available. It can be called from the
>     group_requires() in tests/nvme-of/rc before _check_kernel_option() to ensure
>     the kernel option check is valid.
>

Re: [PATCH blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON

[yangx.jy]

On 2022/6/25 15:51, Li, Zhijian wrote:
>> I think Li's point is still valid, but let's take action for it later. 
>> One more
>> point I want to mention is that "unset SKIP_REASON" is not a good 
>> practice. To
>> seek for the best shape, I can think of following changes:
> 
> below changes sound good :)
> 
Hi Li, Shinichiro

Sorry for the late reply.

Agreed. I also think "unset SKIP_REASON" is a temporary solution (not a 
better solution).

I will try to do the following change as you suggested.

Best Regards,
Xiao Yang
> 
>>
>> 1) Introduce _check_kernel_option(), which checks the specified kernel 
>> option
>>     is defined, but does not set SKIP_RESAON. Using this, "unset 
>> SKIP_REASON" of
>>     group_requires() in tests/nvme-of/rc (and tests/nvme/039) can be 
>> avoided.
>>
>> 2) Introduce _have_kernel_config_file() which sets SKIP_REASON when 
>> neither
>>     /boot/config* nor /proc/config.gz is available. It can be called 
>> from the
>>     group_requires() in tests/nvme-of/rc before _check_kernel_option() 
>> to ensure
>>     the kernel option check is valid.