Allow configuring `mbuffer` path and size separately for each destination (and source) system #630

jimklimov · 2024-01-16T14:44:10Z

Closes: #629

See rationale and investigation details in the linked issue above.

Practical run with legacy-only setup (and debug mode):

...
# zfs list -H -o name -t filesystem,volume rpool/home/abuild
[2024-01-16 14:33:55.30202] [623247] [info] WARNING: property 'src_mbuffer_size' not set on backup for rpool/home/abuild, inheriting from legacy 'mbuffer_size': 128M
# ssh -o batchMode=yes -o ConnectTimeout=30 znapzend zfs list -H -o name -t 'filesystem,volume' pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild
[2024-01-16 14:33:55.59895] [623247] [info] WARNING: property 'dst_0_mbuffer' not set on backup for rpool/home/abuild, inheriting path[:port] from legacy 'mbuffer': mbuffer
[2024-01-16 14:33:55.59917] [623247] [info] WARNING: property 'dst_0_mbuffer_size' not set on backup for rpool/home/abuild, inheriting from legacy 'mbuffer_size': 128M
# ssh -o batchMode=yes -o ConnectTimeout=30 znapzend test -x mbuffer || ssh -o batchMode=yes -o ConnectTimeout=30 znapzend command -v mbuffer
WARNING: Only found executable file basename 'mbuffer' with 'command -v': /home/znapzend-ci-oi/bin/mbuffer

...

[2024-01-16 14:36:25.82687] [624979] [debug] sending snapshots from rpool/home/abuild to znapzend:pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild
[2024-01-16 14:36:25.82715] [624979] [debug] Are we sending "--since"? since=="0", skipIntermediates=="0", forbidDestRollback=="0", justCreated=="false"
# zfs list -H -o name -t snapshot -s creation -d 1 rpool/home/abuild
# ssh -o batchMode=yes -o ConnectTimeout=30 znapzend zfs list -H -o name -t snapshot -s creation -d 1 pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild
# zfs send -Lce -I 'rpool/home/abuild@znapzend-auto-2024-01-16T11:51:05Z' 'rpool/home/abuild@znapzend-auto-2024-01-16T14:36:24Z'|ssh -o batchMode=yes -o ConnectTimeout=30 znapzend 'mbuffer -q -s 256k -W 600 -m 128M|zfs recv -u -F pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild'
# zfs list -H -o name -t snapshot rpool/home/abuild@znapzend-auto-2024-01-16T14:36:24Z
# zfs set org.znapzend:dst_0_synced=1 rpool/home/abuild@znapzend-auto-2024-01-16T14:36:24Z
# zfs set org.znapzend:dst_0=znapzend:pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild rpool/home/abuild@znapzend-auto-2024-01-16T14:36:24Z

And for a setup with dedicated values:

src# zfs set org.znapzend:src_mbuffer=`which mbuffer` rpool/home
src# zfs set org.znapzend:dst_0_mbuffer="/home/znapzend-ci-oi/bin/mbuffer" rpool/home
src# zfs set org.znapzend:dst_0_mbuffer_size=1G rpool/home
src# zfs set org.znapzend:src_mbuffer_size=128M rpool/home

src# ./bin/znapzend --pidfile=/dev/null --features=compressed,recvu,sendIntermediates --inherited --runonce=rpool/home/abuild --debug
...
[2024-01-16 14:40:13.89197] [627314] [debug] === getDataSetProperties():
        Collected: $VAR1 = {
          'pre_znap_cmd' => 'off',
          'recursive' => 'on',
          'src_mbuffer_size' => '128M',
          'dst_0_mbuffer' => '/home/znapzend-ci-oi/bin/mbuffer',
          'dst_0_plan' => '1weeks=>1days,1months=>1weeks,1years=>1months,10years=>6months',
          'zend_delay' => '0',
          'tsformat' => 'znapzend-auto-%Y-%m-%dT%H:%M:%SZ',
          'post_znap_cmd' => 'off',
          'mbuffer' => 'mbuffer',
          'enabled' => 'on',
          'mbuffer_size' => '128M',
          'src' => 'rpool/home/abuild',
          'dst_0_mbuffer_size' => '1G',
          'src_plan' => '1weeks=>1days,1months=>1weeks,1years=>1months,10years=>6months',
          'src_mbuffer' => '/usr/bin/mbuffer',
          'dst_0' => 'znapzend:pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild'
        };
