| Message ID | 20210719071337.217501-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] fstests: allow running custom hooks | expand |
On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: > This patch will allow fstests to run custom hooks before and after each > test case. Nice! This is better than what I had been doing which was to set: export LOGGER_PROG=/usr/local/lib/gce-logger ... and then parse the passed message to be logged for "run xfstests $seqnum", and which only worked to hook the start of each test. > diff --git a/README.hooks b/README.hooks > new file mode 100644 > index 00000000..be92a7d7 > --- /dev/null > +++ b/README.hooks > @@ -0,0 +1,72 @@ > +To run extra commands before and after each test case, there is the > +'hooks/start.hook' and 'hooks/end.hook' files for such usage. > + > +Some notes for those two hooks: > + > +- Both hook files needs to be executable > + Or they will just be ignored Minor nit: I'd reword this as: - The hook script must be executable or it will be ignored. > diff --git a/check b/check > index bb7e030c..f24906f5 100755 > --- a/check > +++ b/check > @@ -846,6 +846,10 @@ function run_section() > # to be reported for each test > (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 > > + # Remove previous $seqres.full before start hook > + rm -f $seqres.full > + > + _run_start_hook I wonder if it would be useful to have the start hook have a way to signal that a particular test should be skipped. This might allow for various programatic tests that could be inserted by the test runner framework. (E.g., this is the 5.4 kernel, we know this test is guaranteed to fail, so tell check to skip the test) - Ted
On 2021/7/19 下午10:02, Theodore Y. Ts'o wrote: > On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: >> This patch will allow fstests to run custom hooks before and after each >> test case. > > Nice! This is better than what I had been doing which was to set: > > export LOGGER_PROG=/usr/local/lib/gce-logger > > ... and then parse the passed message to be logged for "run xfstests > $seqnum", and which only worked to hook the start of each test. > >> diff --git a/README.hooks b/README.hooks >> new file mode 100644 >> index 00000000..be92a7d7 >> --- /dev/null >> +++ b/README.hooks >> @@ -0,0 +1,72 @@ >> +To run extra commands before and after each test case, there is the >> +'hooks/start.hook' and 'hooks/end.hook' files for such usage. >> + >> +Some notes for those two hooks: >> + >> +- Both hook files needs to be executable >> + Or they will just be ignored > > Minor nit: I'd reword this as: > > - The hook script must be executable or it > will be ignored. > >> diff --git a/check b/check >> index bb7e030c..f24906f5 100755 >> --- a/check >> +++ b/check >> @@ -846,6 +846,10 @@ function run_section() >> # to be reported for each test >> (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 >> >> + # Remove previous $seqres.full before start hook >> + rm -f $seqres.full >> + >> + _run_start_hook > > I wonder if it would be useful to have the start hook have a way to > signal that a particular test should be skipped. This might allow for > various programatic tests that could be inserted by the test runner > framework. Currently it's impossible, as the design is to prevent any hook to interrupt the test. But if we allow test case to return its result, then it should be not hard to make us to skip test cases using start hook. I can enhance the next version to do that, but that also means any error inside the hook will bring down the whole test run. Not sure the trade-off is worthy then. Thanks, Qu > > (E.g., this is the 5.4 kernel, we know this test is guaranteed to > fail, so tell check to skip the test) > > - Ted >
On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: > This patch will allow fstests to run custom hooks before and after each > test case. > > These hooks will need to follow requirements: > > - Both hook files needs to be executable > Or they will just be ignored > > - Stderr and stdout will be redirected to "$seqres.full" > With extra separator to distinguish the hook output with real > test output > > Thus if any of the hook is specified, all tests will generate > "$seqres.full" which may increase the disk usage for results. > > - Error in hooks script will be ignored completely > > - Environment variable "$HOOK_TEMP" will be exported for both hooks > And the variable will be ensured not to change for both hooks. > > Thus it's possible to store temporary values between the two hooks, > like pid. > > - Start hook has only one parameter passed in > $1 is "$seq" from "check" script. The content will the path of current > test case. E.g "tests/btrfs/001" > > - End hook has two parameters passed in > $1 is the same as start hook. > $2 is the return value of the test case. > NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak > check. > > For more info, please refer to "README.hooks". This is all info that should be in README.hooks, not in the commit message. Commit messages are about explaining why something needs to exist or be changed, not to describe the change being made. This commit message doesn't tell me anything about what this is for, so I can't really make any value judgement on it - exactly what is this intended to be used for? FWIW, if a test needs something to be run before/after the test, it really should be in the test, run as part of the test. Adding overhead to every test being just to check for something that doesn't actually have a defined use, nor will exist or be used on the vast majority of systems running fstests doesn't seem like the best idea to me. Cheers, Dave.
On 2021/7/20 上午8:25, Dave Chinner wrote: > On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: >> This patch will allow fstests to run custom hooks before and after each >> test case. >> >> These hooks will need to follow requirements: >> >> - Both hook files needs to be executable >> Or they will just be ignored >> >> - Stderr and stdout will be redirected to "$seqres.full" >> With extra separator to distinguish the hook output with real >> test output >> >> Thus if any of the hook is specified, all tests will generate >> "$seqres.full" which may increase the disk usage for results. >> >> - Error in hooks script will be ignored completely >> >> - Environment variable "$HOOK_TEMP" will be exported for both hooks >> And the variable will be ensured not to change for both hooks. >> >> Thus it's possible to store temporary values between the two hooks, >> like pid. >> >> - Start hook has only one parameter passed in >> $1 is "$seq" from "check" script. The content will the path of current >> test case. E.g "tests/btrfs/001" >> >> - End hook has two parameters passed in >> $1 is the same as start hook. >> $2 is the return value of the test case. >> NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak >> check. >> >> For more info, please refer to "README.hooks". > > This is all info that should be in README.hooks, not in the commit > message. Commit messages are about explaining why something needs > to exist or be changed, not to describe the change being made. This > commit message doesn't tell me anything about what this is for, so I > can't really make any value judgement on it - exactly what is this > intended to be used for? To run whatever you may want. Don't you want to run some trace-cmd to record the ftrace buffer for certain tests to debug? > > FWIW, if a test needs something to be run before/after the test, it > really should be in the test, run as part of the test. Not the trace-cmd things one is going to debug. > Adding > overhead to every test being just to check for something that > doesn't actually have a defined use, nor will exist or be used on > the vast majority of systems running fstests doesn't seem like the > best idea to me. Then you can do whatever you did when you debug certain test case like before, adding whatever commands you need into "check" script. If you believe that's the cleanest way to debug, then sure. Thanks, Qu > > Cheers, > > Dave. >
On Tue, Jul 20, 2021 at 06:06:00AM +0800, Qu Wenruo wrote: > > I can enhance the next version to do that, but that also means any error > inside the hook will bring down the whole test run. I don't see why that would be? We just have to sample the exit status of the hook script, and if it matches a specific value, we skip the test. If the hook script crashes, the exit status will be some other value, e.g., 128+<signal_number>, 127 if the script doesn't exist, 126 if the script exists but is not executable, etc. So we just sample $? and if it is, say, 83 (ascii 'S') we skip the test; otherwise, we run the test. How would an error inside the hook "bring down the whole test run"? Cheers, - Ted
On 2021/7/20 上午8:43, Theodore Y. Ts'o wrote: > On Tue, Jul 20, 2021 at 06:06:00AM +0800, Qu Wenruo wrote: >> >> I can enhance the next version to do that, but that also means any error >> inside the hook will bring down the whole test run. > > I don't see why that would be? We just have to sample the exit status > of the hook script, and if it matches a specific value, we skip the > test. That's what I expect to do next. > If the hook script crashes, the exit status will be some other > value, e.g., 128+<signal_number>, 127 if the script doesn't exist, 126 > if the script exists but is not executable, etc. That's the concern I have. If something crashed, I really don't want that to affect the test case itself. E.g. if some setup is half backed and then crashed, how to ensure the test can still run without problem? But I also understand that, only developers will utilize this feature anyway, so it's should be the developers' response to make sure the hook runs correctly. > So we just sample $? > and if it is, say, 83 (ascii 'S') we skip the test; otherwise, we run > the test. How would an error inside the hook "bring down the whole > test run"? My original concern would be something like error injection setup. But that could make the test case fail anyway, thus it shouldn't be a concern. Anyway, I'll add extra comment to README.hook to explicit say, if one is using hooks, it's their response to make sure their hook to work as expected. Thanks, Qu > > Cheers, > > - Ted >
On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: > This patch will allow fstests to run custom hooks before and after each > test case. > > These hooks will need to follow requirements: > > - Both hook files needs to be executable > Or they will just be ignored > > - Stderr and stdout will be redirected to "$seqres.full" > With extra separator to distinguish the hook output with real > test output > > Thus if any of the hook is specified, all tests will generate > "$seqres.full" which may increase the disk usage for results. > > - Error in hooks script will be ignored completely > > - Environment variable "$HOOK_TEMP" will be exported for both hooks > And the variable will be ensured not to change for both hooks. > > Thus it's possible to store temporary values between the two hooks, > like pid. > > - Start hook has only one parameter passed in > $1 is "$seq" from "check" script. The content will the path of current > test case. E.g "tests/btrfs/001" > > - End hook has two parameters passed in > $1 is the same as start hook. > $2 is the return value of the test case. > NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak > check. > > For more info, please refer to "README.hooks". > > Also update .gitignore to ignore "hooks/start.hook" and "hooks/end.hook" > so that one won't accidentally submit the debug hook. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Instead of previous attempt to manually utilize sysfs interface of > ftrace, this time just add some hooks to allow one to do whatever they > want. > > RFC for how everything should be passed to hooks. > Currently it's using a mixed method, $seq/$sts is passed as paramaters, > while HOOK_TMP is passed as environmental variable. > > Not sure what's the recommended way for hooks. > --- > .gitignore | 4 +++ > README.hooks | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ > check | 5 ++++ > common/preamble | 4 --- > common/rc | 43 +++++++++++++++++++++++++++++ > 5 files changed, 124 insertions(+), 4 deletions(-) > create mode 100644 README.hooks > > diff --git a/.gitignore b/.gitignore > index 2d72b064..99905ff9 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -201,3 +201,7 @@ tags > # cscope files > cscope.* > ncscope.* > + > +# hook scripts > +/hooks/start.hook > +/hooks/end.hook > diff --git a/README.hooks b/README.hooks > new file mode 100644 > index 00000000..be92a7d7 > --- /dev/null > +++ b/README.hooks > @@ -0,0 +1,72 @@ > +To run extra commands before and after each test case, there is the > +'hooks/start.hook' and 'hooks/end.hook' files for such usage. > + > +Some notes for those two hooks: > + > +- Both hook files needs to be executable > + Or they will just be ignored > + > +- Stderr and stdout will be redirected to "$seqres.full" > + With extra separator to distinguish the hook output with real > + test output > + > + Thus if any of the hook is specified, all tests will generate > + "$seqres.full" which may increase the disk usage for results. > + > +- Error in hooks script will be ignored completely > + > +- Environment variable "$HOOK_TEMP" will be exported for both hooks > + And the variable will be ensured not to change for both hooks. > + > + Thus it's possible to store temporary values between the two hooks, > + like pid. > + > +- Start hook has only one parameter passed in > + $1 is "$seq" from "check" script. The content will the path of current > + test case. E.g "tests/btrfs/001" > + > +- End hook has two parameters passed in > + $1 is the same as start hook. > + $2 is the return value of the test case. > + NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak > + check. > + > +The very basic test hooks would look like: > + > +hooks/start.hook: > + #!/bin/bash > + seq=$1 > + echo "HOOK_TMP=$HOOK_TMP" > + echo "seq=$seq" > + > +hooks/end.hook: > + #!/bin/bash > + seq=$1 > + sts=$2 > + echo "HOOK_TMP=$HOOK_TMP" > + echo "seq=$seq" > + echo "sts=$sts" > + > +Then run test btrfs/001 and btrfs/002, their "$seqres.full" would look like: > + > +result/btrfs/001.full: > + === Running start hook === > + HOOK_TMP=/tmp/78973.hook > + seq=tests/btrfs/001 > + === Start hook finished === > + === Running end hook === > + HOOK_TMP=/tmp/78973.hook > + seq=tests/btrfs/001 > + sts=0 > + === End hook finished === > + > +result/btrfs/002.full: > + === Running start hook === > + HOOK_TMP=/tmp/78973.hook > + seq=tests/btrfs/002 > + === Start hook finished === > + === Running end hook === > + HOOK_TMP=/tmp/78973.hook > + seq=tests/btrfs/002 > + sts=0 > + === End hook finished === > diff --git a/check b/check > index bb7e030c..f24906f5 100755 > --- a/check > +++ b/check > @@ -846,6 +846,10 @@ function run_section() > # to be reported for each test > (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 > > + # Remove previous $seqres.full before start hook > + rm -f $seqres.full > + > + _run_start_hook Seeing as we now have standardized preamble and cleanup in every single test, why don't you run this from _begin_fstest and modify _register_cleanup to run _run_end_hook from the trap handler? This way the start hooks run from within each test and therefore can modify the test's environment (as opposed to ./check's environment; the two are /not/ the same thing!) and/or _notrun the test like Ted wants. (Granted I wonder why not use an exclude list, because I bet my 3.10 kernel has patches that yours doesn't, so kernel versions aren't all that meaningful...) --D > if [ "$DUMP_OUTPUT" = true ]; then > _run_seq 2>&1 | tee $tmp.out > # Because $? would get tee's return code > @@ -854,6 +858,7 @@ function run_section() > _run_seq >$tmp.out 2>&1 > sts=$? > fi > + _run_end_hook > > if [ -f core ]; then > _dump_err_cont "[dumped core]" > diff --git a/common/preamble b/common/preamble > index 66b0ed05..41a12060 100644 > --- a/common/preamble > +++ b/common/preamble > @@ -56,8 +56,4 @@ _begin_fstest() > _register_cleanup _cleanup > > . ./common/rc > - > - # remove previous $seqres.full before test > - rm -f $seqres.full > - > } > diff --git a/common/rc b/common/rc > index d4b1f21f..ec434aa5 100644 > --- a/common/rc > +++ b/common/rc > @@ -4612,6 +4612,49 @@ _require_names_are_bytes() { > esac > } > > +_run_start_hook() > +{ > + if [[ ! -d "hooks" ]]; then > + return > + fi > + > + if [[ ! -x "hooks/start.hook" ]]; then > + return > + fi > + > + # Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp", > + # so we won't overwrite any existing $tmp.* files > + export HOOK_TMP=$tmp.hook > + > + echo "=== Running start hook ===" >> $seqres.full > + # $1 is alwasy $seq > + ./hooks/start.hook $seq >> $seqres.full 2>&1 > + echo "=== Start hook finished ===" >> $seqres.full > + unset HOOK_TMP > +} > + > +_run_end_hook() > +{ > + if [[ ! -d "hooks" ]]; then > + return > + fi > + > + if [[ ! -x "hooks/end.hook" ]]; then > + return > + fi > + > + # Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp", > + # so we won't overwrite any existing $tmp.* files > + export HOOK_TMP=$tmp.hook > + > + echo "=== Running end hook ===" >> "$seqres.full" > + # $1 is alwasy $seq > + # $2 is alwasy $sts > + ./hooks/end.hook $seq $sts >> "$seqres.full" 2>&1 > + echo "=== End hook finished ===" >> "$seqres.full" > + unset HOOK_TMP > +} > + > init_rc > > ################################################################################ > -- > 2.31.1 >
On 2021/7/20 上午9:16, Darrick J. Wong wrote: > On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: >> This patch will allow fstests to run custom hooks before and after each >> test case. >> >> These hooks will need to follow requirements: >> >> - Both hook files needs to be executable >> Or they will just be ignored >> >> - Stderr and stdout will be redirected to "$seqres.full" >> With extra separator to distinguish the hook output with real >> test output >> >> Thus if any of the hook is specified, all tests will generate >> "$seqres.full" which may increase the disk usage for results. >> >> - Error in hooks script will be ignored completely >> >> - Environment variable "$HOOK_TEMP" will be exported for both hooks >> And the variable will be ensured not to change for both hooks. >> >> Thus it's possible to store temporary values between the two hooks, >> like pid. >> >> - Start hook has only one parameter passed in >> $1 is "$seq" from "check" script. The content will the path of current >> test case. E.g "tests/btrfs/001" >> >> - End hook has two parameters passed in >> $1 is the same as start hook. >> $2 is the return value of the test case. >> NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak >> check. >> >> For more info, please refer to "README.hooks". >> >> Also update .gitignore to ignore "hooks/start.hook" and "hooks/end.hook" >> so that one won't accidentally submit the debug hook. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> Instead of previous attempt to manually utilize sysfs interface of >> ftrace, this time just add some hooks to allow one to do whatever they >> want. >> >> RFC for how everything should be passed to hooks. >> Currently it's using a mixed method, $seq/$sts is passed as paramaters, >> while HOOK_TMP is passed as environmental variable. >> >> Not sure what's the recommended way for hooks. >> --- >> .gitignore | 4 +++ >> README.hooks | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ >> check | 5 ++++ >> common/preamble | 4 --- >> common/rc | 43 +++++++++++++++++++++++++++++ >> 5 files changed, 124 insertions(+), 4 deletions(-) >> create mode 100644 README.hooks >> >> diff --git a/.gitignore b/.gitignore >> index 2d72b064..99905ff9 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -201,3 +201,7 @@ tags >> # cscope files >> cscope.* >> ncscope.* >> + >> +# hook scripts >> +/hooks/start.hook >> +/hooks/end.hook >> diff --git a/README.hooks b/README.hooks >> new file mode 100644 >> index 00000000..be92a7d7 >> --- /dev/null >> +++ b/README.hooks >> @@ -0,0 +1,72 @@ >> +To run extra commands before and after each test case, there is the >> +'hooks/start.hook' and 'hooks/end.hook' files for such usage. >> + >> +Some notes for those two hooks: >> + >> +- Both hook files needs to be executable >> + Or they will just be ignored >> + >> +- Stderr and stdout will be redirected to "$seqres.full" >> + With extra separator to distinguish the hook output with real >> + test output >> + >> + Thus if any of the hook is specified, all tests will generate >> + "$seqres.full" which may increase the disk usage for results. >> + >> +- Error in hooks script will be ignored completely >> + >> +- Environment variable "$HOOK_TEMP" will be exported for both hooks >> + And the variable will be ensured not to change for both hooks. >> + >> + Thus it's possible to store temporary values between the two hooks, >> + like pid. >> + >> +- Start hook has only one parameter passed in >> + $1 is "$seq" from "check" script. The content will the path of current >> + test case. E.g "tests/btrfs/001" >> + >> +- End hook has two parameters passed in >> + $1 is the same as start hook. >> + $2 is the return value of the test case. >> + NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak >> + check. >> + >> +The very basic test hooks would look like: >> + >> +hooks/start.hook: >> + #!/bin/bash >> + seq=$1 >> + echo "HOOK_TMP=$HOOK_TMP" >> + echo "seq=$seq" >> + >> +hooks/end.hook: >> + #!/bin/bash >> + seq=$1 >> + sts=$2 >> + echo "HOOK_TMP=$HOOK_TMP" >> + echo "seq=$seq" >> + echo "sts=$sts" >> + >> +Then run test btrfs/001 and btrfs/002, their "$seqres.full" would look like: >> + >> +result/btrfs/001.full: >> + === Running start hook === >> + HOOK_TMP=/tmp/78973.hook >> + seq=tests/btrfs/001 >> + === Start hook finished === >> + === Running end hook === >> + HOOK_TMP=/tmp/78973.hook >> + seq=tests/btrfs/001 >> + sts=0 >> + === End hook finished === >> + >> +result/btrfs/002.full: >> + === Running start hook === >> + HOOK_TMP=/tmp/78973.hook >> + seq=tests/btrfs/002 >> + === Start hook finished === >> + === Running end hook === >> + HOOK_TMP=/tmp/78973.hook >> + seq=tests/btrfs/002 >> + sts=0 >> + === End hook finished === >> diff --git a/check b/check >> index bb7e030c..f24906f5 100755 >> --- a/check >> +++ b/check >> @@ -846,6 +846,10 @@ function run_section() >> # to be reported for each test >> (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 >> >> + # Remove previous $seqres.full before start hook >> + rm -f $seqres.full >> + >> + _run_start_hook > > Seeing as we now have standardized preamble and cleanup in every single > test, why don't you run this from _begin_fstest and modify > _register_cleanup to run _run_end_hook from the trap handler? That's a great advice. As my mindset is still in the pre-preamble age... > > This way the start hooks run from within each test and therefore can > modify the test's environment (as opposed to ./check's environment; the > two are /not/ the same thing!) and/or _notrun the test like Ted wants. That would be super awesome, although it means the hook user should also be extra cautious not to crash the hook itself... Thanks, Qu > > (Granted I wonder why not use an exclude list, because I bet my 3.10 > kernel has patches that yours doesn't, so kernel versions aren't all > that meaningful...) > > --D > >> if [ "$DUMP_OUTPUT" = true ]; then >> _run_seq 2>&1 | tee $tmp.out >> # Because $? would get tee's return code >> @@ -854,6 +858,7 @@ function run_section() >> _run_seq >$tmp.out 2>&1 >> sts=$? >> fi >> + _run_end_hook >> >> if [ -f core ]; then >> _dump_err_cont "[dumped core]" >> diff --git a/common/preamble b/common/preamble >> index 66b0ed05..41a12060 100644 >> --- a/common/preamble >> +++ b/common/preamble >> @@ -56,8 +56,4 @@ _begin_fstest() >> _register_cleanup _cleanup >> >> . ./common/rc >> - >> - # remove previous $seqres.full before test >> - rm -f $seqres.full >> - >> } >> diff --git a/common/rc b/common/rc >> index d4b1f21f..ec434aa5 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -4612,6 +4612,49 @@ _require_names_are_bytes() { >> esac >> } >> >> +_run_start_hook() >> +{ >> + if [[ ! -d "hooks" ]]; then >> + return >> + fi >> + >> + if [[ ! -x "hooks/start.hook" ]]; then >> + return >> + fi >> + >> + # Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp", >> + # so we won't overwrite any existing $tmp.* files >> + export HOOK_TMP=$tmp.hook >> + >> + echo "=== Running start hook ===" >> $seqres.full >> + # $1 is alwasy $seq >> + ./hooks/start.hook $seq >> $seqres.full 2>&1 >> + echo "=== Start hook finished ===" >> $seqres.full >> + unset HOOK_TMP >> +} >> + >> +_run_end_hook() >> +{ >> + if [[ ! -d "hooks" ]]; then >> + return >> + fi >> + >> + if [[ ! -x "hooks/end.hook" ]]; then >> + return >> + fi >> + >> + # Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp", >> + # so we won't overwrite any existing $tmp.* files >> + export HOOK_TMP=$tmp.hook >> + >> + echo "=== Running end hook ===" >> "$seqres.full" >> + # $1 is alwasy $seq >> + # $2 is alwasy $sts >> + ./hooks/end.hook $seq $sts >> "$seqres.full" 2>&1 >> + echo "=== End hook finished ===" >> "$seqres.full" >> + unset HOOK_TMP >> +} >> + >> init_rc >> >> ################################################################################ >> -- >> 2.31.1 >> >
On Tue, Jul 20, 2021 at 08:36:49AM +0800, Qu Wenruo wrote: > > > On 2021/7/20 上午8:25, Dave Chinner wrote: > > On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: > > > This patch will allow fstests to run custom hooks before and after each > > > test case. > > > > > > These hooks will need to follow requirements: > > > > > > - Both hook files needs to be executable > > > Or they will just be ignored > > > > > > - Stderr and stdout will be redirected to "$seqres.full" > > > With extra separator to distinguish the hook output with real > > > test output > > > > > > Thus if any of the hook is specified, all tests will generate > > > "$seqres.full" which may increase the disk usage for results. > > > > > > - Error in hooks script will be ignored completely > > > > > > - Environment variable "$HOOK_TEMP" will be exported for both hooks > > > And the variable will be ensured not to change for both hooks. > > > > > > Thus it's possible to store temporary values between the two hooks, > > > like pid. > > > > > > - Start hook has only one parameter passed in > > > $1 is "$seq" from "check" script. The content will the path of current > > > test case. E.g "tests/btrfs/001" > > > > > > - End hook has two parameters passed in > > > $1 is the same as start hook. > > > $2 is the return value of the test case. > > > NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak > > > check. > > > > > > For more info, please refer to "README.hooks". > > > > This is all info that should be in README.hooks, not in the commit > > message. Commit messages are about explaining why something needs > > to exist or be changed, not to describe the change being made. This > > commit message doesn't tell me anything about what this is for, so I > > can't really make any value judgement on it - exactly what is this > > intended to be used for? > > To run whatever you may want. No, don't try to turn this around and put it on me to think up use cases to justify your change. You have a use case for this, so *document it so everyone understands what it is*. > Don't you want to run some trace-cmd to record the ftrace buffer for > certain tests to debug? I already have a way of doing that, thanks - the command line is just fine for tracing failing tests. IOWs, I don't actually need hooks inside fstests for that. Again, this isn't about what I need from fstests. This is something _you_ want, so describe your use case and how these hooks are the best way to provide the functionality you require. > > FWIW, if a test needs something to be run before/after the test, it > > really should be in the test, run as part of the test. > > Not the trace-cmd things one is going to debug. I don't follow your reasoning, likely because you haven't actually described how you intend to use these hooks. If it's for tracing one-off test failures, then we can already do that from the command line. If it's for something else, then you haven't described what that is yet.... > > Adding > > overhead to every test being just to check for something that > > doesn't actually have a defined use, nor will exist or be used on > > the vast majority of systems running fstests doesn't seem like the > > best idea to me. > > Then you can do whatever you did when you debug certain test case like > before, adding whatever commands you need into "check" script. > > If you believe that's the cleanest way to debug, then sure. Again, this isn't about what I "beleive". My concerns are about whether the infrastructure is maintainable from a long term persepective, and that all depends on what use cases we have for it and whether global hooks are the likely best solution to those use cases over the long term. I'm not opposed to adding hooks, I'm just asking for context and justification that is needed to be able to consider if this is the best solution for the use cases that are put forward... It may be there are different ways to do what you need, or there are better places to implement it, or it might need more fine grained scope than an single global hook that all tests run. I can see that per-test granularity would be much preferable to having to do this sort of stuff in global hook files: case $seq in generic/001) .... ;; generic/005) .... ;; .... esac But that assumes that this is intended for hooking every test and doing different things for different tests (which appears to be what the implementation does). IOWs, without a description of your use case and requirements, I have no basis from which to determine if this is useful infrastructure over the long term or not. It may be a horrible maintenance nightmare when people start writing tests that are dependent on certain hooks being present once the infrastructure is in place.... Cheers, Dave.
On 2021/7/20 上午10:14, Dave Chinner wrote: > On Tue, Jul 20, 2021 at 08:36:49AM +0800, Qu Wenruo wrote: >> >> >> On 2021/7/20 上午8:25, Dave Chinner wrote: >>> On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: >>>> This patch will allow fstests to run custom hooks before and after each >>>> test case. >>>> >>>> These hooks will need to follow requirements: >>>> >>>> - Both hook files needs to be executable >>>> Or they will just be ignored >>>> >>>> - Stderr and stdout will be redirected to "$seqres.full" >>>> With extra separator to distinguish the hook output with real >>>> test output >>>> >>>> Thus if any of the hook is specified, all tests will generate >>>> "$seqres.full" which may increase the disk usage for results. >>>> >>>> - Error in hooks script will be ignored completely >>>> >>>> - Environment variable "$HOOK_TEMP" will be exported for both hooks >>>> And the variable will be ensured not to change for both hooks. >>>> >>>> Thus it's possible to store temporary values between the two hooks, >>>> like pid. >>>> >>>> - Start hook has only one parameter passed in >>>> $1 is "$seq" from "check" script. The content will the path of current >>>> test case. E.g "tests/btrfs/001" >>>> >>>> - End hook has two parameters passed in >>>> $1 is the same as start hook. >>>> $2 is the return value of the test case. >>>> NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak >>>> check. >>>> >>>> For more info, please refer to "README.hooks". >>> >>> This is all info that should be in README.hooks, not in the commit >>> message. Commit messages are about explaining why something needs >>> to exist or be changed, not to describe the change being made. This >>> commit message doesn't tell me anything about what this is for, so I >>> can't really make any value judgement on it - exactly what is this >>> intended to be used for? >> >> To run whatever you may want. > > No, don't try to turn this around and put it on me to think up use > cases to justify your change. You have a use case for this, so > *document it so everyone understands what it is*. If you don't need it, then fine. But there are already other guys interesting in this feature. Talk to them too. Something you don't need doesn't mean other don't. > >> Don't you want to run some trace-cmd to record the ftrace buffer for >> certain tests to debug? > > I already have a way of doing that, thanks - the command line is > just fine for tracing failing tests. IOWs, I don't actually need > hooks inside fstests for that. Then don't teach me how to do my debug setup. > > Again, this isn't about what I need from fstests. This is something > _you_ want, so describe your use case and how these hooks are the > best way to provide the functionality you require. > >>> FWIW, if a test needs something to be run before/after the test, it >>> really should be in the test, run as part of the test. >> >> Not the trace-cmd things one is going to debug. > > I don't follow your reasoning, likely because you haven't actually > described how you intend to use these hooks. > > If it's for tracing one-off test failures, then we can already do > that from the command line. If it's for something else, then you > haven't described what that is yet.... > >>> Adding >>> overhead to every test being just to check for something that >>> doesn't actually have a defined use, nor will exist or be used on >>> the vast majority of systems running fstests doesn't seem like the >>> best idea to me. >> >> Then you can do whatever you did when you debug certain test case like >> before, adding whatever commands you need into "check" script. >> >> If you believe that's the cleanest way to debug, then sure. > > Again, this isn't about what I "beleive". > > My concerns are about whether the infrastructure is maintainable > from a long term persepective, and that all depends on what use > cases we have for it and whether global hooks are the likely best > solution to those use cases over the long term. I'm not opposed to > adding hooks, I'm just asking for context and justification that is > needed to be able to consider if this is the best solution for the > use cases that are put forward... Nope. All hook users are responsible for whatever they do. Maybe it's adding trace-cmd calls, maybe it's to do extra error injection setup, I don't care. I just provide a way to do that more simply, to add two points to call executable scripts, and any modification in their hooks won't be submitted to fstests by default (check the gitignore update) What users do in their hooks is not what I really care, and nor what you should care. You use your wrapper or whatever command to do your debug, fine. Don't expect others should follow whatever you do. > > It may be there are different ways to do what you need, or there are > better places to implement it, or it might need more fine grained > scope than an single global hook that all tests run. I can see that > per-test granularity would be much preferable to having to do this > sort of stuff in global hook files: > > case $seq in > generic/001) .... > ;; > generic/005) .... > ;; > .... > esac > > But that assumes that this is intended for hooking every test and > doing different things for different tests (which appears to be what > the implementation does). If someone is running hooks with every test case, it's their problem or intention. I don't care! > > IOWs, without a description of your use case and requirements, I > have no basis from which to determine if this is useful > infrastructure over the long term or not. OK, my use case is just to run "trace-cmd clear" before one test, btrfs/160, and I don't want to populate my workplace so that I may submit some patch with my debugging setup included. I would only run that btrfs/160 with my custom hook, that's all. But I don't know whether Ted/Darrick would want to do for their workload. Everyone has their own special debug setup, and I don't really want to be taught how I should debug. > It may be a horrible > maintenance nightmare when people start writing tests that are > dependent on certain hooks being present once the infrastructure is > in place.... Check the gitignore! The hook will be ignored by default! I should add something to the README like: "If you use hook, don't expect any support from fstests community, you're on your own" Thanks, Qu > > Cheers, > > Dave. >
On Mon, Jul 19, 2021 at 10:02:14AM -0400, Theodore Y. Ts'o wrote: > On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: > > This patch will allow fstests to run custom hooks before and after each > > test case. > > Nice! This is better than what I had been doing which was to set: > > export LOGGER_PROG=/usr/local/lib/gce-logger > > ... and then parse the passed message to be logged for "run xfstests > $seqnum", and which only worked to hook the start of each test. > > > diff --git a/README.hooks b/README.hooks > > new file mode 100644 > > index 00000000..be92a7d7 > > --- /dev/null > > +++ b/README.hooks > > @@ -0,0 +1,72 @@ > > +To run extra commands before and after each test case, there is the > > +'hooks/start.hook' and 'hooks/end.hook' files for such usage. > > + > > +Some notes for those two hooks: > > + > > +- Both hook files needs to be executable > > + Or they will just be ignored > > Minor nit: I'd reword this as: > > - The hook script must be executable or it > will be ignored. > > > diff --git a/check b/check > > index bb7e030c..f24906f5 100755 > > --- a/check > > +++ b/check > > @@ -846,6 +846,10 @@ function run_section() > > # to be reported for each test > > (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 > > > > + # Remove previous $seqres.full before start hook > > + rm -f $seqres.full > > + > > + _run_start_hook > > I wonder if it would be useful to have the start hook have a way to > signal that a particular test should be skipped. This might allow for > various programatic tests that could be inserted by the test runner > framework. I have the same question as Darrick does, we now have the exclude list infra to skip a given set of tests, does that work in your case? Why do you need pre hook to skip a test? Thanks, Eryu > > (E.g., this is the 5.4 kernel, we know this test is guaranteed to > fail, so tell check to skip the test) > > - Ted
On Tue, Jul 20, 2021 at 10:45:17AM +0800, Qu Wenruo wrote: > > > On 2021/7/20 上午10:14, Dave Chinner wrote: > > On Tue, Jul 20, 2021 at 08:36:49AM +0800, Qu Wenruo wrote: > > > > > > > > > On 2021/7/20 上午8:25, Dave Chinner wrote: > > > > On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: > > > > > This patch will allow fstests to run custom hooks before and after each > > > > > test case. > > > > > > > > > > These hooks will need to follow requirements: > > > > > > > > > > - Both hook files needs to be executable > > > > > Or they will just be ignored > > > > > > > > > > - Stderr and stdout will be redirected to "$seqres.full" > > > > > With extra separator to distinguish the hook output with real > > > > > test output > > > > > > > > > > Thus if any of the hook is specified, all tests will generate > > > > > "$seqres.full" which may increase the disk usage for results. > > > > > > > > > > - Error in hooks script will be ignored completely > > > > > > > > > > - Environment variable "$HOOK_TEMP" will be exported for both hooks > > > > > And the variable will be ensured not to change for both hooks. > > > > > > > > > > Thus it's possible to store temporary values between the two hooks, > > > > > like pid. > > > > > > > > > > - Start hook has only one parameter passed in > > > > > $1 is "$seq" from "check" script. The content will the path of current > > > > > test case. E.g "tests/btrfs/001" > > > > > > > > > > - End hook has two parameters passed in > > > > > $1 is the same as start hook. > > > > > $2 is the return value of the test case. > > > > > NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak > > > > > check. > > > > > > > > > > For more info, please refer to "README.hooks". > > > > > > > > This is all info that should be in README.hooks, not in the commit > > > > message. Commit messages are about explaining why something needs > > > > to exist or be changed, not to describe the change being made. This > > > > commit message doesn't tell me anything about what this is for, so I > > > > can't really make any value judgement on it - exactly what is this > > > > intended to be used for? > > > > > > To run whatever you may want. > > > > No, don't try to turn this around and put it on me to think up use > > cases to justify your change. You have a use case for this, so > > *document it so everyone understands what it is*. > > If you don't need it, then fine. > > But there are already other guys interesting in this feature. > > Talk to them too. > > Something you don't need doesn't mean other don't. Hold up, what's with the attitude? I asked you to describe the use case for the hooks because it wasn't in the commit message and I don't have a clue how you intend to use it. Hence I need you, the patch author and submitter, to tell me what it is intended for. Shouting at me telling how I should read about what others want when instead of actually answering my question by describing your use case as I've asked you (repeatedly) to describe is not helpful. So please clam down, take a step back and please explain to me in a calm, rational, professional manner what this functionality is needed for. > > My concerns are about whether the infrastructure is maintainable > > from a long term persepective, and that all depends on what use > > cases we have for it and whether global hooks are the likely best > > solution to those use cases over the long term. I'm not opposed to > > adding hooks, I'm just asking for context and justification that is > > needed to be able to consider if this is the best solution for the > > use cases that are put forward... > > Nope. All hook users are responsible for whatever they do. > > Maybe it's adding trace-cmd calls, maybe it's to do extra error > injection setup, I don't care. You might not care, but other people will. I can see that if these execution hooks becomes a common thing people use, it will need to be formalised and further developed and documented. Because if they are in widespread use, we really, really care about things like API changes because breaking everyone's custom hook scripts because we removed or changed a global variable or function is not in anyone's interests. > I just provide a way to do that more simply, to add two points to call > executable scripts, and any modification in their hooks won't be > submitted to fstests by default (check the gitignore update) > What users do in their hooks is not what I really care, and nor what you > should care. Yes, I get it that you don't care. And that's a problem. ISTM that you haven't thought this through beyond "add a hook for running throw-away debug code". You clearly haven't thought about what a developer would need to do to build a library of hook implementations for debugging different tests. I say that because maintaining that library via commits in local fstests repositories is impossible because gitignore rules you added. I know, you don't care, but I very much do, because storing stacks of custom changes to fstests in local repositories is how I deploy fstests to my test machines... And given that it appears you haven't thought about maintaining a local repository of hooks, I strongly doubt you've even consider the impact of future changes to the hook API on existing hook scripts that devs and test engineers have written over months and years while debugging test failures. Darrick pointed out the difference between running in the check vs test environment, which is something that is very much an API consideration - we change the test environment arbitrarily and fix all the tests that change affects at the same time. But if there are private scripts that depend on the test environment and variables being stable, then we can't do things like, say, rename the "seqres" variable across all tests because that will break every custom hook script out there that writes to $seqres.full... See what I'm getting at here? I've looked at the proposal and I'm asking questions that your implementation raises. I'm not asking these questions to be difficult, I'm asking them because I want to know if I can use the hooks to replace some of the things I do with other methods. And if I do, how I'm expected to maintain and deploy them to the test machine farm from my master git repository they all pull from... > If someone is running hooks with every test case, it's their problem or > intention. I don't care! > > > > > IOWs, without a description of your use case and requirements, I > > have no basis from which to determine if this is useful > > infrastructure over the long term or not. > OK, my use case is just to run "trace-cmd clear" before one test, > btrfs/160, and I don't want to populate my workplace so that I may > submit some patch with my debugging setup included. > > I would only run that btrfs/160 with my custom hook, that's all. If you have to submit a patch with the debugging and custom hook defined in it to the build system, why can't you just submit a patch that, say, runs "trace-cmd clear" at the start of btrfs/160 itself? IOWs, I don't understand why generic, fstests-global script execution hooks are needed just to run a simple command for helping debug a single test. Why not just *patch the test* and then once you're done throw away the debug patch? Cheers, Dave.
On 2021/7/20 下午2:43, Dave Chinner wrote: > On Tue, Jul 20, 2021 at 10:45:17AM +0800, Qu Wenruo wrote: >> >> >> On 2021/7/20 上午10:14, Dave Chinner wrote: >>> On Tue, Jul 20, 2021 at 08:36:49AM +0800, Qu Wenruo wrote: >>>> >>>> >>>> On 2021/7/20 上午8:25, Dave Chinner wrote: >>>>> On Mon, Jul 19, 2021 at 03:13:37PM +0800, Qu Wenruo wrote: >>>>>> This patch will allow fstests to run custom hooks before and after each >>>>>> test case. >>>>>> >>>>>> These hooks will need to follow requirements: >>>>>> >>>>>> - Both hook files needs to be executable >>>>>> Or they will just be ignored >>>>>> >>>>>> - Stderr and stdout will be redirected to "$seqres.full" >>>>>> With extra separator to distinguish the hook output with real >>>>>> test output >>>>>> >>>>>> Thus if any of the hook is specified, all tests will generate >>>>>> "$seqres.full" which may increase the disk usage for results. >>>>>> >>>>>> - Error in hooks script will be ignored completely >>>>>> >>>>>> - Environment variable "$HOOK_TEMP" will be exported for both hooks >>>>>> And the variable will be ensured not to change for both hooks. >>>>>> >>>>>> Thus it's possible to store temporary values between the two hooks, >>>>>> like pid. >>>>>> >>>>>> - Start hook has only one parameter passed in >>>>>> $1 is "$seq" from "check" script. The content will the path of current >>>>>> test case. E.g "tests/btrfs/001" >>>>>> >>>>>> - End hook has two parameters passed in >>>>>> $1 is the same as start hook. >>>>>> $2 is the return value of the test case. >>>>>> NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak >>>>>> check. >>>>>> >>>>>> For more info, please refer to "README.hooks". >>>>> >>>>> This is all info that should be in README.hooks, not in the commit >>>>> message. Commit messages are about explaining why something needs >>>>> to exist or be changed, not to describe the change being made. This >>>>> commit message doesn't tell me anything about what this is for, so I >>>>> can't really make any value judgement on it - exactly what is this >>>>> intended to be used for? >>>> >>>> To run whatever you may want. >>> >>> No, don't try to turn this around and put it on me to think up use >>> cases to justify your change. You have a use case for this, so >>> *document it so everyone understands what it is*. >> >> If you don't need it, then fine. >> >> But there are already other guys interesting in this feature. >> >> Talk to them too. >> >> Something you don't need doesn't mean other don't. > > Hold up, what's with the attitude? > > I asked you to describe the use case for the hooks because it wasn't > in the commit message and I don't have a clue how you intend to use > it. Hence I need you, the patch author and submitter, to tell me > what it is intended for. > > Shouting at me telling how I should read about what others want when > instead of actually answering my question by describing your use > case as I've asked you (repeatedly) to describe is not helpful. > > So please clam down, take a step back and please explain to me in a > calm, rational, professional manner what this functionality is > needed for. I only need to do the very simple work like call trace-cmd clear and trace-cmd set -b 4. But I can see other guys may want such generic hook to do whatever they want (but still overall simple things). Bringing a generic simple framework while you can't see the usage to replace your complex stack isn't the correct way to go. I admit it's not obvious in the RFC that only simple work should be put into the hook (aka throw away nature). > >>> My concerns are about whether the infrastructure is maintainable >>> from a long term persepective, and that all depends on what use >>> cases we have for it and whether global hooks are the likely best >>> solution to those use cases over the long term. I'm not opposed to >>> adding hooks, I'm just asking for context and justification that is >>> needed to be able to consider if this is the best solution for the >>> use cases that are put forward... >> >> Nope. All hook users are responsible for whatever they do. >> >> Maybe it's adding trace-cmd calls, maybe it's to do extra error >> injection setup, I don't care. > > You might not care, but other people will. I can see that if these > execution hooks becomes a common thing people use, it will need to > be formalised and further developed and documented. This is already your guess. Again, here the hook is simple, do your work there, if it works, good the hook works for you. If it brings some unexpected behavior, you're on your own, RTFC crafted by yourself. We will explicitly state that, this feature is only for debug purpose and use on your risk. No complain, no code compatibility, if one day code update break your hook, it's not our problem, it's the one who crafts the hook. > Because if they > are in widespread use, we really, really care about things like API > changes because breaking everyone's custom hook scripts because we > removed or changed a global variable or function is not in anyone's > interests. You're again building the whole thing on the "if" part. > >> I just provide a way to do that more simply, to add two points to call >> executable scripts, and any modification in their hooks won't be >> submitted to fstests by default (check the gitignore update) >> What users do in their hooks is not what I really care, and nor what you >> should care. > > Yes, I get it that you don't care. And that's a problem. > > ISTM that you haven't thought this through beyond "add a hook for > running throw-away debug code". You clearly haven't thought about > what a developer would need to do to build a library of hook > implementations for debugging different tests. No, we are not doing that complex thing whatever you want to utilize. It's just a simple hook, no guarantee, just to make it simple to do simple things. > I say that because > maintaining that library via commits in local fstests repositories > is impossible because gitignore rules you added. Because it's supposed to be simple. > I know, you don't > care, but I very much do, because storing stacks of custom changes > to fstests in local repositories is how I deploy fstests to my test > machines... If your stack is so complex, this is not for you. Submit whatever you may want to add to fstests, and try to push a complex framework for your workload. > > And given that it appears you haven't thought about maintaining a > local repository of hooks, I strongly doubt you've even consider the > impact of future changes to the hook API on existing hook scripts > that devs and test engineers have written over months and years > while debugging test failures. Let me state it again, fstests itself only cares for the test cases. You use the hook, you're on your own. Don't expect any stable thing, this is just for simple usage. > > Darrick pointed out the difference between running in the check vs > test environment, which is something that is very much an API > consideration - we change the test environment arbitrarily and fix > all the tests that change affects at the same time. But if there are > private scripts that depend on the test environment and variables > being stable, then we can't do things like, say, rename the "seqres" > variable across all tests because that will break every custom hook > script out there that writes to $seqres.full... > > See what I'm getting at here? I've looked at the proposal and I'm > asking questions that your implementation raises. I'm not asking > these questions to be difficult, I'm asking them because I want to > know if I can use the hooks to replace some of the things I do with > other methods. My answer to this question is, this hook is only for simple work. Either you maintain your complex hook to work for latest fstests, or just put simple things into it. If you want more complex work or stable ABI, then this is not for you. > And if I do, how I'm expected to maintain and deploy > them to the test machine farm from my master git repository they all > pull from... This feature is at least not designed for such usage. It's more for the through away debug setup. > >> If someone is running hooks with every test case, it's their problem or >> intention. I don't care! >> >>> >>> IOWs, without a description of your use case and requirements, I >>> have no basis from which to determine if this is useful >>> infrastructure over the long term or not. >> OK, my use case is just to run "trace-cmd clear" before one test, >> btrfs/160, and I don't want to populate my workplace so that I may >> submit some patch with my debugging setup included. >> >> I would only run that btrfs/160 with my custom hook, that's all. > > If you have to submit a patch with the debugging and custom hook > defined in it to the build system, why can't you just submit a patch > that, say, runs "trace-cmd clear" at the start of btrfs/160 itself? > > IOWs, I don't understand why generic, fstests-global script > execution hooks are needed just to run a simple command for helping > debug a single test. Why not just *patch the test* and then once > you're done throw away the debug patch? It's again back to the original question. I only need to run several small commands, but I guess others may be interested in adding some more complex but still overall simple work. If it helps you, great. If it doesn't, just don't use it and go back to what works for you. > > Cheers, > > Dave. >
On Tue, Jul 20, 2021 at 04:43:17PM +1000, Dave Chinner wrote: [snip] > > And given that it appears you haven't thought about maintaining a > local repository of hooks, I strongly doubt you've even consider the > impact of future changes to the hook API on existing hook scripts > that devs and test engineers have written over months and years > while debugging test failures. > > Darrick pointed out the difference between running in the check vs > test environment, which is something that is very much an API > consideration - we change the test environment arbitrarily and fix > all the tests that change affects at the same time. But if there are > private scripts that depend on the test environment and variables > being stable, then we can't do things like, say, rename the "seqres" > variable across all tests because that will break every custom hook > script out there that writes to $seqres.full... I was thinking about this as well, if such private hook scripts are useful to others as well, then I think maybe it's worth to maintain such scripts in fstests repo, and further changes to the hook API won't break the scripts Thanks, Eryu
On 2021/7/20 下午3:57, Eryu Guan wrote: > On Tue, Jul 20, 2021 at 04:43:17PM +1000, Dave Chinner wrote: > [snip] >> >> And given that it appears you haven't thought about maintaining a >> local repository of hooks, I strongly doubt you've even consider the >> impact of future changes to the hook API on existing hook scripts >> that devs and test engineers have written over months and years >> while debugging test failures. >> >> Darrick pointed out the difference between running in the check vs >> test environment, which is something that is very much an API >> consideration - we change the test environment arbitrarily and fix >> all the tests that change affects at the same time. But if there are >> private scripts that depend on the test environment and variables >> being stable, then we can't do things like, say, rename the "seqres" >> variable across all tests because that will break every custom hook >> script out there that writes to $seqres.full... > > I was thinking about this as well, if such private hook scripts are > useful to others as well, then I think maybe it's worth to maintain such > scripts in fstests repo, and further changes to the hook API won't break > the scripts But those hook scripts are really craft by each developer, which can have vastly different usage. How could that be maintained inside fstests? Thanks, Qu > > Thanks, > Eryu >
On 2021/7/20 下午4:29, Qu Wenruo wrote: > > > On 2021/7/20 下午3:57, Eryu Guan wrote: >> On Tue, Jul 20, 2021 at 04:43:17PM +1000, Dave Chinner wrote: >> [snip] >>> >>> And given that it appears you haven't thought about maintaining a >>> local repository of hooks, I strongly doubt you've even consider the >>> impact of future changes to the hook API on existing hook scripts >>> that devs and test engineers have written over months and years >>> while debugging test failures. >>> >>> Darrick pointed out the difference between running in the check vs >>> test environment, which is something that is very much an API >>> consideration - we change the test environment arbitrarily and fix >>> all the tests that change affects at the same time. But if there are >>> private scripts that depend on the test environment and variables >>> being stable, then we can't do things like, say, rename the "seqres" >>> variable across all tests because that will break every custom hook >>> script out there that writes to $seqres.full... >> >> I was thinking about this as well, if such private hook scripts are >> useful to others as well, then I think maybe it's worth to maintain such >> scripts in fstests repo, and further changes to the hook API won't break >> the scripts > > But those hook scripts are really craft by each developer, which can > have vastly different usage. > > How could that be maintained inside fstests? Anyway, if building a stable and complex API just for hooks, then it's completely against my initial purpose. I'll just discard this patch then. Thanks, Qu > > Thanks, > Qu >> >> Thanks, >> Eryu >>
On Tue, Jul 20, 2021 at 04:44:29PM +0800, Qu Wenruo wrote: > Anyway, if building a stable and complex API just for hooks, then it's > completely against my initial purpose. So you said you are only using this for debugging purposes; at least in my workflow, when I'm trying to debug a particular test case, I'm *only* run a single test. So today, the way I handle this is to run hook scripts in the gce-xfstests framework before and after running the check script. It does mean that there is excess work which gets traced from the check script setting up and cleaning up, but it's mainly been good enough for me. It would be slightly nicer if there was a way to start and stop various debugging hooks (e.g., starting and stopping traces, clearing and grabing the lock_Stat info), etc.,) jut before and starting the test. But it was never important to me to propose changes to the upstream xfstests. In answer to the question which Darrick and Dave asked, yes, I could do this via the exclude list. It just seemed that if we're going to add a programmtic hook, maybe it would make sense to do it via the hook script. It is true that there are ways to do this, though, although there is a difference to a static excldue list that has to get created for all tests that might be run, versus hook script that is run for each test. <Shrug> There are other ways of doing things; the question is whether the hook script approach is sufficiently better that it's worth getting something like this upstream. It may be that keeping things like this in each developer's own private test runner is the right thing. I have "gce-xfstests --hooks <hookdir>" for my own use, and have had it for years. Cheers, - Ted
On 2021/7/20 下午11:38, Theodore Y. Ts'o wrote: > On Tue, Jul 20, 2021 at 04:44:29PM +0800, Qu Wenruo wrote: >> Anyway, if building a stable and complex API just for hooks, then it's >> completely against my initial purpose. > > So you said you are only using this for debugging purposes; at least > in my workflow, when I'm trying to debug a particular test case, I'm > *only* run a single test. Under most case, yes, and that's also what I usually do. But I have seen some strange case which doesn't trigger error by itself, but needs some other tests to be run before the triggering one. > So today, the way I handle this is to run > hook scripts in the gce-xfstests framework before and after running > the check script. It does mean that there is excess work which gets > traced from the check script setting up and cleaning up, but it's > mainly been good enough for me. > > It would be slightly nicer if there was a way to start and stop > various debugging hooks (e.g., starting and stopping traces, clearing > and grabing the lock_Stat info), etc.,) jut before and starting the > test. But it was never important to me to propose changes to the > upstream xfstests. From the feedback, I guess that's the case. I would no longer consider to upstream any simple debug purposed code. Thanks, Qu > > In answer to the question which Darrick and Dave asked, yes, I could > do this via the exclude list. It just seemed that if we're going to > add a programmtic hook, maybe it would make sense to do it via the > hook script. It is true that there are ways to do this, though, > although there is a difference to a static excldue list that has to > get created for all tests that might be run, versus hook script that > is run for each test. <Shrug> There are other ways of doing things; > the question is whether the hook script approach is sufficiently > better that it's worth getting something like this upstream. It may > be that keeping things like this in each developer's own private test > runner is the right thing. I have "gce-xfstests --hooks <hookdir>" for > my own use, and have had it for years. > > Cheers, > > - Ted >
On Wed, Jul 21, 2021 at 06:34:16AM +0800, Qu Wenruo wrote:
> I would no longer consider to upstream any simple debug purposed code.
Qu, please stop behaving like a small child throwing a tantrum
because they were told no.
If there's good reason to host debug code in the fstests repository,
that's where it should go. See the patch I just posted that adds a
dm-logwrites replay script to the tools/ directory:
https://lore.kernel.org/fstests/20210721001333.2999103-1-david@fromorbit.com/T/#u
This is really necessary to be able to analyse failures from tests
that use dm-logwrites, and such a tool does not exist. Rather than
requiring every developer that has to debug a dm-logwrites failure
have to write their own replay tool, fstests should provide one.
That's the whole point here. I could be selfish and say "it's a
debugging tool, I don't need to publish it because others can just
write their own", but that ignores the fact it took me the best part
of two days just to come up to speed on what dm-logwrites and
generic/482 was doing before I could even begin to debug the
failure.
Requiring everyone to pass that high bar just to begin to debug a
g/482 failure is not an effective use of community time and
resources. The script I wrote embodies the main logwrites
interactions I needed to reproduce and debug the issue, and I don't
think anyone else should need to spend a couple of days of WTFing
around the logwrites code just to be able to manually replay a
failed g/482 test case. I've sunk that cost into a simple to use
script and by pushing it into the fstests repository nobody else now
needs to spend that time to write a manual replay script.
If we apply that same logic to debugging hooks and the scripts that
they run, then a hook script that is useful to one person for
debugging a complex test is probably going to be useful to many more
people. Hence if we are going to include hooks into the fstests
infrastructure, we also need to consider including a method of
curating a libary of hook scripts that people can just link into the
hooks/ directory and start using with no development time at all.
You need to stop thinking about debug code as "throw-away code".
Debug code is just as important, if not more important, than the code
that is being tested. As Brian Kernighan once said:
"Debugging is twice as hard as writing the code in the first
place. Therefore, if you write the code as cleverly as
possible, you are, by definition, not smart enough to debug
it."
Put simply, anything we can do to lower the bar for debugging
complex code exercised by complex tests is worth doing and *worth
doing well*. Hooks can be a powerful debugging tool, but the
introduction of such infrastructure needs more discussion and
consideration than "here's a rudimetary start/end hook for one-off
throw-away debug code".
Most importantly, the discussion needs a much more constructive
conversation than responding "No because I don't care about anyone
else" to every suggestion or potential issue that is raised. Please
try to be constructive and help move the discussion forward,
otherwise the functionality you propose won't go anywhere largely
because of your own behaviour rather than for unsovlable technical
reasons...
Cheers,
Dave.
On 2021/7/21 上午9:11, Dave Chinner wrote: > On Wed, Jul 21, 2021 at 06:34:16AM +0800, Qu Wenruo wrote: >> I would no longer consider to upstream any simple debug purposed code. > > Qu, please stop behaving like a small child throwing a tantrum > because they were told no. Well, if you think so, go ahead, no one can change your mind anyway. > > If there's good reason to host debug code in the fstests repository, > that's where it should go. See the patch I just posted that adds a > dm-logwrites replay script to the tools/ directory: > > https://lore.kernel.org/fstests/20210721001333.2999103-1-david@fromorbit.com/T/#u > > This is really necessary to be able to analyse failures from tests > that use dm-logwrites, and such a tool does not exist. Rather than > requiring every developer that has to debug a dm-logwrites failure > have to write their own replay tool, fstests should provide one. > > That's the whole point here. I could be selfish and say "it's a > debugging tool, I don't need to publish it because others can just > write their own", but that ignores the fact it took me the best part > of two days just to come up to speed on what dm-logwrites and > generic/482 was doing before I could even begin to debug the > failure. > > Requiring everyone to pass that high bar just to begin to debug a > g/482 failure is not an effective use of community time and > resources. The script I wrote embodies the main logwrites > interactions I needed to reproduce and debug the issue, and I don't > think anyone else should need to spend a couple of days of WTFing > around the logwrites code just to be able to manually replay a > failed g/482 test case. I've sunk that cost into a simple to use > script and by pushing it into the fstests repository nobody else now > needs to spend that time to write a manual replay script. > > If we apply that same logic to debugging hooks and the scripts that > they run, then a hook script that is useful to one person for > debugging a complex test is probably going to be useful to many more > people. Hence if we are going to include hooks into the fstests > infrastructure, we also need to consider including a method of > curating a libary of hook scripts that people can just link into the > hooks/ directory and start using with no development time at all. > > You need to stop thinking about debug code as "throw-away code". > Debug code is just as important, if not more important, than the code > that is being tested. As Brian Kernighan once said: > > "Debugging is twice as hard as writing the code in the first > place. Therefore, if you write the code as cleverly as > possible, you are, by definition, not smart enough to debug > it." > > Put simply, anything we can do to lower the bar for debugging > complex code exercised by complex tests is worth doing and *worth > doing well*. Hooks can be a powerful debugging tool, but the > introduction of such infrastructure needs more discussion and > consideration than "here's a rudimetary start/end hook for one-off > throw-away debug code". > > Most importantly, the discussion needs a much more constructive > conversation than responding "No because I don't care about anyone > else" to every suggestion or potential issue that is raised. Please > try to be constructive and help move the discussion forward, > otherwise the functionality you propose won't go anywhere largely > because of your own behaviour rather than for unsovlable technical > reasons... I'm pretty clear about the hook I supposed, it's not for stable ABI or complex framework, just a simple kit to make things a little easier. The single purpose is just to make some throw-away debug setup simpler. Whether debug tool should be throw-away is very debatable, and you're pushing your narrative so much, that's very annoying already. You can have your complex framework for your farm, I can also have my simple setup running on RPI4. I won't bother however you build your debug environment, nor you should. Sometimes I already see the test setup of fstests too complex. I totally understand it's for the portability and reproducibility, but for certain debugs, I prefer to craft a small bash script with the core contents copied from fstests, with all the complex probing/requirement, which can always populate the ftrace buffer. If you believe your philosophy that every test tool should be a complex mess, you're free to do whatever you always do. And I can always express my objection just like you. So, you want to build a complex framework using the simple hook, I would just say NO. And you have made yourself clear that you want to make your debug setup complex and stable, then I understand and just won't waste my time on someone can't understand something KISS. Thanks, Qu > > Cheers, > > Dave. >
On 2021/07/21 10:53, Qu Wenruo wrote: > > > On 2021/7/21 上午9:11, Dave Chinner wrote: >> On Wed, Jul 21, 2021 at 06:34:16AM +0800, Qu Wenruo wrote: >>> I would no longer consider to upstream any simple debug purposed code. >> >> Qu, please stop behaving like a small child throwing a tantrum >> because they were told no. > > Well, if you think so, go ahead, no one can change your mind anyway. > >> >> If there's good reason to host debug code in the fstests repository, >> that's where it should go. See the patch I just posted that adds a >> dm-logwrites replay script to the tools/ directory: >> >> https://lore.kernel.org/fstests/20210721001333.2999103-1-david@fromorbit.com/T/#u >> >> This is really necessary to be able to analyse failures from tests >> that use dm-logwrites, and such a tool does not exist. Rather than >> requiring every developer that has to debug a dm-logwrites failure >> have to write their own replay tool, fstests should provide one. >> >> That's the whole point here. I could be selfish and say "it's a >> debugging tool, I don't need to publish it because others can just >> write their own", but that ignores the fact it took me the best part >> of two days just to come up to speed on what dm-logwrites and >> generic/482 was doing before I could even begin to debug the >> failure. >> >> Requiring everyone to pass that high bar just to begin to debug a >> g/482 failure is not an effective use of community time and >> resources. The script I wrote embodies the main logwrites >> interactions I needed to reproduce and debug the issue, and I don't >> think anyone else should need to spend a couple of days of WTFing >> around the logwrites code just to be able to manually replay a >> failed g/482 test case. I've sunk that cost into a simple to use >> script and by pushing it into the fstests repository nobody else now >> needs to spend that time to write a manual replay script. >> >> If we apply that same logic to debugging hooks and the scripts that >> they run, then a hook script that is useful to one person for >> debugging a complex test is probably going to be useful to many more >> people. Hence if we are going to include hooks into the fstests >> infrastructure, we also need to consider including a method of >> curating a libary of hook scripts that people can just link into the >> hooks/ directory and start using with no development time at all. >> >> You need to stop thinking about debug code as "throw-away code". >> Debug code is just as important, if not more important, than the code >> that is being tested. As Brian Kernighan once said: >> >> "Debugging is twice as hard as writing the code in the first >> place. Therefore, if you write the code as cleverly as >> possible, you are, by definition, not smart enough to debug >> it." >> >> Put simply, anything we can do to lower the bar for debugging >> complex code exercised by complex tests is worth doing and *worth >> doing well*. Hooks can be a powerful debugging tool, but the >> introduction of such infrastructure needs more discussion and >> consideration than "here's a rudimetary start/end hook for one-off >> throw-away debug code". >> >> Most importantly, the discussion needs a much more constructive >> conversation than responding "No because I don't care about anyone >> else" to every suggestion or potential issue that is raised. Please >> try to be constructive and help move the discussion forward, >> otherwise the functionality you propose won't go anywhere largely >> because of your own behaviour rather than for unsovlable technical >> reasons... > > I'm pretty clear about the hook I supposed, it's not for stable ABI or > complex framework, just a simple kit to make things a little easier. > > The single purpose is just to make some throw-away debug setup simpler. > > Whether debug tool should be throw-away is very debatable, and you're > pushing your narrative so much, that's very annoying already. > > You can have your complex framework for your farm, I can also have my > simple setup running on RPI4. > > I won't bother however you build your debug environment, nor you should. > > Sometimes I already see the test setup of fstests too complex. > I totally understand it's for the portability and reproducibility, but > for certain debugs, I prefer to craft a small bash script with the core > contents copied from fstests, with all the complex probing/requirement, > which can always populate the ftrace buffer. > > > If you believe your philosophy that every test tool should be a complex > mess, you're free to do whatever you always do. > > And I can always express my objection just like you. > > So, you want to build a complex framework using the simple hook, I would > just say NO. I think that Dave's opinion is actually the reverse of this: hooks, if well designed, can be useful and facilitate adding functionalities to a complex test framework. And sure, the hook infrastructure implementation can in itself be complex, but if well designed, its use can be, and should be, very simple. Implementation is done once. The complexity at that stage matters much less than the end result usability. As a general principle, the more time one put in design and development, the better the end result. Here, that means hooks being useful, flexible, extensible, and reusable. And one of the functionality of the hook setup could be "temporary, external hook" for some very special case debugging as you seem to need that. What you want to do and what Dave is proposing are not mutually exclusive. > > And you have made yourself clear that you want to make your debug setup > complex and stable, then I understand and just won't waste my time on > someone can't understand something KISS. > > Thanks, > Qu > >> >> Cheers, >> >> Dave. >> >
On 2021/7/21 上午10:23, Damien Le Moal wrote: >> >> I'm pretty clear about the hook I supposed, it's not for stable ABI or >> complex framework, just a simple kit to make things a little easier. >> >> The single purpose is just to make some throw-away debug setup simpler. >> >> Whether debug tool should be throw-away is very debatable, and you're >> pushing your narrative so much, that's very annoying already. >> >> You can have your complex framework for your farm, I can also have my >> simple setup running on RPI4. >> >> I won't bother however you build your debug environment, nor you should. >> >> Sometimes I already see the test setup of fstests too complex. >> I totally understand it's for the portability and reproducibility, but >> for certain debugs, I prefer to craft a small bash script with the core >> contents copied from fstests, with all the complex probing/requirement, >> which can always populate the ftrace buffer. >> >> >> If you believe your philosophy that every test tool should be a complex >> mess, you're free to do whatever you always do. >> >> And I can always express my objection just like you. >> >> So, you want to build a complex framework using the simple hook, I would >> just say NO. > > I think that Dave's opinion is actually the reverse of this: hooks, if well > designed, can be useful and facilitate adding functionalities to a complex test > framework. And sure, the hook infrastructure implementation can in itself be > complex, but if well designed, its use can be, and should be, very simple. In fact, if we really integrate the hook into the fstests to the standard of Dave, I doubt it can be that simple ever. E.g. if the hook is going to inherit all the fstests shell macros, from _not_run() to various _require_*, then it's completely the opposite what I want, and it's not going to be simpler than writing a proper test case. What I original want is so simple that for start hook, it only accepts one parameter (for the sequence number), one environment variable (for the temporary path). That's the level of simplicity I want, no complex calls/probes just a proper test case. Yes, a well designed hook can do that without problem, as it will be superset of the simple hook. But then when one is going to do complex things, and reading the doc, it will be way more complex than the original purpose. And I hate to even have such possibility to be complex. Ironically not to mention the maintaining effort involved. E.g. if the hook is going to inherit those shell variables/macors, then any changing in the fstests itself will affect the hooks, no matter if there is any real users of such complex thing. > > Implementation is done once. The complexity at that stage matters much less than > the end result usability. As a general principle, the more time one put in > design and development, the better the end result. Here, that means hooks being > useful, flexible, extensible, and reusable. Hard to say. I still can't yet get used to the preamble change introduced recently. (The group list not properly updated) And it's pretty clear it won't be the last change of the fstest infrastructure. Hook won't be more stable than fstests itself, and it will take time to maintain, no matter if there is really users for it. > > And one of the functionality of the hook setup could be "temporary, external > hook" for some very special case debugging as you seem to need that. What you > want to do and what Dave is proposing are not mutually exclusive. Yeah, it's a subset in theory. But then that's completely unrelated to my initial purpose. I won't object if someone is willing to do that separately, and may be even happy if a such dedicated, simple hook can be provided without reading the complex doc/design of the framework. But putting tons of unrelated things into an originally simple purpose, no. Please do that in a separate patch/thread for the complex hook framework then. Thanks, Qu > >> >> And you have made yourself clear that you want to make your debug setup >> complex and stable, then I understand and just won't waste my time on >> someone can't understand something KISS. >> >> Thanks, >> Qu >> >>> >>> Cheers, >>> >>> Dave. >>> >> > >
On Wed, Jul 21, 2021 at 02:23:14AM +0000, Damien Le Moal wrote: > On 2021/07/21 10:53, Qu Wenruo wrote: > > On 2021/7/21 上午9:11, Dave Chinner wrote: > > If you believe your philosophy that every test tool should be a complex > > mess, you're free to do whatever you always do. > > > > And I can always express my objection just like you. > > > > So, you want to build a complex framework using the simple hook, I would > > just say NO. > > I think that Dave's opinion is actually the reverse of this: hooks, if well > designed, can be useful and facilitate adding functionalities to a complex test > framework. And sure, the hook infrastructure implementation can in itself be > complex, but if well designed, its use can be, and should be, very simple. > > Implementation is done once. The complexity at that stage matters much less than > the end result usability. As a general principle, the more time one put in > design and development, the better the end result. Here, that means hooks being > useful, flexible, extensible, and reusable. Yup, that's pretty much what I'm advocating for. I'm thinking that it is something relatively simple like this: fstests/tests/hooks - directory containing library of hook scripts fstests/hooks/ - directory containing symlinks to hook scripts - kept under gitignore, as hooks are a runtime configurable state, not a global repository configured state. - symlink names indicate run target and order: $seq.{B,E}.X where: $seq is the test that the hook runs for use "global" for a hook that every test runs {B,E} indicates a test Begin or End hook X is an integer indicating run order, - 0 being the first script - Run in ascending numerical order Now if you want to add a 'trace-cmd clear' command to run before the start of a specific test, you do: $ echo trace-cmd clear > tests/hooks/trace-cmd-clear $ ln -s tests/hooks/trace-cmd-clear hooks/btrfs-160.B.0 And now when btrfs/160 starts, the Begin hook that you've set up is run.... If you want to do this for other tests, too, then just add symlinks for those tests. If you want all tests to run this trace hook, link global.B.0 to the hook script. Essentially, managing the hooks to run becomes an exercise in linking and unlinking external hook scripts. This means we can add curated hook scripts such as "use trace-cmd to trace all xfs trace points and then dump the report into $RESULTS_DIR/$seqres.traces" and hook them into to specific tests. The setup also allows stacked hook scripts, so we can have multiple monitoring options running at the same time (e.g. blktrace-scratch + trace-cmd-xfs) without having to write a custom hook scripts. > And one of the functionality of the hook setup could be "temporary, external > hook" for some very special case debugging as you seem to need that. What you > want to do and what Dave is proposing are not mutually exclusive. Yup, if you want a one-off throwaway hook script, then just add a file in to fstests/hooks and either name it as per above or link it to the test it should hook. Note: this just addresses the management side of running and curating hook scripts. There's a whole 'nother discussion about APIs to be had, but a curated hook library inside fstests is definitely a viable solution to that problem, too, because then the hook scripts can be changed at the same time the rest of fstests is changed to use the modified API.... This isn't a huge amount of work to implement, but we really need to decide how these hooks are going to be maintained, managed and curated before we go much further... Cheers, Dave.
On Thu, Jul 22, 2021 at 09:28:30AM +1000, Dave Chinner wrote: > > I'm thinking that it is something relatively simple like this: > > fstests/tests/hooks > - directory containing library of hook scripts I'd suggest fstests/common/hooks instead, since the hook scripts aren't actually *tests* per so, but rather utility scripts, and common would be a better place for it, I think. > fstests/hooks/ > - directory containing symlinks to hook scripts This might be a good default, but it might be better if the location of the hook directory could be overridden via an environment variable. In some cases, instead of having run-time configuration inside the fstests directtory with .gitignore, it might be more convenient for it if were made available externally (for example, via a 9p file system in a case where tests are being run via KVM using a rootfs test image with qmeu's snapshot mode so the hook directory could be supplied from the host). - Ted
On Thu, Jul 22, 2021 at 10:41:29AM -0400, Theodore Ts'o wrote: > On Thu, Jul 22, 2021 at 09:28:30AM +1000, Dave Chinner wrote: > > > > I'm thinking that it is something relatively simple like this: > > > > fstests/tests/hooks > > - directory containing library of hook scripts > > I'd suggest fstests/common/hooks instead, since the hook scripts > aren't actually *tests* per so, but rather utility scripts, and common > would be a better place for it, I think. True, but I don't think common/ is the right place, either, because that's for common test infrastructure. I only just looked, but there's a lib/ directory in fstests. lib/hooks seems like the right place for this, and if I had of looked yesterday I would have put it there from the start. :/ Is that an acceptible location? > > fstests/hooks/ > > - directory containing symlinks to hook scripts > > This might be a good default, but it might be better if the location > of the hook directory could be overridden via an environment variable. > In some cases, instead of having run-time configuration inside the > fstests directtory with .gitignore, it might be more convenient for it > if were made available externally (for example, via a 9p file system > in a case where tests are being run via KVM using a rootfs test image > with qmeu's snapshot mode so the hook directory could be supplied from > the host). Yup, that's easy enough to do. We can do it exactly the same way we allow RESULT_BASE to point the results to a user defined directory. Cheers, Dave.
On Fri, Jul 23, 2021 at 08:21:50AM +1000, Dave Chinner wrote: > On Thu, Jul 22, 2021 at 10:41:29AM -0400, Theodore Ts'o wrote: > > On Thu, Jul 22, 2021 at 09:28:30AM +1000, Dave Chinner wrote: > > > > > > I'm thinking that it is something relatively simple like this: > > > > > > fstests/tests/hooks > > > - directory containing library of hook scripts > > > > I'd suggest fstests/common/hooks instead, since the hook scripts > > aren't actually *tests* per so, but rather utility scripts, and common > > would be a better place for it, I think. > > True, but I don't think common/ is the right place, either, because > that's for common test infrastructure. I only just looked, but > there's a lib/ directory in fstests. lib/hooks seems like the right > place for this, and if I had of looked yesterday I would have put it > there from the start. :/ > > Is that an acceptible location? Sounds good to me! > > > fstests/hooks/ > > > - directory containing symlinks to hook scripts > > > > This might be a good default, but it might be better if the location > > of the hook directory could be overridden via an environment variable. > > In some cases, instead of having run-time configuration inside the > > fstests directtory with .gitignore, it might be more convenient for it > > if were made available externally (for example, via a 9p file system > > in a case where tests are being run via KVM using a rootfs test image > > with qmeu's snapshot mode so the hook directory could be supplied from > > the host). > > Yup, that's easy enough to do. We can do it exactly the same way we > allow RESULT_BASE to point the results to a user defined directory. Excellent, thanks! - Ted
On Fri, Jul 23, 2021 at 08:21:50AM +1000, Dave Chinner wrote: > On Thu, Jul 22, 2021 at 10:41:29AM -0400, Theodore Ts'o wrote: > > On Thu, Jul 22, 2021 at 09:28:30AM +1000, Dave Chinner wrote: > > > > > > I'm thinking that it is something relatively simple like this: > > > > > > fstests/tests/hooks > > > - directory containing library of hook scripts > > > > I'd suggest fstests/common/hooks instead, since the hook scripts > > aren't actually *tests* per so, but rather utility scripts, and common > > would be a better place for it, I think. Agreed, then we could avoid naming hooks dir as "Hooks". > > True, but I don't think common/ is the right place, either, because > that's for common test infrastructure. I only just looked, but > there's a lib/ directory in fstests. lib/hooks seems like the right > place for this, and if I had of looked yesterday I would have put it > there from the start. :/ > > Is that an acceptible location? Right now the lib dir contains mainly c libs used by test binaries, it looks like part of the build infrastructure to me. OTOH, I think common/hooks is fine, as hooks is part of the test harness to me. Or maybe tools/hooks is another choice? Thanks, Eryu > > > > fstests/hooks/ > > > - directory containing symlinks to hook scripts > > > > This might be a good default, but it might be better if the location > > of the hook directory could be overridden via an environment variable. > > In some cases, instead of having run-time configuration inside the > > fstests directtory with .gitignore, it might be more convenient for it > > if were made available externally (for example, via a 9p file system > > in a case where tests are being run via KVM using a rootfs test image > > with qmeu's snapshot mode so the hook directory could be supplied from > > the host). > > Yup, that's easy enough to do. We can do it exactly the same way we > allow RESULT_BASE to point the results to a user defined directory. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/.gitignore b/.gitignore index 2d72b064..99905ff9 100644 --- a/.gitignore +++ b/.gitignore @@ -201,3 +201,7 @@ tags # cscope files cscope.* ncscope.* + +# hook scripts +/hooks/start.hook +/hooks/end.hook diff --git a/README.hooks b/README.hooks new file mode 100644 index 00000000..be92a7d7 --- /dev/null +++ b/README.hooks @@ -0,0 +1,72 @@ +To run extra commands before and after each test case, there is the +'hooks/start.hook' and 'hooks/end.hook' files for such usage. + +Some notes for those two hooks: + +- Both hook files needs to be executable + Or they will just be ignored + +- Stderr and stdout will be redirected to "$seqres.full" + With extra separator to distinguish the hook output with real + test output + + Thus if any of the hook is specified, all tests will generate + "$seqres.full" which may increase the disk usage for results. + +- Error in hooks script will be ignored completely + +- Environment variable "$HOOK_TEMP" will be exported for both hooks + And the variable will be ensured not to change for both hooks. + + Thus it's possible to store temporary values between the two hooks, + like pid. + +- Start hook has only one parameter passed in + $1 is "$seq" from "check" script. The content will the path of current + test case. E.g "tests/btrfs/001" + +- End hook has two parameters passed in + $1 is the same as start hook. + $2 is the return value of the test case. + NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak + check. + +The very basic test hooks would look like: + +hooks/start.hook: + #!/bin/bash + seq=$1 + echo "HOOK_TMP=$HOOK_TMP" + echo "seq=$seq" + +hooks/end.hook: + #!/bin/bash + seq=$1 + sts=$2 + echo "HOOK_TMP=$HOOK_TMP" + echo "seq=$seq" + echo "sts=$sts" + +Then run test btrfs/001 and btrfs/002, their "$seqres.full" would look like: + +result/btrfs/001.full: + === Running start hook === + HOOK_TMP=/tmp/78973.hook + seq=tests/btrfs/001 + === Start hook finished === + === Running end hook === + HOOK_TMP=/tmp/78973.hook + seq=tests/btrfs/001 + sts=0 + === End hook finished === + +result/btrfs/002.full: + === Running start hook === + HOOK_TMP=/tmp/78973.hook + seq=tests/btrfs/002 + === Start hook finished === + === Running end hook === + HOOK_TMP=/tmp/78973.hook + seq=tests/btrfs/002 + sts=0 + === End hook finished === diff --git a/check b/check index bb7e030c..f24906f5 100755 --- a/check +++ b/check @@ -846,6 +846,10 @@ function run_section() # to be reported for each test (echo 1 > $DEBUGFS_MNT/clear_warn_once) > /dev/null 2>&1 + # Remove previous $seqres.full before start hook + rm -f $seqres.full + + _run_start_hook if [ "$DUMP_OUTPUT" = true ]; then _run_seq 2>&1 | tee $tmp.out # Because $? would get tee's return code @@ -854,6 +858,7 @@ function run_section() _run_seq >$tmp.out 2>&1 sts=$? fi + _run_end_hook if [ -f core ]; then _dump_err_cont "[dumped core]" diff --git a/common/preamble b/common/preamble index 66b0ed05..41a12060 100644 --- a/common/preamble +++ b/common/preamble @@ -56,8 +56,4 @@ _begin_fstest() _register_cleanup _cleanup . ./common/rc - - # remove previous $seqres.full before test - rm -f $seqres.full - } diff --git a/common/rc b/common/rc index d4b1f21f..ec434aa5 100644 --- a/common/rc +++ b/common/rc @@ -4612,6 +4612,49 @@ _require_names_are_bytes() { esac } +_run_start_hook() +{ + if [[ ! -d "hooks" ]]; then + return + fi + + if [[ ! -x "hooks/start.hook" ]]; then + return + fi + + # Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp", + # so we won't overwrite any existing $tmp.* files + export HOOK_TMP=$tmp.hook + + echo "=== Running start hook ===" >> $seqres.full + # $1 is alwasy $seq + ./hooks/start.hook $seq >> $seqres.full 2>&1 + echo "=== Start hook finished ===" >> $seqres.full + unset HOOK_TMP +} + +_run_end_hook() +{ + if [[ ! -d "hooks" ]]; then + return + fi + + if [[ ! -x "hooks/end.hook" ]]; then + return + fi + + # Export $HOOK_TMP for hooks, here we add ".hook" suffix to "$tmp", + # so we won't overwrite any existing $tmp.* files + export HOOK_TMP=$tmp.hook + + echo "=== Running end hook ===" >> "$seqres.full" + # $1 is alwasy $seq + # $2 is alwasy $sts + ./hooks/end.hook $seq $sts >> "$seqres.full" 2>&1 + echo "=== End hook finished ===" >> "$seqres.full" + unset HOOK_TMP +} + init_rc ################################################################################
This patch will allow fstests to run custom hooks before and after each test case. These hooks will need to follow requirements: - Both hook files needs to be executable Or they will just be ignored - Stderr and stdout will be redirected to "$seqres.full" With extra separator to distinguish the hook output with real test output Thus if any of the hook is specified, all tests will generate "$seqres.full" which may increase the disk usage for results. - Error in hooks script will be ignored completely - Environment variable "$HOOK_TEMP" will be exported for both hooks And the variable will be ensured not to change for both hooks. Thus it's possible to store temporary values between the two hooks, like pid. - Start hook has only one parameter passed in $1 is "$seq" from "check" script. The content will the path of current test case. E.g "tests/btrfs/001" - End hook has two parameters passed in $1 is the same as start hook. $2 is the return value of the test case. NOTE: $2 doesn't take later golden output mismatch check nor dmesg/kmemleak check. For more info, please refer to "README.hooks". Also update .gitignore to ignore "hooks/start.hook" and "hooks/end.hook" so that one won't accidentally submit the debug hook. Signed-off-by: Qu Wenruo <wqu@suse.com> --- Instead of previous attempt to manually utilize sysfs interface of ftrace, this time just add some hooks to allow one to do whatever they want. RFC for how everything should be passed to hooks. Currently it's using a mixed method, $seq/$sts is passed as paramaters, while HOOK_TMP is passed as environmental variable. Not sure what's the recommended way for hooks. --- .gitignore | 4 +++ README.hooks | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ check | 5 ++++ common/preamble | 4 --- common/rc | 43 +++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 README.hooks