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

ZTS: Clean up do_vol_test in zfs_copies tests #9286

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

ghost
Copy link

@ghost ghost commented Sep 4, 2019

Motivation and Context

This test was stuck in an infinite loop, which caused me to stare at the code for quite a while to figure out why.

Description

Get rid of the get_used_prop function. get_prop used works fine.

Fix the comment describing the function parameters. The type does not have a default, and mntp is also used for ext2.

Rename the variable for the number of copies from copy to copies.

Use a case statement to match the type parameter, order the cases alphabetically, and add a little sanity checking for good measure.

Use eval to make sure the output of commands is silenced rather than the log messages when redirecting output to /dev/null.

Simplify cases where zfs requires special behavior.

Don't allow the test to loop forever in the event space usage does not change. Bail out of the loop and fail after an arbitrary number of iterations.

Add more information to the log message when the test fails, to help debugging.

How Has This Been Tested?

./scripts/zfs-tests.sh -vxT zfs_copies on Debian 10.

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:

Get rid of the `get_used_prop` function. `get_prop used` works fine.

Fix the comment describing the function parameters. The type does not
have a default, and mntp is also used for ext2.

Rename the variable for the number of copies from `copy` to `copies`.

Use a `case` statement to match the type parameter, order the cases
alphabetically, and add a little sanity checking for good measure.

Use eval to make sure the output of commands is silenced rather than
the log messages when redirecting output to /dev/null.

Simplify cases where zfs requires special behavior.

Don't allow the test to loop forever in the event space usage does not
change. Bail out of the loop and fail after an arbitrary number of
iterations.

Add more information to the log message when the test fails, to help
debugging.

Signed-off-by: Ryan Moeller <[email protected]>
@ghost ghost added the Status: Code Review Needed Ready for review and testing label Sep 4, 2019
@ghost ghost changed the title Clean up do_vol_test in zfs_copies tests ZTS: Clean up do_vol_test in zfs_copies tests Sep 4, 2019
@behlendorf behlendorf requested a review from jwk404 September 4, 2019 23:36
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.

Thanks for making this test more robust. Did you ever determine why it was spinning in your local testing? I presume it must have been due to the space not changing, so with this change you'd now get a failure.

@ghost
Copy link
Author

ghost commented Sep 5, 2019

Indeed, the space used was not changing. I don't think I figured out exactly the reason for that, but broadly it was because our zvol integration was broken / incomplete.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 9, 2019
@behlendorf behlendorf merged commit 43f4495 into openzfs:master Sep 9, 2019
@ghost ghost deleted the zfs_copies-cleanup branch September 10, 2019 03:26
mattmacy pushed a commit to zfsonfreebsd/ZoF that referenced this pull request Sep 10, 2019
Get rid of the `get_used_prop` function. `get_prop used` works fine.

Fix the comment describing the function parameters. The type does not
have a default, and mntp is also used for ext2.

Rename the variable for the number of copies from `copy` to `copies`.

Use a `case` statement to match the type parameter, order the cases
alphabetically, and add a little sanity checking for good measure.

Use eval to make sure the output of commands is silenced rather than
the log messages when redirecting output to /dev/null.

Simplify cases where zfs requires special behavior.

Don't allow the test to loop forever in the event space usage does not
change. Bail out of the loop and fail after an arbitrary number of
iterations.

Add more information to the log message when the test fails, to help
debugging.

Reviewed-by: John Kennedy <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#9286
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 24, 2019
Get rid of the `get_used_prop` function. `get_prop used` works fine.

Fix the comment describing the function parameters. The type does not
have a default, and mntp is also used for ext2.

Rename the variable for the number of copies from `copy` to `copies`.

Use a `case` statement to match the type parameter, order the cases
alphabetically, and add a little sanity checking for good measure.

Use eval to make sure the output of commands is silenced rather than
the log messages when redirecting output to /dev/null.

Simplify cases where zfs requires special behavior.

Don't allow the test to loop forever in the event space usage does not
change. Bail out of the loop and fail after an arbitrary number of
iterations.

Add more information to the log message when the test fails, to help
debugging.

Reviewed-by: John Kennedy <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#9286
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
Get rid of the `get_used_prop` function. `get_prop used` works fine.

Fix the comment describing the function parameters. The type does not
have a default, and mntp is also used for ext2.

Rename the variable for the number of copies from `copy` to `copies`.

Use a `case` statement to match the type parameter, order the cases
alphabetically, and add a little sanity checking for good measure.

Use eval to make sure the output of commands is silenced rather than
the log messages when redirecting output to /dev/null.

Simplify cases where zfs requires special behavior.

Don't allow the test to loop forever in the event space usage does not
change. Bail out of the loop and fail after an arbitrary number of
iterations.

Add more information to the log message when the test fails, to help
debugging.

Reviewed-by: John Kennedy <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#9286
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
Get rid of the `get_used_prop` function. `get_prop used` works fine.

Fix the comment describing the function parameters. The type does not
have a default, and mntp is also used for ext2.

Rename the variable for the number of copies from `copy` to `copies`.

Use a `case` statement to match the type parameter, order the cases
alphabetically, and add a little sanity checking for good measure.

Use eval to make sure the output of commands is silenced rather than
the log messages when redirecting output to /dev/null.

Simplify cases where zfs requires special behavior.

Don't allow the test to loop forever in the event space usage does not
change. Bail out of the loop and fail after an arbitrary number of
iterations.

Add more information to the log message when the test fails, to help
debugging.

Reviewed-by: John Kennedy <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #9286
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.

2 participants