# zfs list -H -o name -t filesystem,volume rpool/home/abuild
# test -x /usr/bin/mbuffer || command -v /usr/bin/mbuffer
# ssh -o batchMode=yes -o ConnectTimeout=30 znapzend zfs list -H -o name -t 'filesystem,volume' pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild
# ssh -o batchMode=yes -o ConnectTimeout=30 znapzend test -x /home/znapzend-ci-oi/bin/mbuffer || ssh -o batchMode=yes -o ConnectTimeout=30 znapzend command -v /home/znapzend-ci-oi/bin/mbuffer
=== getBackupSet() : got 1 dataset(s) with a local or inherited backup plan
...
# zfs send -Lce -I 'rpool/home/abuild@znapzend-auto-2024-01-16T14:36:24Z' 'rpool/home/abuild@znapzend-auto-2024-01-16T14:40:14Z'|ssh -o batchMode=yes -o ConnectTimeout=30 znapzend '/home/znapzend-ci-oi/bin/mbuffer -q -s 256k -W 600 -m 1G|zfs recv -u -F pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild'

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

jimklimov · 2024-01-16T14:44:50Z

Hm, it did honour the destination settings but did not add a sender mbuffer. Checking...

…ource and destinations [oetiker#629] Signed-off-by: Jim Klimov <jimklimov@gmail.com>

jimklimov · 2024-01-16T14:55:21Z

A few typo fixes and a force-push later, the test with local settings passes (uses zfs | mbuffer | ssh "mbuffer|zfs" chain), wrapped for readability below:

# zfs send -Lce -I 'rpool/home/abuild@znapzend-auto-2024-01-16T14:36:24Z' 'rpool/home/abuild@znapzend-auto-2024-01-16T14:53:57Z' \
    |/usr/bin/mbuffer -q -s 256k -W 600 -m 128M \
    |ssh -o batchMode=yes -o ConnectTimeout=30 znapzend '\
        mbuffer -q -s 256k -W 600 -m 1G\
        |zfs recv -u -F pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/home/abuild'

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

…tiker#629] Signed-off-by: Jim Klimov <jimklimov@gmail.com>

jimklimov

@oetiker : got a late-arriving question, posted with code context.

jimklimov · 2024-01-17T10:35:05Z

lib/ZnapZend/Config.pm

                $self->zfs->fileExistsAndExec($file)
-                    or warn "*** WARNING: executable '$mbuffer' does not exist" . ($remote ? " on $remote\n\n" : "\n\n");
+                    or warn "*** WARNING: executable '$mbuffer' does not exist on " . ($remote ? "remote $remote" : "local") . " system, zfs receive can fail\n\n";
+                    # TOTHINK: Reset to 'off'/undef and ignore the validity checks below?

                #check if mbuffer size is valid
-                $backupSet->{mbuffer_size} =~ /^\d+[bkMG%]?$/
-                    or die "ERROR: mbuffer size '" . $backupSet->{mbuffer_size} . "' invalid\n";
+                $backupSet->{$dst . '_mbuffer_size'} =~ /^\d+[bkMG%]?$/
+                    or die "ERROR: mbuffer size '" . $backupSet->{$dst . '_mbuffer_size'} . "' invalid\n";
                #check if port is numeric
                $mbufferPort && do {
                    $mbufferPort =~ /^\d{1,5}$/ && int($mbufferPort) < 65535


@oetiker : WDYT about the TOTHINK comment here?

If we checked and could not confirm the mbuffer presence on remote destination, should we still try piping through it on receiver? Maybe there's a miracle like some shell alias or profile function to take care of it... we did warn about possible broken sending.

The alternative is - if we could not confirm its presence, set to undef or off here, and avoid using it at all. So SSH pipes right into zfs recv, maybe sub-optimally, but reliably. On the downside, the user never gets a nudge to actually fix their setup and optimize the transfer with buffering.

Now that I think of it, znapzendzetup list probably uses these methods (it does try to go to remote and confirm stuff), so resetting the values (perhaps for src_mbuffer above also) may be not a very good idea for visibility of on-disk setup; it does however represent the currently active setup (what would be executed if we run this right now).

So, I have a simmering concern but no particular opinion about what is "right" here, at the moment. Any inputs? :)

with things like this I am a fan of the software not trying to guess and hoping for a lucky strike

if there is no mbuffer at the remote end as promissed, things should fail ... clear and simple

Hm, that src_mbuffer=undef is seen if you search for it e.g. in config printout (not practically an issue -- is handled in sendRecvSnapshots() as off):

[2024-01-17 13:28:43.93473] [1538179] [info] WARNING: property 'src_mbuffer_size' not set on backup for rpool/ROOT, inheriting from legacy 'mbuffer_size': 128M [2024-01-17 13:28:44.23728] [1538179] [info] WARNING: property 'dst_0_mbuffer' not set on backup for rpool/ROOT, inheriting path[:port] from legacy 'mbuffer': mbuffer [2024-01-17 13:28:44.23749] [1538179] [info] WARNING: property 'dst_0_mbuffer_size' not set on backup for rpool/ROOT, inheriting from legacy 'mbuffer_size': 128M ... *** backup plan: rpool/ROOT *** dst_0 = znapzend:pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/ROOT dst_0_mbuffer = mbuffer dst_0_mbuffer_size = 128M dst_0_plan = 1month=>1week,1year=>1month,10years=>6months enabled = on mbuffer = mbuffer mbuffer_size = 128M post_znap_cmd = off pre_znap_cmd = off recursive = on src = rpool/ROOT Use of uninitialized value in printf at /usr/bin/znapzendzetup line 36. src_mbuffer = src_mbuffer_size = 128M src_plan = 1month=>1week,1year=>1month,10years=>6months tsformat = znapzend-auto-%Y-%m-%dT%H:%M:%SZ zend_delay = 0

Note how dst_0_mbuffer is not defined in the properties and gets inherited from legacy mbuffer -- maybe the right thing for src_mbuffer is to fall it back as off instead of undef after all (unless in "networked mbuffer" mode)?

# zfs get -s local all rpool/ROOT | sort | grep znap rpool/ROOT org.znapzend:dst_0 znapzend:pond/export/DUMP/NUTCI/znapzend/ci-deb/rpool/ROOT local rpool/ROOT org.znapzend:dst_0_plan 1months=>1weeks,1years=>1months,10years=>6months local rpool/ROOT org.znapzend:enabled on local rpool/ROOT org.znapzend:mbuffer mbuffer local rpool/ROOT org.znapzend:mbuffer_size 128M local rpool/ROOT org.znapzend:post_znap_cmd off local rpool/ROOT org.znapzend:pre_znap_cmd off local rpool/ROOT org.znapzend:recursive on local rpool/ROOT org.znapzend:src_plan 1months=>1weeks,1years=>1months,10years=>6months local rpool/ROOT org.znapzend:tsformat znapzend-auto-%Y-%m-%dT%H:%M:%SZ local rpool/ROOT org.znapzend:zend_delay 0 local

Ah, nice:

*** WARNING: executable 'off' does not exist on source system, will ignore ... Use of uninitialized value in printf at /usr/bin/znapzendzetup line 36. src_mbuffer =

Oh the edge cases :)

