Adopt pyzfs from ClusterHQ

[loli10K]

Description

This commit introduces several changes:

• Import pyzfs source code from ClusterHQ GitHub repository into contrib/

• Give existing source a good PEP8 talk

• Add RPM/DEB packaging for pyzfs

• Fix some outstanding issues with the existing pyzfs code caused by changes in the ABI since the last time the code was updated

• Integrate pyzfs Python unittest with the ZFS Test Suite

• Enable temporarily disabled tests in the Python libzfs_core test suite.

• Credit the original authors: pushed existing pyzfs code retaining original authorship

• Better integration with autotools & rpmbuild (disable pyzfs when python/setuptools/cffi are not available?)

• Add missing libzfs_core lzc_* functions to pyzfs (EDIT: this is going to take a while)

  • lzc_change_key
  • lzc_channel_program
  • lzc_channel_program_nosync
  • lzc_load_key
  • lzc_receive_one
  • lzc_receive_resumable
  • lzc_receive_with_cmdprops
  • lzc_receive_with_header
  • lzc_reopen
  • lzc_send_resume
  • lzc_sync
  • lzc_unload_key

Motivation and Context

pyzfs is a python wrapper around the libzfs_core C library developed by ClusterHQ (https://github.com/ClusterHQ/pyzfs): unfortunately the original code does not work correctly due to ABI changes and therfore cannot be used with the current master branch.
Importing this code will allow the project to test/verify changes in the ABI and provide users with a programmatical way to access ZFS objects/properties and run different tasks from a Python shell or script.

How Has This Been Tested?

Tested locally on Debian8 builder running the (updated) libzfs_core Python test suite. The buildbots will probably lack some -dev(el) packages and will need to be updated.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.
• Change has been approved by a ZFS on Linux member.

[dinatale2]

@loli10K I went ahead and merged the buildbot patch. Feel free to refresh this.

[loli10K]

I've been trying to squash some FIXMEs and either i have found a bug in the existing lzc_hold() interface usage or i'm missing something obvious:

In zfs_hold_nvl() (userland) we fnvlist_free() the error list when lzc_hold() return value is 0:
https://github.com/zfsonlinux/zfs/blob/2a0428f16b93b7d5160c277dd678e65ad27d996f/lib/libzfs/libzfs_dataset.c#L4837-L4844

but doing this we fail to detect, for instance, ENOENT errors from dsl_dataset_user_hold_check():

https://github.com/zfsonlinux/zfs/blob/2a0428f16b93b7d5160c277dd678e65ad27d996f/module/zfs/dsl_userhold.c#L116-L126

Consider the following example where we try to "zfs hold" a snapshot that does not (no longer) exist; i'm using gdb to modify the input nvl and "simulate" a race condition where "testpool/fs2@snap1" gets deleted just before we try to "hold" it:

root@linux:~# POOLNAME='testpool'
root@linux:~# TMPDIR='/var/tmp'
root@linux:~# mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
root@linux:~# zpool destroy -f $POOLNAME
root@linux:~# rm -f $TMPDIR/zpool.dat
root@linux:~# fallocate -l 128m $TMPDIR/zpool.dat
root@linux:~# # zpool create -f $POOLNAME $TMPDIR/zpool.dat
root@linux:~# echo password | zpool create -f -O encryption=on -O keyformat=passphrase $POOLNAME $TMPDIR/zpool.dat
root@linux:~# zfs create $POOLNAME/fs1
root@linux:~# zfs snap $POOLNAME/fs1@snap1
root@linux:~# 
root@linux:~# gdb -q zfs -ex 'b lzc_hold' -ex "run hold tag $POOLNAME/fs1@snap1" 
Really redefine built-in command "skip"? (y or n) [answered Y; input not from terminal]
Reading symbols from zfs...done.
Function "lzc_hold" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (lzc_hold) pending.
Starting program: /sbin/zfs hold tag testpool/fs1@snap1
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, lzc_hold (holds=0x62b8b0, cleanup_fd=-1, errlist=0x7fffffffe560) at libzfs_core.c:447
447		elem = nvlist_next_nvpair(holds, NULL);
(gdb) # replace valid hold with non-existing hold
(gdb) call nvlist_print(stderr, holds)
nvlist version: 0
	testpool/fs1@snap1 = tag
(gdb) call nvlist_remove_all(holds, "testpool/fs1@snap1")
$1 = 0
(gdb) call nvlist_add_string(holds, "testpool/fs2@snap1", "tag")
$2 = 0
(gdb) call nvlist_print(stderr, holds)
nvlist version: 0
	testpool/fs2@snap1 = tag
(gdb) !zfs get all testpool/fs2@snap1
cannot open 'testpool/fs2@snap1': dataset does not exist
(gdb) b lzc_ioctl
Breakpoint 2 at 0x7ffff752bc6b: file libzfs_core.c, line 130.
(gdb) c
Continuing.

lzc_ioctl() returns 0 with a "valid" errlist (testpool/fs2@snap1 = 2 (ENOENT)) but our caller (zfs_hold_nvl()) will ignore it and fnvlist_free(errors):

Breakpoint 2, lzc_ioctl (ioc=ZFS_IOC_HOLD, name=0x7fffffffe010 "testpool", source=0x625fd0, resultp=0x7fffffffe560) at libzfs_core.c:130
130		zfs_cmd_t zc = {"\0"};
(gdb) fin
Run till exit from #0  lzc_ioctl (ioc=ZFS_IOC_HOLD, name=0x7fffffffe010 "testpool", source=0x625fd0, resultp=0x7fffffffe560) at libzfs_core.c:130
0x00007ffff752c804 in lzc_hold (holds=0x62b8b0, cleanup_fd=-1, errlist=0x7fffffffe560) at libzfs_core.c:458
458		error = lzc_ioctl(ZFS_IOC_HOLD, pool, args, errlist);
Value returned is $3 = 0
(gdb) n
459		nvlist_free(args);
(gdb) list
454		fnvlist_add_nvlist(args, "holds", holds);
455		if (cleanup_fd != -1)
456			fnvlist_add_int32(args, "cleanup_fd", cleanup_fd);
457	
458		error = lzc_ioctl(ZFS_IOC_HOLD, pool, args, errlist);
459		nvlist_free(args);
460		return (error);
461	}
462	
463	/*
(gdb) p error
$4 = 0
(gdb) call nvlist_print(stderr, *errlist)
nvlist version: 0
	testpool/fs2@snap1 = 2
(gdb) fin

No error is reported to the user running "zfs hold tag testpool/fs2@snap1"

(gdb) c
Continuing.
[Inferior 1 (process 4603) exited normally]
(gdb) q
root@linux:~# echo $?
0

[loli10K]

This test is failing on current master but passes on zfs-0.6.5.11:

----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/libzfs_core/test/test_libzfs_core.py", line 431, in test_hold_missing_fs
    lzc.lzc_hold({snap: 'tag'})
AssertionError: HoldFailure not raised

Actually this seems to be caused by "OpenZFS 7071 - lzc_snapshot does not fill in errlist on ENOENT" (e88551d): before this commit lzc_hold() will return ENOENT which raises the expected HoldFailure in test_hold_missing_fs(), but the ENOENT is actually from zfs_secpolicy_write_perms(), not zfs_ioc_hold().

After said commit both zfs_secpolicy_write_perms() and zfs_ioc_hold() return 0, and so does lzc_hold(): HoldFailure doesn't get raised in the Python code and thus the test fails:

root@linux:~# cat /sys/module/zfs/version 
0.7.0-339_g2a0428f
root@linux:~# cat > ./zfs_hold.sh <<'EOF'
> POOLNAME='testpool'
> TMPDIR='/var/tmp'
> mountpoint -q $TMPDIR || mount -t tmpfs tmpfs $TMPDIR
> zpool destroy -f $POOLNAME
> rm -f $TMPDIR/zpool.dat
> fallocate -l 128m $TMPDIR/zpool.dat
> zpool create -f $POOLNAME $TMPDIR/zpool.dat
> zfs create $POOLNAME/fs1
> zfs snap $POOLNAME/fs1@snap1
> python -c "import libzfs_core as lzc; print lzc.lzc_hold({'$POOLNAME/fs2@snap2': 'tag'})"
> EOF
root@linux:~# chmod +x ./zfs_hold.sh
root@linux:~# 
root@linux:~# stap -d zfs -e '
> probe
> module("zfs").function("zfs_ioc_hold").return,
> module("zfs").function("zfs_secpolicy_write_perms").return,
> module("zfs").function("dsl_dataset_user_hold").return,
> module("zfs").function("dsl_dataset_hold").return
> { printf(" <- %s ret=%s\n", ppfunc(), $$return$$); }' -c ./zfs_hold.sh
 <- zfs_secpolicy_write_perms ret=return=0
 <- zfs_secpolicy_write_perms ret=return=0
 <- zfs_secpolicy_write_perms ret=return=0
['testpool/fs2@snap2']
 <- zfs_secpolicy_write_perms ret=return=0
 <- dsl_dataset_hold ret=return=2
 <- dsl_dataset_user_hold ret=return=0
 <- zfs_ioc_hold ret=return=0
root@linux:~# 

[dinatale2]

@loli10K Merged openzfs/zfs-buildbot#135

[dinatale2]

@avg-I What are your thoughts about including pyzfs in our tree?

@scotws Would you mind giving this a look over?

[avg-I]

@dinatale2 I will be very happy with that. Especially, if it will help with new features like Python 3 support (see a PR in the ClusterHQ repo). I can also transfer ownership of the project on pypi and readthedocs. The ClusterHQ GitHub repository is readonly since the company went out of business and I personally didn't have time to look after the project.

[scotws]

@loli10K Just as a thought, I'm wondering if it would make the code cleaner if the large if chains in _nvlist.py were replaced by some sort of dictionary-based dispatch to small functions (something like https://www.safaribooksonline.com/library/view/python-cookbook-2nd/0596007973/ch04s17.html)? The problem, of course, is that some of those functions would then be very small, so it might not be worth it.

[loli10K]

@scotws thanks, i will gladly welcome any form of cleanup / optimization, it that doesn't make the code less readable, though my primary objective now is to have all the temporary disabled "FIXME" test cases up and running.

[loli10K]

The only remaining tests that are still failing ("test_recv_..._origin") seem to be broken by Illumos 5925 zfs receive -o origin= (fcff0f3):

root@linux:~# stap -g -DMAXSTRINGLEN=10240 -v -d zfs -e '
> probe
> module("zfs").function("zfs_ioc_recv*").call,
> module("zfs").function("recv_begin_check_existing_impl").call,
> module("zfs").function("dmu_recv_resume_begin_check").call,
> module("zfs").function("dmu_recv_begin_sync").call,
> module("zfs").function("dmu_recv_begin_check").call,
> module("zfs").function("dsl_sync_task").call,
> module("zfs").function("dmu_recv_begin").call
> { printf(" -> %s %s\n", ppfunc(), $$parms$$); }
> probe
> module("zfs").function("recv_begin_check_existing_impl").return,
> module("zfs").function("dmu_recv_resume_begin_check").return,
> module("zfs").function("dmu_recv_begin_sync").return,
> module("zfs").function("dmu_recv_begin_check").return,
> module("zfs").function("dsl_sync_task").return,
> module("zfs").function("dmu_recv_begin").return,
> module("zfs").function("zfs_ioc_recv*").return
> { printf(" <- %s %s\n", ppfunc(), $$return$$); }' -c "python -m unittest --verbose libzfs_core.test.test_libzfs_core.ZFSTest.test_recv_incremental_non_clone_but_set_origin"
...
 -> zfs_ioc_recv_impl tofs="pool.fc6cfadf-7ce8-45c4-9a0d-8c29cc94d0f5/fs2/received-20" tosnap="snap2" origin="pool.fc6cfadf-7ce8-45c4-9a0d-8c29cc94d0f5/fs2/received-20@snap1" ...
 -> dmu_recv_begin tofs="pool.fc6cfadf-7ce8-45c4-9a0d-8c29cc94d0f5/fs2/received-20" tosnap="snap2" drr_begin={... .drr_u={.drr_begin={.... .drr_flags=4, ..., .drr_toname="pool.fc6cfadf-7ce8-45c4-9a0d-8c29cc94d0f5/fs1@snap2"}, ...} force=0 resumable=0 origin="pool.fc6cfadf-7ce8-45c4-9a0d-8c29cc94d0f5/fs2/received-20@snap1", ...}
 -> dsl_sync_task pool="pool.fc6cfadf-7ce8-45c4-9a0d-8c29cc94d0f5/fs2/received-20" ...
 -> dmu_recv_begin_check ...
 <- dmu_recv_begin_check return=22
 <- dsl_sync_task return=22
 <- dmu_recv_begin return=22
 <- zfs_ioc_recv_impl return=22
 <- zfs_ioc_recv return=22

With drr_flags=DRR_FLAG_FREERECORDS and origin="pool.fc6cfadf-7ce8-45c4-9a0d-8c29cc94d0f5/fs2/received-20@snap1" we are failing here:

https://github.com/zfsonlinux/zfs/blob/e086e717c3fc2c5b0867ee4b5ec07200a53b8dac/module/zfs/dmu_send.c#L1687-L1695

[loli10K]

@scotws i considered converting "if" chains to dictionary-based dispatch functions in "_nvlist.py" (mainly in _nvlist_add_array() and _dict_to_nvlist()) but the change seems almost unnecessary given the code looks very much readable and clean as it is. I'm not a Python developer though, my experience is limited and there may be things i've not taken into account.

[scotws]

@loli10K Yes, I think you're right, changing the code would be change for formal sake and wouldn't help anybody. The Zen of Python reference would seem to be "Practicality beats purity" 😃

[loli10K]

The following additional interfaces are going to be marked "uncommitted":

• lzc_reopen introduced in d3f2cd7, not in any tagged release
• lzc_sync landed in bec1067, tagged since zfs-0.7.0-rc5, not in OpenZFS
• lzc_receive_with_cmdprops landed in a3eeab2, tagged since zfs-0.7.0-rc5, not in OpenZFS
• lzc_receive_one 43e52ed, tagged since zfs-0.7.0-rc1, not in OpenZFS
• lzc_load_key, lzc_unload_key and lzc_change_key landed with b525630, not in any tagged release

We are missing lzc_pool_checkpoint/lzc_pool_checkpoint_discard from OpenZFS 9166 zfs storage pool checkpoint and lzc_remap from OpenZFS 7614 zfs device evacuation/removal.

why it can't run in-tree?

I think this is because libzfs_core.py is not installed in Python's sys.path when working in-tree. It seems we may have to change the logic in https://github.com/zfsonlinux/zfs/blob/master/scripts/zfs-tests.sh#L203 to copy pyzfs source in the "constrained" path and export PYTHONPATH environment variable in "scripts/zfs-tests.sh". I am going to do some tests locally and push the change (along with changes to "uncommitted" interfaces) if it looks good.

The ABI Compliance Checker seems very nice to have but it needs binaries compiled with "-g -Og" and an old version of the library to work properly; we could ask the abicc maintainer to be added to the ABI tracker: https://abi-laboratory.pro/tracker/

[loli10K]

Unless i am missing something obvious it seems running pyzfs in-tree would require quite some changes:

1. install python source into some location python can find when running under the test-runner: this can be taken care of in zfs-tests.sh

2. set/export PYTHONPATH to previous localtion so python can find "libzfs_core" module and load it: this cannot be done py the test runner, sudo will modify the environment, we most likery need to set PYTHONPATH in "libtest.shlib":

https://github.com/zfsonlinux/zfs/blob/74df0c5e251a920a1966a011c16f960cd7ba562e/tests/zfs-tests/include/libtest.shlib#L36-L42

3. now that we can find and load libzfs_core python code we need libzfs_core.so to be in ld search path: zfs binaries used by the ZTS are just wrapper scripts when running in-tree, libtool takes care of hacking LD_LIBRARY_PATH into something functional. A similar approach could be used for python, creating a wrapper script and export LD_LIBRARY_PATH to "$zfs_srcdir/lib/libzfs_core/". To be honest this seems a lot hackish to me.

[avg-I]

How about pip install -e . ?
Or are you solving a different kind of problem?

[loli10K]

@avg-I i just tried pip install -e . on a local Ubuntu17 builder but that unfortunately doesn't seem to solve the problem, python doesn't find libzfs_core module:

--- Configuration ---
Runfile:         /usr/src/zfs/tests/runfiles/linux.run
STF_TOOLS:       /usr/src/zfs/tests/test-runner
STF_SUITE:       /usr/src/zfs/tests/zfs-tests
STF_PATH:        /usr/src/zfs/bin
FILEDIR:         /var/tmp
FILES:           /var/tmp/file-vdev0 /var/tmp/file-vdev1 /var/tmp/file-vdev2
LOOPBACKS:       /dev/loop0 /dev/loop1 /dev/loop2 
DISKS:           loop0 loop1 loop2 
NUM_DISKS:       3
FILESIZE:        4G
ITERATIONS:      1
TAGS:            pyzfs
Keep pool(s):    rpool
Missing util(s): mmap_libaio bc fio umask wait 

/usr/src/zfs/tests/test-runner/bin/test-runner.py  -c /usr/src/zfs/tests/runfiles/linux.run -T pyzfs -i /usr/src/zfs/tests/zfs-tests -I 1
Test: /usr/src/zfs/tests/zfs-tests/tests/functional/pyzfs/pyzfs_unittest (run as root) [00:00] [SKIP]

Results Summary
SKIP	   1

Running Time:	00:00:00
Percent passed:	0.0%
Log directory:	/var/tmp/test_results/20180410T193920

root@ubuntu:/usr/src/zfs# cat /var/tmp/test_results/current/log 
Test: /usr/src/zfs/tests/zfs-tests/tests/functional/pyzfs/pyzfs_unittest (run as root) [00:00] [SKIP] 
19:39:20.46 libzfs_core not found by Python

Python cwd during test: /var/tmp/test_results/20180410T193920
Python sys.path: ['', '/usr/lib/python2.7', '/usr/lib/python2.7/plat-x86_64-linux-gnu', '/usr/lib/python2.7/lib-tk', '/usr/lib/python2.7/lib-old', '/usr/lib/python2.7/lib-dynload', '/usr/local/lib/python2.7/dist-packages', '/usr/lib/python2.7/dist-packages']

Unless i copy libzfs_core into one of those directories (which defeats the purpose of running in-tree, i think) python will not be able to load the module.

[behlendorf]

Unless i copy libzfs_core into one of those directories (which defeats the purpose of running in-tree, i think) python will not be able to load the module.

Could you extend the zfs-helpers.sh script to create a symlink for libzfs_core from one of those directories to its in-tree location. This isn't ideal, but this was already done to test the mount.zfs helper and udev rules which must be installed in specifical locations on the system.

This script it never invoked by the ZTS directly so it has the advantage that this is a manual customization. Otherwise the test cases which depend on installed system components are skipped.

The ABI Compliance Checker seems very nice to have but it needs binaries compiled with "-g -Og" and an old version of the library to work properly; we could ask the abicc maintainer to be added to the ABI tracker: https://abi-laboratory.pro/tracker/

That's unfortunate. Perhaps there are alternate tools out there we could take advantage of.

[behlendorf]

@loli10K you'll want to resubmit this. The "coverage" builder was split from the the other "test" builders so you can request it independantly with Requires-builders: coverage.

[loli10K]

This is, unfortunately, the only way i managed to run pyzfs ztests in-tree: "libzfs_core" code coverage is now at ~97% according to codecov, which i think is good compared to the decreased (!?) coverage (-0.2%) introduced by the zpool split tests.

[behlendorf]

I agree this isn't pretty, but it is something we can live with. The target audience for this script is only in-tree developer and it'll be nice to benefit from the additional coverage. As for the zpool split tests I was surprised by that too.

[behlendorf]

I agree this isn't pretty, but it is something we can live with. The target audience for this script is only in-tree developer and it'll be nice to benefit from the additional coverage. As for the zpool split tests I was surprised by that too.

[loli10K]

Latest push adds lzc_remap() (and tests).

[codecov]

Codecov Report

Merging #7230 into master will increase coverage by 0.55%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7230      +/-   ##
==========================================
+ Coverage   76.53%   77.08%   +0.55%     
==========================================
  Files         335      335              
  Lines      107100   107100              
==========================================
+ Hits        81965    82556     +591     
+ Misses      25135    24544     -591

Flag	Coverage Δ
#kernel	77.34% <100%> (+0.29%)	⬆️
#user	66.56% <100%> (+0.68%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d68ac65...63597af. Read the comment docs.

[behlendorf]

@loli10K the out of band feedback I've gotten on this PR has all been positive. Unless you have additional proposed changes I'd like to go ahead an integrate this.

[loli10K]

@behlendorf nice, positive feedback is good. This should be good to go, i was mostly waiting to integrate lzc_pool_checkpoint, lzc_pool_checkpoint_discard and lzc_initialize which we can always do later when the relevant OpenZFS changes gets merged.

[FireDrunk]

Is there some guide / tutorial / basic example available for installing and using the pyzfs library?
I think if it makes the next release, it would be nice to get some basic example up and running in the docs.
I'd be happy to help if you give me some pointers.