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

Znapzendzetup list recursive #439

Merged
merged 50 commits into from Oct 18, 2019

Conversation

@jimklimov
Copy link
Contributor

jimklimov commented Oct 14, 2019

So I was debugging my laptop backup setup today, and was annoyed that I must either znapzendzetup list source datasets that I know to have been configured by name, one by one, or suffer the whole-system dataset listing (with USB HDD taking many minutes to give the list)...

And so I scratched a few itches:

  • added ability to list several named dataset configs in one command call
  • added ability to recurse into children of named dataset(s)
  • added a fallback for systems whose memory vs ZFS tree sizing won't permit listing all the child dataset attributes :-)
  • extended ZFS.pm routines getDataSetProperties() and listDataSets() with ability to inspect not only a single string $dataSet value, but also arrays of strings, to reduce the number of (slow) calls into zfs command
  • added optional debugging and features (for lowmemRecurse) support in znapzendzetup

Tested with various combinations locally, seems to work for stuff I typed :)

But CI selftests not yet updated, probably zfs mockup needs some love :(

jimklimov added 7 commits Oct 14, 2019
Solution: extend getDataSetProperties() and getBackupSet()
with recursion ability, than can be optionally invoked from
`znapzendzetup list -r poolname(/dataset)`.

Try to call `zfs` as few times as possible, listing the ZFS
dataset attributes recursively (if asked to at all) starting
from the named dataset.

Signed-off-by: Jim Klimov <jimklimov@gmail.com>
Solution: Extend getDataSetProperties() and getBackupSet()
with support for passing a $dataSet argument as a perl array,
as well as a currently supported string, and so allow to
`znapzendzetup list (-r) ds1 ds2 ...`.

Compatible with recently added recursion.

Signed-off-by: Jim Klimov <jimklimov@gmail.com>
Solution: add a --features=lowmemRecurse support to first list the
datasets recursively, and then one by one inspect their attributes.
This is slower (more calls to `zfs`) but should be more reliable on
systems that are mis-dimensioned to fail otherwise.

Signed-off-by: Jim Klimov <jimklimov@gmail.com>
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 14, 2019

Coverage Status

Coverage increased (+0.9%) to 88.414% when pulling 8b20412 on jimklimov:znapzendzetup-list-recursive into 95f9e5a on oetiker:master.

if (ref($dataSet) eq 'ARRAY') {
print STDERR "=== getDataSetProperties(): Is array...\n" if $self->debug;
if ($self->lowmemRecurse && $recurse) {
my $listds = $self->listDataSets(undef, $dataSet, $recurse);

This comment has been minimized.

Copy link
@jimklimov

jimklimov Oct 14, 2019

Author Contributor

Also a question for reviewers : in the fallback logic here and a bit below, I pass undef for the $remote meaning to look at the local system. First it seemed to match the earlier call when used without any arguments, or verbatim use of the $dataSet string when with arguments.

However here we can pass several args (as an array), some of which potentially could have the user@remote:data/set format... should I bother about this possibility now? :-) I do not think this change breaks any use-case fundamentally, but maybe others have more visibility and memory :)

This comment has been minimized.

Copy link
@oetiker

oetiker Oct 15, 2019

Owner

I would suggest to have as little ssh magic on the commandline and rather encurrage people to create appropriate .ssh/config files and then work with remote names matching these definitions.

@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 14, 2019

Oh wow, seeing green CI builds at https://travis-ci.org/jimklimov/znapzend/builds/597797056 now :)

Gotta go get some sleep to not break anything further :)

@jimklimov jimklimov force-pushed the jimklimov:znapzendzetup-list-recursive branch from 30af21d to 199f194 Oct 14, 2019
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 14, 2019

Can't make sense of some logs... says tests are green but by coverage seems something failed mid-way.

And also this:

# zfs destroy remote/destination@2019-10-14-184000,2019-10-14-184200,2019-10-14-184300,2019-10-14-184400,2019-10-14-184500,2019-10-14-184600,2019-10-14-184700,2019-10-14-184800,2019-10-14-184900,2019-10-14-185100,2019-10-14-185200,2019-10-14-185300,2019-10-14-185400,2019-10-14-185500,2019-10-14-185600,2019-10-14-185700,2019-10-14-185800,2019-10-14-185900,2019-10-14-190200,2019-10-14-190300,2019-10-14-190400,2019-10-14-190600,2019-10-14-190700,2019-10-14-190800,2019-10-14-190900,2019-10-14-191100,2019-10-14-191200,2019-10-14-191300,2019-10-14-191400,2019-10-14-191600,2019-10-14-191700,2019-10-14-191800,2019-10-14-191900

