Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test Compression Algorithm Selection Refactor #9645

Merged
merged 1 commit into from
Dec 4, 2019

Conversation

PrivatePuffin
Copy link
Contributor

@PrivatePuffin PrivatePuffin commented Nov 28, 2019

This PR is based on my previous work in #9626 trying to find a more standardised way to select compression algorithms for the test suit.

TL:DR:

  • Removes duplicate algorithms from compression tests
  • Refactors compression algorithm selection for tests to use one uniform format

Signed-off-by: Kjeld Schouten-Lebbing [email protected]

Motivation and Context

During my work on investigating the compression test suite for #8941 a few things became appearant:

  1. There are a multitude of different ways/arrays/variables containing compression algorithms for tests
  2. Already some algorithms seem to have skipped over certain tests because of this
  3. Listing multiple (gzip) levels leads to duplicate and decreases the chance of other algorithms being choosen
  4. The decreased chance for different algorithms increases a lot when other "level based" algorithms get added
  5. Listing aliasses of algorithms leads to duplicate tests
  6. Somehow someone had the idea to use a function to simply pass array values to a for loop.

Description

This PR refactors it all.

  • The only compression algorithm list left is the one inside properties.shlib
  • It removes all seperate levels from the compression algorithms list for tests, except one (the default)
  • It removes the on alias for the selection of tests, in favor of lz4 to prevent mistakes switching default algorithm
  • It removes random algorithm selection, if the new array is just 1 or 2 longer than the previous amount of random draws
  • It removes the function onder libtest.shlib that was used to pass compresion algorithms (now directly uses the array in properties.shlib instead)
  • As a consequence, all tests now use the right set of algorithms
  • Also cleans the copyright notices a bit, layout was not always the best

One list to use them and in compression store them

Is * taken into account aka FAQ?

Q: Can't test failure of different levels slip past now?
A: Mostly levels are similair enough to the default to be found out just by using the default. If that's not the case a seperate test testing the different levels one time is more efficient than doing so for all tests.

Q: Can't test failure of aliasses slip past now?
A: Aliasses already have their own tests script or (in case of on) still get used a few times directly. So failures shouldn't slip past.

Q: Don't we need more randomised selection again when more compression algorithms get added?
A: If the amount of algorithms in the list increases byond 8 or 9, 1 or 2 extra random draws might be needed. Considering the current number and the (slow) speed of new algorithms being added, this will suffice for quite a while

Q: Doesn't adding the same complete selection of algorithms increase test runtime?
_A: In scenario's where some compression algorithms where left out and no randomisation is used, it might take a little longer. However the same subset of algorithms should already have been used and, by decreasing the amount of gzip-based algorithms in the selection, in general less performance intensive tests are being used. Net it will stay about the same or a little faster, but in general the test will be more repeatable and comparable.

How Has This Been Tested?

I manually checked if all tests actually used the algorithms referenced and they do.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@PrivatePuffin
Copy link
Contributor Author

PrivatePuffin commented Nov 28, 2019

@c0d3z3r0
This is just a quick draft, But I guess this is more like it...

@behlendorf
This is what I had in mind when closing #9626

@PrivatePuffin PrivatePuffin force-pushed the Comp-Sel-Refactor branch 19 times, most recently from 23dd980 to c68580f Compare November 29, 2019 20:00
@PrivatePuffin PrivatePuffin marked this pull request as ready for review November 29, 2019 20:01
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 30, 2019
@PrivatePuffin
Copy link
Contributor Author

edit to description
Forgot to change the "how has this been tested"...

I manually checked all edited tests to make sure they actually got the compression algorithms referenced.

Also nice to note for review:
refactor uses one of the many different reference styles used by @dankimmel from a700472 and uses that one to unify compression algorithm references (including other styles included)

So this reference style is not new, it just unifies different reference styles.

Copy link
Contributor

@c0d3z3r0 c0d3z3r0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides some nitpicks

@PrivatePuffin PrivatePuffin requested a review from jwk404 December 3, 2019 19:59
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When giving this one last look a noticed a cosmetic style issue which cstyle appears to have missed. Aside from that, looks good.

tests/zfs-tests/include/libtest.shlib Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #9645 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9645      +/-   ##
==========================================
- Coverage   79.32%   79.17%   -0.15%     
==========================================
  Files         418      418              
  Lines      123562   123531      -31     
==========================================
- Hits        98010    97807     -203     
- Misses      25552    25724     +172
Flag Coverage Δ
#kernel 79.87% <ø> (-0.06%) ⬇️
#user 66.79% <ø> (-0.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a08977...0574409. Read the comment docs.

- Moves compression algorithms for tests to properties.shlib
- Removes all compression algorithms levels from general tests
- Replaces on with lz4 for compression tests
- Removes random algorithm selection, if not needed
- Cleans copyright header formating

Signed-off-by: Kjeld Schouten-Lebbing <[email protected]>
@PrivatePuffin
Copy link
Contributor Author

@behlendorf
Squashed and ready for re-review :)

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 4, 2019
@behlendorf behlendorf merged commit 618b6ad into openzfs:master Dec 4, 2019
PrivatePuffin pushed a commit to PrivatePuffin/zfs that referenced this pull request Dec 4, 2019
This sets send_realloc_files.ksh to use properties.shlib
(like the other compression related tests)

It was missing from openzfs#9645

Signed-off-by: Kjeld Schouten-Lebbing <[email protected]>
PrivatePuffin pushed a commit to PrivatePuffin/zfs that referenced this pull request Dec 4, 2019
This sets send_realloc_files.ksh to use properties.shlib
(like the other compression related tests)

It was missing from openzfs#9645

Signed-off-by: Kjeld Schouten-Lebbing <[email protected]>
behlendorf pushed a commit that referenced this pull request Dec 6, 2019
This sets send_realloc_files.ksh to use properties.shlib
(like the other compression related tests)

It was missing from #9645

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Kjeld Schouten-Lebbing <[email protected]>
Issue #9645
Closes #9679
@PrivatePuffin PrivatePuffin deleted the Comp-Sel-Refactor branch December 19, 2019 18:58
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 26, 2019
This sets send_realloc_files.ksh to use properties.shlib
(like the other compression related tests)

It was missing from openzfs#9645

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Kjeld Schouten-Lebbing <[email protected]>
Issue openzfs#9645
Closes openzfs#9679
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
This sets send_realloc_files.ksh to use properties.shlib
(like the other compression related tests)

It was missing from openzfs#9645

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Kjeld Schouten-Lebbing <[email protected]>
Issue openzfs#9645
Closes openzfs#9679
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
This sets send_realloc_files.ksh to use properties.shlib
(like the other compression related tests)

It was missing from #9645

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Kjeld Schouten-Lebbing <[email protected]>
Issue #9645
Closes #9679
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants