| Message ID | 20220624075023.23104-1-yangx.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] nvmeof-mp/rc: Avoid skipping tests due to the expected SKIP_REASON | expand |
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.
> 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
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.
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!
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. >
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.
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
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(+)