syncoid: Fix multiple regressions caused by #818 #931
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
An oversight during the merging of subroutine `getsnapsfallback` into `getsnaps` (jimsalterjrs@e301b5b1) caused the recursive call to prepend `$sshcmd` to `$rhost` again, leading to this error: ```shell # syncoid --debug [email protected]:rpool/before syncoid-test-11/after … [TRUNCATED] … WARNING: snapshot listing failed, trying fallback command DEBUG: getting list of snapshots on rpool/before using ssh ssh -S /tmp/syncoid-root19216812226-1718239169-761069-2198 [email protected] zfs get -Hpd 1 all ''"'"'rpool/before'"'"'' |... [email protected]: Command not found. CRITICAL ERROR: snapshots couldn't be listed for rpool/before (exit code 256) at /usr/sbin/syncoid line 1900. ``` Fixes: jimsalterjrs#818 (comment)
In the fallback mode, non-snapshot datasets would be included in the output, leading to this parsing error: ```shell # syncoid --debug [email protected]:rpool/before syncoid-test-11/after … [TRUNCATED] … WARNING: snapshot listing failed, trying fallback command DEBUG: getting list of snapshots on rpool/before using ssh -S /tmp/syncoid-root19216812226-1718239740-763496-6973 [email protected] zfs get -Hpd 1 all ''"'"'rpool/before'"'"'' |... CRITICAL ERROR: Unexpected dataset format in rpool/before type filesystem - at /usr/sbin/syncoid line 1914. ``` Fixes: jimsalterjrs#818 (comment)
Return to using `zfs get guid,creation,createtxg` to avoid the performance overhead of `zfs get all`. There is a per-host global cache to remember: * if `zfs get` supports the type filter argument, `-t snapshot` and * which ZFS properties are available. The feature check introduced by subroutine `check_zfs_get_features` eliminates the need for a `getsnaps` fallback recursion, as there is no longer any guesswork into what features `zfs get` supports. Fixes: jimsalterjrs#818 (comment)
6 tasks
Deltik
changed the title
Deltik
force-pushed
the
hotfix/regression/getsnaps
branch
from
8a023d6 to
191ddfe
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
#818 only added a functional test for the new feature of sorting snapshots by the
createtxgproperty, so it and the existing tests did not catch the following regressions:Slower listing of snapshots' properties
From @phreaker0:
getsnapsfallback fails due to the wrong argument being passed inFrom @Deltik:
getsnapsparsing error in fallback mode due to non-snapshots appearing in the listFrom @Deltik:
Description
These three regressions are fixed in three separate commits:
perf(syncoid): Regression: Bad performance due to
zfs get allReturn to using
zfs get guid,creation,createtxgto avoid the performance overhead ofzfs get all.There is a per-host global cache to remember:
zfs getsupports the type filter argument,-t snapshotandThe feature check introduced by subroutine
check_zfs_get_featureseliminates the need for agetsnapsfallback recursion, as there is no longer any guesswork into what featureszfs getsupports.Fixes: #818 (comment)
fix(syncoid): Regression: Fallback recursion
$rhostmutated alreadyAn oversight during the merging of subroutine
getsnapsfallbackintogetsnaps(e301b5b1) caused the recursive call to prepend$sshcmdto$rhostagain, leading to this error:Fixes: #818 (comment)
fix(syncoid): Regression:
zfs getparser not compatible with fallbackIn the fallback mode, non-snapshot datasets would be included in the output, leading to this parsing error:
Fixes: #818 (comment)
How Has This Been Tested?
I haven't managed to figure out an easy way to test
syncoid's new reduced feature set detection features in subroutinecheck_zfs_get_features, so I performed manual acceptance tests with a FreeBSD 8.3 machine, which is preloaded with an old version of ZFS that would trigger what was formerly known asgetsnapsfallbackbecause it does not supportzfs get -t snapshot.I created some dummy snapshots:
And then was able to
syncoidthem out:To test the supported properties detection feature, I added a fake requested property and observed that it was not detected as a supported property:
Types of Changes