Allow to set dst_N_enabled=off to comment away targets without deleting definitions

[jimklimov]

UPDATE: No longer "WIP": got selftests to pass and cover all changed code.

[coveralls]

Coverage increased (+0.2%) to 90.05% when pulling c8e75dc2c8c2d43f2722d35fabd526ab44fee379 on jimklimov:dst-disable into bfb68766ba5a13bcaba87292cb04191138bc9d3e on oetiker:master.

[jimklimov]

Interesting... I had some errors in fact for the management part, and the CI tests found them as not ok, and aborted it seems in mid-flight, but completed green :(

ok 15 - znapzendzetup import --write
# zfs list -H -o name -t filesystem,volume dst_0
not ok 16 - znapzendzetup enable-dst tank/source dst_0 - succeeds
#   Failed test 'znapzendzetup enable-dst tank/source dst_0 - succeeds'
#   at ./t/znapzendzetup.t line 113.
#          got: '0'
#     expected: '1'
not ok 17 - znapzendzetup enable-dst tank/source 0 - succeeds
#   Failed test 'znapzendzetup enable-dst tank/source 0 - succeeds'
#   at ./t/znapzendzetup.t line 114.
#          got: '0'
#     expected: '1'
# zfs list -H -o name -t filesystem,volume dst_1_badkey
ok 18 - znapzendzetup enable-dst tank/source dst_1_badkey - fails
# zfs list -H -o name -t filesystem,volume 1
ok 19 - znapzendzetup enable-dst tank/source 1 - fails
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 1 just after 19.
Devel::Cover: getting BEGIN block coverage
...
Devel::Cover: 100% - 1s taken
Devel::Cover: Writing coverage database to /home/travis/build/oetiker/znapzend/cover_db/runs/1573750225.9503.05495
---------------------- ------ ------ ------ ------ ------ ------ ------
File                     stmt   bran   cond    sub    pod   time  total
---------------------- ------ ------ ------ ------ ------ ------ ------
bin/znapzendzetup        87.8   61.1   43.7  100.0    n/a    0.6   75.6
lib/ZnapZend.pm          14.5    0.0    0.0   20.7   50.0    0.0   10.2
lib/ZnapZend/Config.pm   72.9   41.3   33.3   72.4   72.7    0.3   64.0
lib/ZnapZend/Time.pm     54.8   28.5   20.0   54.1   83.3    0.3   49.1
lib/ZnapZend/ZFS.pm      37.8   21.4   14.0   40.3   95.4   96.4   31.8
t/znapzendzetup.t        72.0   50.0   33.3  100.0    n/a    2.1   71.1
Total                    47.2   28.2   14.9   49.7   85.1  100.0   40.3
---------------------- ------ ------ ------ ------ ------ ------ ------

[oetiker]

autoscrub.t seems to lack done_testing at the end ... and I am not sure what the effect of the 1; at the end of every test script is ...

[jimklimov]

Usually in Perl (and shell depending on interpreter), including the external files can fail. If inclusion involves run-time parsing (like shell sourcing), it must succeed with perl 1; or shell true; to surely not-fail if there were no syntax/running errors before that point. Or same for actually running the program - return success if not failed :)

[oetiker]

at least my perl does not seem to care:

oetiker>perl -e '0;'; echo $?
0
oetiker>perl -e '1;'; echo $?
0

for shell, yes

oetiker>bash -c 'true'; echo $?
0
oetiker>bash -c 'false'; echo $?
1

[jimklimov]

Then it is only important for includes, at least (and you can never be sure what context your file ends up used in... so for good measure...):

$ echo '1;' > good.pm
$ perl -e 'use lib (@lib::ORIG_INC, "."); use good;' ; echo $?
0

$ echo '0;' > bad.pm
$ perl -e 'use lib (@lib::ORIG_INC, "."); use bad;' ; echo $?
bad.pm did not return a true value at -e line 1.
BEGIN failed--compilation aborted at -e line 1.
255

$ echo '' > none.pm
$ perl -e 'use lib (@lib::ORIG_INC, "."); use none;' ; echo $?
none.pm did not return a true value at -e line 1.
BEGIN failed--compilation aborted at -e line 1.
255

[oetiker]

yes for perl modules, it is a requirement ...

[jimklimov]

I think I caught the problem... in the command just before aborting the test suite without done_testing(), it handles a wrong arg list and returns help (with error exit code). So probably the perl process dies there.

[jimklimov]

Notably, in the codebase overall there is quite a few die() calls so the lower-level errors are reported without higher-level interpretation (e.g. in this new call stack, the actual returns include ERROR: dataset tank/source backup plan does not have destination 1 but not ERROR: cannot disable backup config for $opts->{src} destination $opts->{dst}. Did you create this config?\n because this was not just a false return but a big evil die).

