[v2,2/2] xfs/068: Verify actual file count instead of reported file count
diff mbox series

Message ID 20190127075057.25254-2-amir73il@gmail.com
State New
Headers show
Series
  • [v2,1/2] common/dump: do not override test cleanup trap
Related show

Commit Message

Amir Goldstein Jan. 27, 2019, 7:50 a.m. UTC
This test has the number of files/dirs created by xfsrestore hardcoded
in golden output.

When fsstress is added new ops, the number of files/dirs created with
the same random seed changes and this regularly breaks this test,
so when new fsstress ops are added they should be either added to the
dump test blacklist or golden output of this test needs to be ammended
to reflect the change.

The golden output includes only the file count reported by xfsrestore
and test does not even verify that this is the correct file count.
Instead, leave the golden output nuetral and explicitly verify that
file count before and after the test are the same.

With this change, the test becomes agnostic to fsstress ops and we
could also stop blacklisting clone/dedup/copy ops if we want.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/dump       |  7 +++++++
 tests/xfs/068     | 14 +++++++++++++-
 tests/xfs/068.out |  2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

Comments

Eryu Guan Feb. 3, 2019, 12:40 p.m. UTC | #1
On Sun, Jan 27, 2019 at 09:50:57AM +0200, Amir Goldstein wrote:
> This test has the number of files/dirs created by xfsrestore hardcoded
> in golden output.
> 
> When fsstress is added new ops, the number of files/dirs created with
> the same random seed changes and this regularly breaks this test,
> so when new fsstress ops are added they should be either added to the
> dump test blacklist or golden output of this test needs to be ammended
> to reflect the change.
> 
> The golden output includes only the file count reported by xfsrestore
> and test does not even verify that this is the correct file count.
> Instead, leave the golden output nuetral and explicitly verify that
> file count before and after the test are the same.
> 
> With this change, the test becomes agnostic to fsstress ops and we
> could also stop blacklisting clone/dedup/copy ops if we want.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks fine to me overall, thanks a lot for the fix! But I'd like an ACK
from XFS folks :)

> ---
>  common/dump       |  7 +++++++
>  tests/xfs/068     | 14 +++++++++++++-
>  tests/xfs/068.out |  2 +-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/common/dump b/common/dump
> index 89fa0391..f112fc37 100644
> --- a/common/dump
> +++ b/common/dump
> @@ -1514,6 +1514,13 @@ _check_quota_file()
>     _check_quota 'xfsdump_quotas' 'xfsdump_quotas_group' 'xfsdump_quotas_proj'
>  }
>  
> +_count_dumpdir_files()
> +{
> +	local ndirs=$(find $dump_dir -type d | wc -l)
> +	local nents=$(find $dump_dir | wc -l)
> +
> +	echo "$ndirs directories and $nents entries"
> +}
>  
>  # make sure this script returns success
>  /bin/true
> diff --git a/tests/xfs/068 b/tests/xfs/068
> index 7f5900fc..264a9e96 100755
> --- a/tests/xfs/068
> +++ b/tests/xfs/068
> @@ -30,12 +30,24 @@ _cleanup()
>  . ./common/rc
>  . ./common/dump
>  
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
>  # real QA test starts here
>  _supported_fs xfs
>  _supported_os Linux
>  
>  _create_dumpdir_stress_num 4096
> -_do_dump_restore
> +
> +echo -n "Before: " >> $seqres.full
> +_count_dumpdir_files | tee $tmp.before >> $seqres.full
> +
> +# filter out the file count, it changes as fsstress adds new operations
> +_do_dump_restore | sed -e "/entries processed$/s/[0-9][0-9]*/NUM/g"
> +
> +echo -n "After: " >> $seqres.full
> +_count_dumpdir_files | tee $tmp.after >> $seqres.full

The "Before" and "After" both count the file entries in $dump_dir, so
they should always be the same, we should count $restore_dir at "After"?

Thanks,
Eryu

> +diff -u $tmp.before $tmp.after
>  
>  # success, all done
>  exit
> diff --git a/tests/xfs/068.out b/tests/xfs/068.out
> index fa3a5523..2b276b77 100644
> --- a/tests/xfs/068.out
> +++ b/tests/xfs/068.out
> @@ -22,7 +22,7 @@ xfsrestore: session id: ID
>  xfsrestore: media ID: ID
>  xfsrestore: searching media for directory dump
>  xfsrestore: reading directories
> -xfsrestore: 383 directories and 1335 entries processed
> +xfsrestore: NUM directories and NUM entries processed
>  xfsrestore: directory post-processing
>  xfsrestore: restoring non-directory files
>  xfsrestore: restore complete: SECS seconds elapsed
> -- 
> 2.17.1
>
Amir Goldstein Feb. 3, 2019, 1:07 p.m. UTC | #2
On Sun, Feb 3, 2019 at 2:40 PM Eryu Guan <guaneryu@gmail.com> wrote:
>
> On Sun, Jan 27, 2019 at 09:50:57AM +0200, Amir Goldstein wrote:
> > This test has the number of files/dirs created by xfsrestore hardcoded
> > in golden output.
> >
> > When fsstress is added new ops, the number of files/dirs created with
> > the same random seed changes and this regularly breaks this test,
> > so when new fsstress ops are added they should be either added to the
> > dump test blacklist or golden output of this test needs to be ammended
> > to reflect the change.
> >
> > The golden output includes only the file count reported by xfsrestore
> > and test does not even verify that this is the correct file count.
> > Instead, leave the golden output nuetral and explicitly verify that
> > file count before and after the test are the same.
> >
> > With this change, the test becomes agnostic to fsstress ops and we
> > could also stop blacklisting clone/dedup/copy ops if we want.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Looks fine to me overall, thanks a lot for the fix! But I'd like an ACK
> from XFS folks :)
>
> > ---
> >  common/dump       |  7 +++++++
> >  tests/xfs/068     | 14 +++++++++++++-
> >  tests/xfs/068.out |  2 +-
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/dump b/common/dump
> > index 89fa0391..f112fc37 100644
> > --- a/common/dump
> > +++ b/common/dump
> > @@ -1514,6 +1514,13 @@ _check_quota_file()
> >     _check_quota 'xfsdump_quotas' 'xfsdump_quotas_group' 'xfsdump_quotas_proj'
> >  }
> >
> > +_count_dumpdir_files()
> > +{
> > +     local ndirs=$(find $dump_dir -type d | wc -l)
> > +     local nents=$(find $dump_dir | wc -l)
> > +
> > +     echo "$ndirs directories and $nents entries"
> > +}
> >
> >  # make sure this script returns success
> >  /bin/true
> > diff --git a/tests/xfs/068 b/tests/xfs/068
> > index 7f5900fc..264a9e96 100755
> > --- a/tests/xfs/068
> > +++ b/tests/xfs/068
> > @@ -30,12 +30,24 @@ _cleanup()
> >  . ./common/rc
> >  . ./common/dump
> >
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> >  # real QA test starts here
> >  _supported_fs xfs
> >  _supported_os Linux
> >
> >  _create_dumpdir_stress_num 4096
> > -_do_dump_restore
> > +
> > +echo -n "Before: " >> $seqres.full
> > +_count_dumpdir_files | tee $tmp.before >> $seqres.full
> > +
> > +# filter out the file count, it changes as fsstress adds new operations
> > +_do_dump_restore | sed -e "/entries processed$/s/[0-9][0-9]*/NUM/g"
> > +
> > +echo -n "After: " >> $seqres.full
> > +_count_dumpdir_files | tee $tmp.after >> $seqres.full
>
> The "Before" and "After" both count the file entries in $dump_dir, so
> they should always be the same, we should count $restore_dir at "After"?
>

Certainly. We should count $restore_dir/$dump_sdir.
I there is an ACK to concept, I will send a fix.

Thanks,
Amir.

Patch
diff mbox series

diff --git a/common/dump b/common/dump
index 89fa0391..f112fc37 100644
--- a/common/dump
+++ b/common/dump
@@ -1514,6 +1514,13 @@  _check_quota_file()
    _check_quota 'xfsdump_quotas' 'xfsdump_quotas_group' 'xfsdump_quotas_proj'
 }
 
+_count_dumpdir_files()
+{
+	local ndirs=$(find $dump_dir -type d | wc -l)
+	local nents=$(find $dump_dir | wc -l)
+
+	echo "$ndirs directories and $nents entries"
+}
 
 # make sure this script returns success
 /bin/true
diff --git a/tests/xfs/068 b/tests/xfs/068
index 7f5900fc..264a9e96 100755
--- a/tests/xfs/068
+++ b/tests/xfs/068
@@ -30,12 +30,24 @@  _cleanup()
 . ./common/rc
 . ./common/dump
 
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
 # real QA test starts here
 _supported_fs xfs
 _supported_os Linux
 
 _create_dumpdir_stress_num 4096
-_do_dump_restore
+
+echo -n "Before: " >> $seqres.full
+_count_dumpdir_files | tee $tmp.before >> $seqres.full
+
+# filter out the file count, it changes as fsstress adds new operations
+_do_dump_restore | sed -e "/entries processed$/s/[0-9][0-9]*/NUM/g"
+
+echo -n "After: " >> $seqres.full
+_count_dumpdir_files | tee $tmp.after >> $seqres.full
+diff -u $tmp.before $tmp.after
 
 # success, all done
 exit
diff --git a/tests/xfs/068.out b/tests/xfs/068.out
index fa3a5523..2b276b77 100644
--- a/tests/xfs/068.out
+++ b/tests/xfs/068.out
@@ -22,7 +22,7 @@  xfsrestore: session id: ID
 xfsrestore: media ID: ID
 xfsrestore: searching media for directory dump
 xfsrestore: reading directories
-xfsrestore: 383 directories and 1335 entries processed
+xfsrestore: NUM directories and NUM entries processed
 xfsrestore: directory post-processing
 xfsrestore: restoring non-directory files
 xfsrestore: restore complete: SECS seconds elapsed