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

cmake,deb,rpm: remove cython 0.29's subinterpreter check, re-enable build with cython 0.29+ #25585

Merged
merged 2 commits into from Dec 20, 2018

Conversation

@tserong
Copy link
Contributor

commented Dec 17, 2018

cython 0.29 introduced a check which prevents multiple python
subinterpreters from loading the same module:

cython/cython@7e27c7c

Unfortunately, this completely breaks ceph-mgr. Until we can
figure out a better long term solution, this commit removes
cython's subinterpreter check, via some careful abuse of the
C preprocessor.

This works because when cython is invoked, it first generates
some C code, then compiles it. We know it's going to generate
C code including:

  int __Pyx_check_single_interpreter(void) { ... }

and:

  if (__Pyx_check_single_interpreter())
      return NULL;

So, we can do the following:

  #define void0 dead_function(void)
  #define __Pyx_check_single_interpreter(ARG)=ARG ## 0

This replaces the call to __Pyx_check_single_interpreter()
with a literal 0, removing the subinterpreter check.

The void0 dead_function(void) thing is necessary because
the __Pyx_check_single_interpreter() macro also clobbers
that function definition, so we need to make sure it's
replaced with something that works as a function definition.

Fixes: https://tracker.ceph.com/issues/37472
Signed-off-by: Tim Serong tserong@suse.com

cmake: remove cython 0.29's subinterpreter check
cython 0.29 introduced a check which prevents multiple python
subinterpreters from loading the same module:

cython/cython@7e27c7c

Unfortunately, this completely breaks ceph-mgr.  Until we can
figure out a better long term solution, this commit removes
cython's subinterpreter check, via some careful abuse of the
C preprocessor.

This works because when cython is invoked, it first generates
some C code, then compiles it.  We know it's going to generate
C code including:

  int __Pyx_check_single_interpreter(void) { ... }

and:

  if (__Pyx_check_single_interpreter())
      return NULL;

So, we can do the following:

  #define void0 dead_function(void)
  #define __Pyx_check_single_interpreter(ARG)=ARG ## 0

This replaces the call to __Pyx_check_single_interpreter()
with a literal 0, removing the subinterpreter check.

The void0 dead_function(void) thing is necessary because
the __Pyx_check_single_interpreter() macro also clobbers
that function definition, so we need to make sure it's
replaced with something that works as a function definition.

Fixes: https://tracker.ceph.com/issues/37472
Signed-off-by: Tim Serong <tserong@suse.com>

@tserong tserong requested review from rjfd, tchaikov, liewegas, badone and smithfarm Dec 17, 2018

@LenzGr LenzGr requested a review from sebastian-philipp Dec 17, 2018

Revert "spec: fix cython package version to less than 0.29"
This reverts commit 088fbff.

Signed-off-by: Tim Serong <tserong@suse.com>

@tserong tserong force-pushed the SUSE:wip-remove-cython-subinterpreter-check branch from 64db08c to 0b29fbc Dec 17, 2018

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

oh god! @tserong , you are a wizard!

could add a "Signed-off-by" line to the revert commit to appease jenkins.

@tchaikov
Copy link
Contributor

left a comment

lgtm

@rjfd

rjfd approved these changes Dec 17, 2018

Copy link
Contributor

left a comment

lgtm

@badone

badone approved these changes Dec 17, 2018

Copy link
Contributor

left a comment

Ingenious. LGTM.

@tchaikov tchaikov changed the title remove cython 0.29's subinterpreter check, re-enable build with cython 0.29+ cmake,deb,rpm: remove cython 0.29's subinterpreter check, re-enable build with cython 0.29+ Dec 19, 2018

@tchaikov tchaikov added the build/ops label Dec 19, 2018

@tchaikov tchaikov merged commit 1189e2e into ceph:master Dec 20, 2018

4 of 5 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
@shaba

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

Hello.
I was compile ceph-14.1.0(with python3 and without python2) with this patch, but have a error for all installed ceph-mgr modules:

ceph-mgr[104044]:   File "/usr/share/ceph/mgr/mgr_module.py", line 9, in <module>
ceph-mgr[104044]:     import rados
ceph-mgr[104044]: ImportError: Interpreter change detected - this module can only be loaded into one interpreter per process.
ceph-mgr[104044]: 2019-03-03 06:51:27.451 7f9c4d9aed00 -1 mgr[py] Class not found in module 'iostat'
ceph-mgr[104044]: 2019-03-03 06:51:27.451 7f9c4d9aed00 -1 mgr[py] Error loading module 'iostat': (22) Invalid argument

@tserong tserong deleted the SUSE:wip-remove-cython-subinterpreter-check branch Mar 4, 2019

@sebastian-philipp

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

which exact cython version do you use?

@shaba

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Cython-0.29.6

@sebastian-philipp

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@shaba could you please create a tracker issue and also include your Linux distribution (+version)?

@tserong

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@shaba was it a completely new, clean checkout, or one that had been built before, then updated? If the latter, was cmake rerun, to recreate the makefiles under build/ before rebuilding?

@shaba

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