Posted a couple of fixes as #632

bin/znapzendzetup: update documentation for --mbuffer option variants

60f9651

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

Refactor "mbuffer(_size)" settings to handle different variants for s…

c81bf48

…ource and destinations [oetiker#629] Signed-off-by: Jim Klimov <jimklimov@gmail.com>

jimklimov force-pushed the issue-629 branch from 2ef25cf to c81bf48 Compare January 16, 2024 14:55

jimklimov added 3 commits January 16, 2024 14:59

README.md: clarify about requirements for a remote destination system

4116359

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

README.md: update for src_mbuffer* [oetiker#629]

ca510b0

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

.github/workflows/spelling/expect.txt: update for mbuffer changes [oe…

dd30d3f

…tiker#629] Signed-off-by: Jim Klimov <jimklimov@gmail.com>

oetiker approved these changes Jan 16, 2024

View reviewed changes

oetiker merged commit fd8c6a8 into oetiker:master Jan 16, 2024
4 checks passed

jimklimov deleted the issue-629 branch January 16, 2024 18:49

jimklimov commented Jan 17, 2024

View reviewed changes

jimklimov mentioned this pull request Jan 17, 2024

Fix handling nuances of src_mbuffer #632

Merged

This was referenced Feb 7, 2024

[RFE] znapzendsetup : support dst_N_enable setting #634

Closed

Dockerfile: fail fast if mbuffer is not compatible/fetchable [#589] #635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring `mbuffer` path and size separately for each destination (and source) system #630

Allow configuring `mbuffer` path and size separately for each destination (and source) system #630

jimklimov commented Jan 16, 2024

jimklimov commented Jan 16, 2024

jimklimov commented Jan 16, 2024 •

edited

Loading

jimklimov left a comment

jimklimov Jan 17, 2024 •

edited

Loading

oetiker Jan 17, 2024

jimklimov Jan 17, 2024

jimklimov Jan 17, 2024

jimklimov Jan 17, 2024

Allow configuring mbuffer path and size separately for each destination (and source) system #630

Allow configuring mbuffer path and size separately for each destination (and source) system #630

Conversation

jimklimov commented Jan 16, 2024

jimklimov commented Jan 16, 2024

jimklimov commented Jan 16, 2024 • edited Loading

jimklimov left a comment

Choose a reason for hiding this comment

jimklimov Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

oetiker Jan 17, 2024

Choose a reason for hiding this comment

jimklimov Jan 17, 2024

Choose a reason for hiding this comment

jimklimov Jan 17, 2024

Choose a reason for hiding this comment

jimklimov Jan 17, 2024

Choose a reason for hiding this comment

Allow configuring `mbuffer` path and size separately for each destination (and source) system #630

Allow configuring `mbuffer` path and size separately for each destination (and source) system #630

jimklimov commented Jan 16, 2024 •

edited

Loading

jimklimov Jan 17, 2024 •

edited

Loading