# zfs list -H -o name -t filesystem,volume backup/destfail

[Mon Oct 14 19:40:13 2019] [warn] destination 'backup/destfail' does not exist or is offline. ignoring it for this round...
[Mon Oct 14 19:40:13 2019] [warn] ERROR: suspending cleanup source dataset because at least one send task failed
[Mon Oct 14 19:40:13 2019] [info] done with backupset tank/source in 3 seconds
[Mon Oct 14 19:40:13 2019] [info] znapzend (PID=6792) initialized -- resuming normal operations.
not ok 4 - znapzend dies with no backup sets defined or enabled at startup
#   Failed test 'znapzend dies with no backup sets defined or enabled at startup'
#   at ./t/znapzend.t line 69.
# expecting: Regexp ((?^:No backup set defined or enabled))
# found: normal exit

So was it supposed to fail but didn't (and have I broken something?) or not?

@jimklimov jimklimov force-pushed the jimklimov:znapzendzetup-list-recursive branch from 199f194 to fa3f487 Oct 14, 2019
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 14, 2019

Seems I did...

=== getDataSetProperties(): Is array...
# zfs list -H -o name -t filesystem,volume -r tank/source
cannot open 'tank/source': dataset does not exist
=== getDataSetProperties(): List all...
# zfs list -H -o name -t filesystem,volume

at least somtehing

@jimklimov jimklimov force-pushed the jimklimov:znapzendzetup-list-recursive branch from fa3f487 to 041021d Oct 14, 2019
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 14, 2019

Seems the CI gods are satisfied now... though in the build log I see

ok 15 - znapzendzetup import --write
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 1 just after 15.

and locally the test 16 passes:

ok 15 - znapzendzetup import --write
=== getDataSetProperties():
...
ok 16 - znapzendzetup list --features=lowmemRecurse --debug --recursive tank/source
1..16
my %properties;
my @cmd = (@{$self->priv}, qw(zfs get -H -s local -o), 'property,value', 'all', $listElem);
my @cmd = (@{$self->priv}, qw(zfs get -H -s local));
push (@cmd, qw(-t), 'filesystem,volume');

This comment has been minimized.

Copy link
@jimklimov

jimklimov Oct 15, 2019

Author Contributor

Also for reviewers: did any users of the older implementation of getDataSetProperties() rely on its ability to query non-fs/vol datasets (e.g. ask for properties of snapshots, or of whatever future dataset types may be defined)?

It can be added back for non-recursive case of explicitly listed dataset(s) to query, but it explodes size- and time-wise if we recurse a pool with hundreds of thousands of snapshots and get attributes of those too.

This comment has been minimized.

Copy link
@oetiker

oetiker Oct 15, 2019

Owner

I am not aware of such a use ...

@oetiker oetiker requested review from hadfl and oetiker Oct 15, 2019
@jimklimov jimklimov referenced this pull request Oct 17, 2019
lib/ZnapZend/Config.pm Outdated Show resolved Hide resolved
lib/ZnapZend/Config.pm Outdated Show resolved Hide resolved
lib/ZnapZend/Config.pm Outdated Show resolved Hide resolved
lib/ZnapZend/Config.pm Outdated Show resolved Hide resolved
lib/ZnapZend/ZFS.pm Outdated Show resolved Hide resolved
oetiker and others added 2 commits Oct 18, 2019
Commit formatting suggestion from PR review.

Co-Authored-By: Tobias Oetiker <tobi@oetiker.ch>
jimklimov and others added 4 commits Oct 18, 2019
Commit formatting suggestion from PR review.

Co-Authored-By: Tobias Oetiker <tobi@oetiker.ch>
Commit formatting suggestion from PR review.

Co-Authored-By: Tobias Oetiker <tobi@oetiker.ch>
Commit formatting suggestion from PR review.

Co-Authored-By: Tobias Oetiker <tobi@oetiker.ch>
@jimklimov

This comment has been minimized.

Copy link
Contributor Author

jimklimov commented Oct 18, 2019

Thanks for that other merge, glued it to this one now, waiting for re-testing :)

@oetiker oetiker merged commit 47e9613 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 88.414%
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.