@tserong each build was in new clean chroot.
I was build from v14.0.1 tag + several cherry-picked commits. You can see http://git.altlinux.org/people/shaba/packages/ceph.new.git (branch alt-fixes for generate patch, which apply as "%patch -p1" in rpm spec)

@tserong

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@shaba v14.0.1 does not contain this commit, but v14.1.0 does:

# git tag --contains 3bde34af8a
v14.1.0

If you want to know if the commit is applied correctly, after running cmake, grep the build directory for dead_function. You should see matches in build/src/pybind/rados3/CMakeFiles/cython3_rados.dir/build.make, build/src/pybind/rbd3/CMakeFiles/cython3_rbd.dir/build.make, build/src/pybind/cephfs3/CMakeFiles/cython3_cephfs.dir/build.make and build/src/pybind/rgw3/CMakeFiles/cython3_rgw.dir/build.make.

If this doesn't help, please create a tracker issue as @sebastian-philipp requested.

@rgpublic

This comment has been minimized.

Copy link

commented May 2, 2019

Could it be possible that the newest Ubuntu version (Disco Dingo) has Ceph packages that contain this bug? After upgrading I get the sub-interpreter error in the ceph-mgr logs, no modules are loaded and ceph health shows a warning. Should I file a bug in Ubuntu's issue tracker?

@sebastian-philipp

This comment has been minimized.

Copy link
Member

commented May 2, 2019

can you verify that the fix merged here is actually applied to your binaries?

@rgpublic

This comment has been minimized.

Copy link

commented May 2, 2019

@sebastian-philipp I wouldn't know how to do that. They are binaries after all, aren't they? As I understand, the patch influences how ceph is compiled. But I installed the ceph binaries with "apt install ceph" and those packages got upgraded when I upgraded the Ubuntu version. I'm currently quite unsure who is responsible for this issue (Ubuntu team? Ceph?) and where to file a bug for this. But it seems to me that the current Ubuntu version is shipped with a defunct Ceph...

@tserong

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

#27067 is also necessary. This was backported to nautilus in #27128, and is present in the v14.2.1 release. @rgpublic what version of ceph is Ubuntu shipping right now?

@rgpublic

This comment has been minimized.

Copy link

commented May 3, 2019

@tserong v13.2.4. See:
https://packages.ubuntu.com/disco/ceph

Yeah, it's not the newest version they ship. Put many people (like us) are relying on the built-in packages and not on a PPA. No problem that we don't get all the fancy new bells and whistles, but at least it should work. I really wonder how this got through testing...

@tserong

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I can't speak to exactly how this got through testing with 13.2.4, but when the first version of ceph mimic (13.x) was released, cython 0.29 wasn't out, so the problem hadn't manifested yet then.

Anyway, given that mimic is now being built against cython 0.29, looks like we need mimic backports of https://tracker.ceph.com/issues/37472 and https://tracker.ceph.com/issues/38788.

@rgpublic

This comment has been minimized.

Copy link

commented May 6, 2019

@tserong Ah okay, I understand now. Thanks a lot for this information. I didn't mean how the Ceph folks didn't test it, though, but rather the Ubuntu folks. They are even actively "advertising" Ceph on their blog:

Ubuntu 19.04 integrates recent innovations from key open infrastructure projects – like OpenStack, Kubernetes, and Ceph – with advanced life-cycle management for multi-cloud and on-prem operations

Okay, just wanted to make sure I'm not seeing things or configured sth. wrong or it's a different issue with similar symptoms. I'll more or less patiently wait for a fixed version to appear... ;-)

Still, I'm really amazed how Ubuntu could ship their distro with such a major piece of software obviously simply not working. I'd understand if a bug only appeared on some configurations, specific devices etc. But anyone trying to use Ceph on Ubuntu 19.04 would be hit by this AFAIU. Weird, that I'm the first one in the world to notice that considering how widespread Ubuntu is. Hard to believe. Weird!

@tserong

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Weird, that I'm the first one in the world to notice that considering how widespread Ubuntu is.

Someone's gotta be the first ;-) Joking aside, the backport to fix this is in progress (#27971)

@javacruft

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Weird, that I'm the first one in the world to notice that considering how widespread Ubuntu is. Hard to believe. Weird!

You are somewhat a-typical in deploying Ceph on an interim Ubuntu release rather than the most recent LTS; I'm tracking resolution this issue under:

https://bugs.launchpad.net/ubuntu/+source/ceph/+bug/1832105

@rgpublic

This comment has been minimized.

Copy link

commented Jul 3, 2019

Yes, it's not a LTS release, but OTOH it's not a beta release or anything. I think you should be able to either safely upgrade without major components of your server OS suddenly falling apart or at the very least there should have been some kind of warning. And I find it a tad bit disturbing that this isn't caught by automated processes. What would happen if e.g. Apache didn't load any modules after an Ubuntu server upgrade? Not so nice to update your server and getting the shock of your life when "ceph health" shows a weird warning :-( Anyways, thanks a lot for the update, @javacruft. It's much appreciated. I will follow the issue on Launchpad going forward...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.