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

pybind: move cephfs to Cython #7745

Merged
merged 3 commits into from Mar 1, 2016
Merged

pybind: move cephfs to Cython #7745

merged 3 commits into from Mar 1, 2016

Conversation

sileht
Copy link
Contributor

@sileht sileht commented Feb 22, 2016

This change moves cephfs binding to Cython.

Closes-bug: #14818
Signed-off-by: Mehdi Abaakouk sileht@redhat.com

@jdurgin jdurgin added feature cephfs Ceph File System pybind labels Feb 22, 2016
@jdurgin
Copy link
Member

jdurgin commented Feb 22, 2016

guessing @jcsp would like to look at this one

@liewegas liewegas added this to the jewel milestone Feb 22, 2016
@@ -1 +1 @@
Subproject commit 9322c258084c6abdeefe00067f8b310a6e0d9a5a
Subproject commit c02b179490123a3212b0c0d23b69da13965d1552
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accidental submodule revert here -- run git submodule update --init --recursive from the root of your checkout to get the latest submodule updates

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@jcsp
Copy link
Contributor

jcsp commented Feb 23, 2016

@sileht this is segfaulting during tests: https://jenkins.ceph.com/job/ceph-pull-requests/1914/console

@jcsp jcsp assigned smbillah and unassigned smbillah Feb 23, 2016
@sileht sileht force-pushed the sileht/rados-cython branch 2 times, most recently from fb98797 to 5067078 Compare February 24, 2016 14:46
@sileht
Copy link
Contributor Author

sileht commented Feb 25, 2016

It's fixed

@wjwithagen
Copy link
Contributor

@sileht
Please Rewite the conditonals in pybind/Makefile.am to actually restrict to those things that are really requested to restict. So WITH_RADOS only for Rados, WITH_CEPHFS, WITH RBD.
So some things do get build even when RADOS and/or RBD are not being build.

@sileht
Copy link
Contributor Author

sileht commented Feb 25, 2016

I have updated pybind/Makefile.am

@sileht sileht force-pushed the sileht/rados-cython branch 5 times, most recently from 0438ed4 to 74711cb Compare February 26, 2016 12:03
@jcsp
Copy link
Contributor

jcsp commented Feb 26, 2016

This is failing when used within ceph_volume_client

Traceback (most recent call last):
  File "<string>", line 13, in <module>
  File "/home/john/ceph.autotools/src/pybind/ceph_volume_client.py", line 570, in authorize
    pool_name = self._get_ancestor_xattr(path, "ceph.dir.layout.pool")
  File "/home/john/ceph.autotools/src/pybind/ceph_volume_client.py", line 546, in _get_ancestor_xattr
    result = self.fs.getxattr(path, attr)
  File "cephfs.pyx", line 711, in cephfs.LibCephFS.getxattr (/home/john/ceph.autotools/src/build/cephfs.c:10003)
  File "cephfs.pyx", line 294, in cephfs.realloc_chk (/home/john/ceph.autotools/src/build/cephfs.c:2682)
MemoryError: realloc failed

@sileht
Copy link
Contributor Author

sileht commented Feb 26, 2016

I have fixed this error, now the code will raise the root cause instead.

So in your case, you will now get an error like "error in getxattr: error code -XX".

I have also added a test to cover that, but it doesn't pass, at least on my setup and even with the old python binding...

With cython, getxattr returns errno -34, while it doesn't complains when the test writes the xattr value with setxattr.
With the old python binding, getattrx was returni,g an empty string instead of my value.

The issue occurs when getxattr have to retreive a payload greater that 255 bytes.

So, Why setxattr doesn't complains ? Does the value have been stored ? Anyways, that looks like a libcephfs bug.

@wjwithagen
Copy link
Contributor

On 26-2-2016 18:09, Mehdi ABAAKOUK wrote:

I have fixed this error, now the code will raise the root cause instead.

So in your case, you will now get an error like "error in getxattr:
error code -XX".

I have also added a test to cover that, but it doesn't pass, at least on
my setup and even with the old python binding...

With cython, getxattr returns errno -34, while it doesn't complains when
the test writes the xattr value with setxattr.
With the old python binding, getattrx was returni,g an empty string
instead of my value.

The issue occurs when getxattr have to retreive a payload greater that
255 bytes.

So, Why setxattr doesn't complains ? Does the value have been stored ?
Anyways, that looks like a libcephfs bug.

I've run into another snag here, which I do not know how to solve since
cython is quite new to me.

For FreeBSD we have a define in:
./include/compat.h
#define ENODATA ENOATTR

Because ENODATA is not really defined in /usr/include/errno.h
so 'from libc cimport errno' does not pick this up.

How would I fix this for FreeBSD?

--WjW

@wjwithagen
Copy link
Contributor

ATM I manually change part to code to use ENOATTR (instead of ENODATA), but then I still get:
INFO:ceph_disk.main:Running command: ../ceph --cluster ceph --name client.bootstrap-osd --keyring test-ceph-disk/bootstrap-osd/ceph.
keyring osd create --concise 9027828a-dcec-11e5-8cd4-1c6f6582ec12
Traceback (most recent call last):
File "/usr/srcs/Ceph/work/ceph/src/ceph-disk/.tox/py27/bin/ceph-disk", line 9, in
load_entry_point('ceph-disk', 'console_scripts', 'ceph-disk')()
File "/usr/srcs/Ceph/work/ceph/src/ceph-disk/ceph_disk/main.py", line 4601, in run
main(sys.argv[1:])
File "/usr/srcs/Ceph/work/ceph/src/ceph-disk/ceph_disk/main.py", line 4553, in main
args.func(args)
File "/usr/srcs/Ceph/work/ceph/src/ceph-disk/ceph_disk/main.py", line 3061, in main_activate
init=args.mark_init,
File "/usr/srcs/Ceph/work/ceph/src/ceph-disk/ceph_disk/main.py", line 2882, in activate_dir
(osd_id, cluster) = activate(path, activate_key_template, init)
File "/usr/srcs/Ceph/work/ceph/src/ceph-disk/ceph_disk/main.py", line 2974, in activate
keyring=keyring,
File "/usr/srcs/Ceph/work/ceph/src/ceph-disk/ceph_disk/main.py", line 925, in allocate_osd_id
raise Error('ceph osd create failed', e, e.output)
Error: Error: ceph osd create failed: Command '../ceph' returned non-zero exit status 1: *** DEVELOPER MODE: setting PATH, PYTHONPAT
H and LD_LIBRARY_PATH ***
Traceback (most recent call last):
File "../ceph", line 108, in
import rados
File "rados.pyx", line 364, in init rados (/usr/srcs/Ceph/work/ceph/src/build/rados.c:48031)
NameError: name 'errno' is not defined

@sileht
Copy link
Contributor Author

sileht commented Feb 29, 2016

This issue looks like related to your Cython installation/version and not to this change.
You need to figure out why https://github.com/cython/cython/blob/master/Cython/Includes/libc/errno.pxd is not compiled on FreeBSD.

Also, Rados and Rbd python module already use libc.errno cython module, I don't think this is the goal of this PR to fix compilation issue on FreeBSD that are already present in ceph master branch.

@wjwithagen
Copy link
Contributor

On 29-2-2016 08:37, Mehdi ABAAKOUK wrote:

This issue looks like related to your Cython installation/version and
not to this change.
You need to figure out why
https://github.com/cython/cython/blob/master/Cython/Includes/libc/errno.pxd
is not compiled on FreeBSD.

Also, Rados and Rbd python module already use libc.errno cython module,
I don't think this is the goal of this PR to fix compilation issue on
FreeBSD that are already present in ceph master branch.

No I appriate that this PR is not to fix the FreeBSD issue.
I'll take that in my FreeBSD wip, but this PR is/was the place where the
work was actually done.
I have disabled RBD in FreeBSD because that device is not there.

I installed cython from the packages that FreeBSD has. And in those
packages there is no ENODATA. And as such it is also not in libc on
FreeBSD???
What I do have is:
/usr/local/lib/python2.7/site-packages/Cython/Includes/libc/errno.pxd
And that file does contain a
cdef extern from "errno.h" nogil:
enum:
With
ENODATA

Could it be that lib.errno only get compiled in when RBD is defined?

--WjW

This change moves cephfs binding to Cython.

Closes-bug: ceph#14818
Signed-off-by: Mehdi Abaakouk <sileht@redhat.com>
@sileht
Copy link
Contributor Author

sileht commented Feb 29, 2016

If you have errno.pxd, and cimport won't work, perhaps that means you have something wrong with your cython package. Cython/Includes/libc is automatically included on Linux for cython compilation and it looks not on FreeBSD.

@jcsp
Copy link
Contributor

jcsp commented Feb 29, 2016

@wjwithagen please pick up discussion elsewhere, this PR is about libcephfs, not Cython in general.

@jcsp
Copy link
Contributor

jcsp commented Feb 29, 2016

@sileht this now passes volume client tests locally when combined with these two commits, please cherry pick them onto this branch:
jcsp@adddab4
jcsp@8969ae2

John Spray added 2 commits February 29, 2016 11:36
The ctypes bindings returned empty string
instead of raising exception.  This was a bug,
because it made it impossible to detect the
difference between missing xattr and empty
xattr.

Signed-off-by: John Spray <john.spray@redhat.com>
No need to explicitly touch the (no-longer-existing)
load_libcephfs method during module load, as with
the cython version we already get an ImportError
if the C library is unavailable.

Signed-off-by: John Spray <john.spray@redhat.com>
@jcsp
Copy link
Contributor

jcsp commented Feb 29, 2016

Pushed branch to main repo as wip-test-cython, will schedule a fs suite on it once gitbuilders have built it.

@wjwithagen
Copy link
Contributor

@jcsp
Sorry, you are right. Will discuss with @sileht directly, to see if he can help me figure it out.

@jcsp
Copy link
Contributor

jcsp commented Feb 29, 2016

@jcsp
Copy link
Contributor

jcsp commented Mar 1, 2016

The test run was all passes except for one "possible recursive locking detected" in FUSE (can't be anything to do with Cython, but it did happen to be in the VolumeClient test, which is currently failing always, so no possibility of a regression here).

jcsp pushed a commit that referenced this pull request Mar 1, 2016
pybind: move cephfs to Cython

Reviewed-by: John Spray <john.spray@redhat.com>
@jcsp jcsp merged commit 3d8fc94 into ceph:master Mar 1, 2016
@jcsp
Copy link
Contributor

jcsp commented Mar 1, 2016

Thanks @sileht !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants