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

Recursive destroy #386

Merged
merged 20 commits into from Oct 18, 2019

Conversation

@jimklimov
Copy link
Contributor

jimklimov commented Oct 11, 2018

This should take care of #385, but so far has not been tested on real systems ;)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 11, 2018

Coverage Status

Coverage increased (+1.8%) to 86.467% when pulling 980e3f1 on jimklimov:recursive-destroy into c319917 on oetiker:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 11, 2018

Coverage Status

Coverage increased (+0.9%) to 87.518% when pulling 1dfbb4c on jimklimov:recursive-destroy into 1154bc2 on oetiker:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 11, 2018

Coverage Status

Coverage increased (+1.8%) to 86.467% when pulling 980e3f1 on jimklimov:recursive-destroy into c319917 on oetiker:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 11, 2018

Coverage Status

Coverage increased (+1.8%) to 86.467% when pulling 980e3f1 on jimklimov:recursive-destroy into c319917 on oetiker:master.

…nation failure vs. SOME destination failure (as needed for source cleanup)
@jimklimov jimklimov force-pushed the jimklimov:recursive-destroy branch from 980e3f1 to 3517c6b Oct 11, 2018
…ng into their children
@jimklimov jimklimov force-pushed the jimklimov:recursive-destroy branch from 3517c6b to 530104c Oct 11, 2018
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 11, 2018

Tested on local system, wrinkled out some issues and optimized a bit, seems to work for me. Details about test setup and routine in #385.

Copy link
Owner

oetiker left a comment

looking good ...

@oetiker

This comment has been minimized.

Copy link
Owner

oetiker commented Oct 12, 2018

what would be really cool would be if you enhanced the test suite accordingly

…sub $createSnapshot
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 12, 2018

I am not actually sure how to go about auto-testing it at this time :\

I did git grep destroySnapshot and found one more use-case I missed before :) but otherwise there were no apparent hits under tests. A gmake check fails for me regardless - in the master branch as well...

@jimklimov jimklimov changed the title [TO TEST FIRST] Recursive destroy Recursive destroy Oct 15, 2018
jimklimov added 2 commits Nov 26, 2018
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 15, 2019

Bumped to align with current master.

Also read up today on ability to have resursive mode but certain datasets not enabled.

@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 15, 2019

Updated to align with current master. Every PR wants some love one year or another ;)

Also read today about an ability to have recursive backup plan, but have it not enabled on some child datasets. I wonder if this recursive mass-removal would contradict anything (e.g. cleaning up datasets that we did not intend to actively back up? good... To touch? bad...)

@oetiker

This comment has been minimized.

Copy link
Owner

oetiker commented Oct 15, 2019

any thoughts on how to add testing ?

@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 15, 2019

Probably a few lines in random files under t/ directory should do it. Just gotta carve them out :)

@jimklimov jimklimov force-pushed the jimklimov:recursive-destroy branch from 52d17f1 to 758034a Oct 17, 2019
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 17, 2019

So I somewhat cheated at first and covered the previously existing but not-covered code ;)

And went from there to cover some of the new paths as well.

@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 17, 2019

I anticipate that after merging #439, the coverage would also include large blocks about (cleaning) child datasets that inherit a backup plan but are not directly configured. I did not port it here because it largely depends on changes made for that PR, in both mock zfs and maybe the production code.

@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 17, 2019

Idea for after merging these PRs (and so taking advantage of the logical structure in that one) - also add tank/source child datasets that would have org.znapzend:enabled==off into t/zfs output, and so test -cover one more family of codepaths.

UPDATE: Added this feature in that PR.

@oetiker

This comment has been minimized.

Copy link
Owner

oetiker commented Oct 18, 2019

let me know when you deem the whole thing complete :) I will then review!

@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 18, 2019

I hope the two PRs are now complete (and beyond, with the testing cleanups) and should hopefully not conflict when merging :)

@oetiker oetiker merged commit 95f9e5a into oetiker:master Oct 18, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.9%) to 87.518%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.