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

Fix zfs_allow_004_pos #7267

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Conversation

dinatale2
Copy link
Contributor

Description

Datasets can be busy when calling zfs destroy. Introduce
a helper function to destroy datasets and use it to destroy
datasets in zfs_allow_004_pos.

Motivation and Context

Fix failing tests.

How Has This Been Tested?

Locally on a CentOS 7 VM.

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:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

fi

if is_global_zone ; then
if datasetexists "$pool" ; then
Copy link
Contributor

Choose a reason for hiding this comment

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

s/$pool/$dataset

@dinatale2 dinatale2 force-pushed the zfs_allow_004_pos-fix branch from d44d3aa to a0e603b Compare March 5, 2018 18:58
@chrisrd
Copy link
Contributor

chrisrd commented Mar 5, 2018

Looks like that should be a nice cleanup of some spurious test failures!

There was one in zfs_promote_008_pos: #7176 (comment)

It looks like all log_must zfs destroy in zfs-tests/tests/functional/cli_root/zfs_promote/ could/should be replaced by destroy_dataset as they're all in cleanup functions. I.e.:

sed -ri '
  /function cleanup/,/}/ {
    s/log_must zfs destroy (-[^[:space:]]+) ([^[:space:]]+)[[:space:]]*$/destroy_dataset "\2" "\1"/;
    s/log_must zfs destroy ([^[:space:]]+)[[:space:]]*$/destroy_dataset "\1"/;
  }
' tests/zfs-tests/tests/functional/cli_root/zfs_promote/*

In fact, that could likely be run over the whole test suite, but that would require careful manual inspection.

@behlendorf
Copy link
Contributor

@chrisrd yes, that's definitely our hope. It looks like zfs_destroy_002 would also benefit from calling destroy_dataset.

http://build.zfsonlinux.org/builders/Ubuntu%2017.10%20x86_64%20%28TEST%29/builds/633/steps/shell_8/logs/summary

@dinatale2 can you extend this to also cover zfs_promote_008_pos and zfs_destroy_002. Then we can consider applying it more broadly as we observe any remaining failures like this.

@dinatale2 dinatale2 force-pushed the zfs_allow_004_pos-fix branch from a0e603b to 0572564 Compare March 5, 2018 23:15
@openzfs openzfs deleted a comment from behlendorf Mar 5, 2018
@dinatale2
Copy link
Contributor Author

@chrisrd @behlendorf Refreshed to include zfs_promote_008_pos and zfs_destroy_002.

{
typeset dataset=$1
typeset mtpt
typeset args="-f"
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably shouldn't default to -f, instead how about typeset args=${2:-""}. Then you can drop the args assignment below, and callers won't need to pass a second argument unless requesting thing the -f behavior.

@dinatale2 dinatale2 force-pushed the zfs_allow_004_pos-fix branch from 0572564 to 7518110 Compare March 6, 2018 00:49
Datasets can be busy when calling zfs destroy. Introduce
a helper function to destroy datasets and use it to destroy
datasets in zfs_allow_004_pos, zfs_promote_008_pos, and
zfs_destroy_002_pos.

Signed-off-by: Giuseppe Di Natale <[email protected]>
@dinatale2 dinatale2 force-pushed the zfs_allow_004_pos-fix branch from 7518110 to 5026d44 Compare March 6, 2018 00:50
@codecov
Copy link

codecov bot commented Mar 6, 2018

Codecov Report

Merging #7267 into master will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7267      +/-   ##
==========================================
+ Coverage   76.35%   76.54%   +0.18%     
==========================================
  Files         327      327              
  Lines      103874   103866       -8     
==========================================
+ Hits        79314    79500     +186     
+ Misses      24560    24366     -194
Flag Coverage Δ
#kernel 76.21% <ø> (+0.03%) ⬆️
#user 65.9% <ø> (+0.17%) ⬆️

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 2705ebf...5026d44. Read the comment docs.

@behlendorf behlendorf merged commit c7b55e7 into openzfs:master Mar 6, 2018
@dinatale2 dinatale2 deleted the zfs_allow_004_pos-fix branch March 7, 2018 04:08
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request May 4, 2018
Datasets can be busy when calling zfs destroy. Introduce
a helper function to destroy datasets and use it to destroy
datasets in zfs_allow_004_pos, zfs_promote_008_pos, and
zfs_destroy_002_pos.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Giuseppe Di Natale <[email protected]>
Closes openzfs#7224
Closes openzfs#7246
Closes openzfs#7249
Closes openzfs#7267
tonyhutter pushed a commit that referenced this pull request May 10, 2018
Datasets can be busy when calling zfs destroy. Introduce
a helper function to destroy datasets and use it to destroy
datasets in zfs_allow_004_pos, zfs_promote_008_pos, and
zfs_destroy_002_pos.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Giuseppe Di Natale <[email protected]>
Closes #7224
Closes #7246
Closes #7249
Closes #7267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants