[PATCH RFC] fstests: allow running custom hooks

[PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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
 		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

Re: [PATCH RFC] fstests: allow running custom hooks

[Theodore Y. Ts'o]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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
>

Re: [PATCH RFC] fstests: allow running custom hooks

[Theodore Y. Ts'o]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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
> 

Re: [PATCH RFC] fstests: allow running custom hooks

[Eryu Guan]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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.
>

Re: [PATCH RFC] fstests: allow running custom hooks

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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.
>

Re: [PATCH RFC] fstests: allow running custom hooks

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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.
>

Re: [PATCH RFC] fstests: allow running custom hooks

[Eryu Guan]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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
>

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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
>>

Re: [PATCH RFC] fstests: allow running custom hooks

[Theodore Y. Ts'o]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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
>

Re: [PATCH RFC] fstests: allow running custom hooks

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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.
>

Re: [PATCH RFC] fstests: allow running custom hooks

[Damien Le Moal]

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.
>>
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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.
>>>
>>
> 
> 

Re: [PATCH RFC] fstests: allow running custom hooks

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC] fstests: allow running custom hooks

[Theodore Ts'o]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC] fstests: allow running custom hooks

[Theodore Ts'o]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Eryu Guan]

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

Re: [PATCH RFC] fstests: allow running custom hooks

[Darrick J. Wong]

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
> 

Re: [PATCH RFC] fstests: allow running custom hooks

[Qu Wenruo]

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
>>
>