For that CLI ARG mismatch the pod2usage(1) gets called. In the codebase overall I see several patterns of it being called:

$ git grep pod2usage
bin/znapzend:        pod2usage(-exitval => 'NOEXIT');
bin/znapzend:    $opts->{man} && pod2usage(-exitstatus => 0, -verbose => 2);
bin/znapzendzetup:    defined $mainOpt or pod2usage(1);
bin/znapzendzetup:        defined $opts->{src} or pod2usage(1);
bin/znapzendzetup:        defined $opts->{src} or pod2usage(1);
bin/znapzendzetup:        defined $opts->{src} or pod2usage(1);
bin/znapzendzetup:        defined $opts->{src} or pod2usage(1);
bin/znapzendzetup:        defined $opts->{dst} or pod2usage(1);
bin/znapzendzetup:        defined $opts->{src} or pod2usage(1);
bin/znapzendzetup:        defined $opts->{dst} or pod2usage(1);
bin/znapzendzetup:        defined $opts->{src} or pod2usage(1);
bin/znapzendzetup:        pod2usage(-exitstatus => 0, -verbose => 2);
bin/znapzendzetup:        pod2usage(-exitval => 'NOEXIT');
bin/znapzendzetup:        pod2usage(1);
bin/znapzendztatz:        pod2usage(-exitval => 'NOEXIT');
bin/znapzendztatz:    $opts->{man} && pod2usage(-exitstatus => 0, -verbose => 2);

[jimklimov]

Hooray, no more done_ and not ok found in logs of Travis tests completed so far.

[jimklimov]

A recent iteration offered an interesting config case... but I think we'd avoid that for now :-)

=== getDataSetProperties(): SKIP: 'tank/source/dest-disabled' because parent config 'tank/source' is already listed (inherited from tank/source)

=== getDataSetProperties(): FOUND: 'tank/source/dest-disabled' => 'dst_0_enabled' == 'off' (source: 'local')

=== getDataSetProperties(): SAVE LAST: 'tank/source/dest-disabled'

...

$VAR4 = {

          'dst_0_enabled' => 'off',

          'src' => 'tank/source/dest-disabled'

        };

[jimklimov]

The test fault however was due to logging something somewhere it should not have:

# zfs list -H -o name -t filesystem,volume remote/destination
# ssh -o batchMode=yes -o ConnectTimeout=30 root@remote test -x /home/travis/build/oetiker/znapzend/t/mbuffer
# ssh -o batchMode=yes -o ConnectTimeout=30 root@remote test -x /home/travis/build/oetiker/znapzend/t/mbuffer

EXCEPTION: zLog must be specified at creation time!

not ok 36 - znapzendzetup list --inherited -r tank tank/source/child succeeds (finds source and anothersource via recursion, and the explicit tank/source/child, but not other descendants)

#   Failed test 'znapzendzetup list --inherited -r tank tank/source/child succeeds (finds source and anothersource via recursion, and the explicit tank/source/child, but not other descendants)'
#   at ./t/znapzendzetup.t line 152.
#          got: '0'
#     expected: '1'

[jimklimov]

Hooray, both CI and coverage passed (and CI does what is expected too :-) )!

One nuance that I did not unwrap yet, and can't well reproduce locally, is:

Backup plan option 'zend-delay' has an invalid value (' ' is not a number) on backupSet tank/anothersource, ignored at /home/travis/build/oetiker/znapzend/t/../lib/ZnapZend.pm line 246.

which seems reported for all znapzend.t runs except for a couple that test custom codepaths regarding delays. The code for getting into this clause is

    if (defined($backupSet->{zend_delay})) {

so I assumed the hash entry to not exist if the zfs attribute is not there.

[oetiker]

Hooray, both CI and coverage passed (and CI does what is expected too :-) )!

One nuance that I did not unwrap yet, and can't well reproduce locally, is:

Backup plan option 'zend-delay' has an invalid value (' ' is not a number) on backupSet tank/anothersource, ignored at /home/travis/build/oetiker/znapzend/t/../lib/ZnapZend.pm line 246.

which seems reported for all znapzend.t runs except for a couple that test custom codepaths regarding delays. The code for getting into this clause is

    if (defined($backupSet->{zend_delay})) {

so I assumed the hash entry to not exist if the zfs attribute is not there.

hmm wondering how it gets that odd value ...

[jimklimov]

The "space" (or something that looks like it) probably comes from chomp... maybe... but how it passes defined in the first place is the puzzle.

Anyhow, this PR bloated with a lot of not directly related CI fixes and improvements, might leave this mystery for another one... :-)

[jimklimov]

Pre-solving merge conflicts vs. #452 changes to t/zfs mock...

[jimklimov]

@oetiker : I hope this one is good to merge before I break more stuff in the branch :)

[oetiker]

thank you!