mgr: load modules in finisher to avoid potential lock cycles #26112

trociny · 2019-01-24T09:08:17Z

Fixes: https://tracker.ceph.com/issues/37997
Signed-off-by: Mykola Golub [email protected]

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

votdev

LGTM. QA tests from #25989 which are testing the module enable/disable functionality and the MgrModule::get_module_option(_ex) methods are successful.

epuertat

Not familiar with the finisher approach, but moving potentially locking stuff to a thread-safe run queue looks ok to me. If this works ok, I'd only favor the other approach (#26092) as it brings less changes in. Let's see what others say!

tchaikov · 2019-01-24T12:57:18Z

src/mgr/StandbyPyModules.cc

+  ceph_assert(modules.count(name) == 0);
+
+  modules[name].reset(new StandbyPyModule(state, py_module, clog));
+  auto standby_module = modules.at(name).get();


why .at() instead of using []?

And why using [] instead of .at(). The advantage of at that it will not try to insert key if it does not exist, thought it is not relevant here. The main rationale was to have the code similar to what we have for ActivePyModules.

okay. it's just weird to do so. as we start worrying about what if it does not exist after inserting an item in the map.

tchaikov · 2019-01-24T12:59:54Z

src/mgr/StandbyPyModules.cc

+
+  ceph_assert(modules.count(name) == 0);
+
+  modules[name].reset(new StandbyPyModule(state, py_module, clog));


could use std::make_unique<> since you are at here.

Again I want it to be the same as it is in ActivePyModules.

fair enough. but IMHO, it's always safer to improve the code piecemeal when touching it instead of always copying from old code as it is.

tchaikov · 2019-01-24T13:00:26Z

src/mgr/StandbyPyModules.cc

-        modules[module_name]->get_name().c_str());
-    return 0;
-  }
+  auto name = py_module->get_name();


could keep const.

tchaikov · 2019-01-24T13:03:55Z

src/mgr/StandbyPyModules.cc

+  // Send all python calls down a Finisher to avoid blocking
+  // C++ code, and avoid any potential lock cycles.
+  finisher.queue(new FunctionContext([this, standby_module, name](int) {
+    std::lock_guard l(lock);


why do you think it's different? we still hold the lock when calling standby_module->load() after this change.

It is a different lock. The lock that caused problem was PyModuleRegistry lock, which is hold by PyModuleRegistry::active_start (and PyModuleRegistry::standby_start) and later may be acquired from python.

Probably it is safe not to hold this lock here too and only reacquire it if load failed and we need to remove it from modules.

ic. and agreed. we don't need to hold the lock unless we are going to access this->modules.

trociny · 2019-01-24T16:14:32Z

updated

dotnwat · 2019-01-25T00:08:10Z

the locking in mgr seems way more complex than necessary, but refactoring that could be a big project. this seems like a reasonable way to lubricate the gears of progress.

dotnwat · 2019-01-25T00:12:07Z

i'm not sure its relevant, but i worked around a lockdep issue as well and it seems like the same ActivePyModules area https://github.com/ceph/ceph/blob/master/src/mgr/ActivePyModules.cc#L869

dotnwat · 2019-01-25T00:19:10Z

src/mgr/ActivePyModules.cc

+
+  // Send all python calls down a Finisher to avoid blocking
+  // C++ code, and avoid any potential lock cycles.
+  finisher.queue(new FunctionContext([this, active_module, name](int) {


what exactly is the cycle that is being avoided? it seems like before the finisher approach all of the calls were synchronous and any deadlock would have always been triggered.

tchaikov · 2019-01-27T03:15:17Z

/a/kchai-2019-01-26_16:44:54-rados-wip-kefu2-testing-2019-01-26-2047-distro-basic-smithi/3512033

 ceph version 14.0.1-2962-g17e2a8b (17e2a8b61ebecf8534ccd79e30a22d18fec9bf2b) nautilus (dev)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x14a) [0x7fc679552a4a]
 2: (ceph::__ceph_assertf_fail(char const*, char const*, int, char const*, char const*, ...)+0) [0x7fc679552c18]
 3: (()+0x2a608c) [0x7fc6795c608c]
 4: (SafeTimer::shutdown()+0x78) [0x7fc6795db828]
 5: (()+0x1a2f23) [0x55cef2342f23]
 6: (FunctionContext::finish(int)+0x2c) [0x55cef22c6a3c]
 7: (Context::complete(int)+0x9) [0x55cef22c31a9]
 8: (Finisher::finisher_thread_entry()+0x156) [0x7fc679594146]
 9: (()+0x7dd5) [0x7fc676c23dd5]
 10: (clone()+0x6d) [0x7fc6758d3ead]

b-ranto

lgtm

LenzGr · 2019-01-30T14:53:23Z

Adding this to the nautilus milestone. Now that we closed #26092 in favor of this one, what's missing on getting it merged?

tchaikov · 2019-01-30T15:15:40Z

failures are tracked by http://tracker.ceph.com/issues/23947

trociny · 2019-01-30T15:16:38Z

Sorry, I missed the comment from @tchaikov about the crash. Now looking at it. It looks related and is due to I missed the step to reacquire the lock in the queued content in MgrStandby::shutdown. Strange, I did not see this issue when running locally.

I will update the PR.

Fixes: https://tracker.ceph.com/issues/37997 Signed-off-by: Mykola Golub <[email protected]>

trociny · 2019-01-30T15:51:17Z

I reproduced the crash and fixed.

liewegas · 2019-02-01T18:40:31Z

http://pulpito.ceph.com/sage-2019-01-31_22:28:36-rados-wip-sage2-testing-2019-01-30-1657-distro-basic-smithi/

* refs/pull/26112/head: mgr: load modules in finisher to avoid potential lock cycles Reviewed-by: Boris Ranto <[email protected]> Reviewed-by: Volker Theile <[email protected]> Reviewed-by: Kefu Chai <[email protected]> Reviewed-by: Ernesto Puerta <[email protected]>

It seems that the current implementation has some side effects which results in segfaults in the Mgr. It looks like boxing parameters for feeding them into a Python function written in C++ does not work very well or we did not handle the GIL correctly here. Because of that the old code from the ceph_(get|set)_module_option functions are used again (see ceph#25651). Thus we have some lines of more or less duplicate code, but we get rid of the problems that arise from the current implementation. Previous improvements like ceph#26112, ceph#26240 and ceph#26149 are still useful for the ceph_(get|set)_module_option_ex functions. Signed-off-by: Volker Theile <[email protected]>

trociny added bug-fix mgr labels Jan 24, 2019

trociny mentioned this pull request Jan 24, 2019

mgr: ceph-mgr crashing due to "recursive lock of PyModuleRegistry::lock" #26092

Closed

3 tasks

votdev requested a review from epuertat January 24, 2019 09:20

LenzGr requested review from votdev and removed request for epuertat January 24, 2019 09:21

votdev requested a review from b-ranto January 24, 2019 09:21

LenzGr requested review from dotnwat, tchaikov and sebastian-philipp and removed request for b-ranto January 24, 2019 09:21

votdev requested review from b-ranto and epuertat and removed request for dotnwat, tchaikov and sebastian-philipp January 24, 2019 09:21

LenzGr requested a review from sebastian-philipp January 24, 2019 09:21

votdev requested review from tchaikov and dotnwat January 24, 2019 09:22

votdev approved these changes Jan 24, 2019

View reviewed changes

epuertat approved these changes Jan 24, 2019

View reviewed changes

tchaikov reviewed Jan 24, 2019

View reviewed changes

trociny force-pushed the wip-37997 branch from ad3cf48 to cbb6dcc Compare January 24, 2019 16:13

tchaikov approved these changes Jan 24, 2019

View reviewed changes

dotnwat reviewed Jan 25, 2019

View reviewed changes

tchaikov added wip-kefu2-testing and removed wip-kefu2-testing labels Jan 25, 2019

tchaikov added the needs-qa label Jan 27, 2019

liewegas added the wip-sage2-testing label Jan 28, 2019

b-ranto approved these changes Jan 28, 2019

View reviewed changes

LenzGr added this to the nautilus milestone Jan 30, 2019

trociny added the DNM label Jan 30, 2019

mgr: load modules in finisher to avoid potential lock cycles

f22347d

Fixes: https://tracker.ceph.com/issues/37997 Signed-off-by: Mykola Golub <[email protected]>

trociny force-pushed the wip-37997 branch from cbb6dcc to f22347d Compare January 30, 2019 15:48

trociny removed the DNM label Jan 30, 2019

liewegas merged commit f22347d into ceph:master Feb 1, 2019

votdev mentioned this pull request Feb 5, 2019

mgr: Remove _ceph_(g|s)et_module_option_ex() method #26280

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr: load modules in finisher to avoid potential lock cycles #26112

mgr: load modules in finisher to avoid potential lock cycles #26112

trociny commented Jan 24, 2019

votdev left a comment •

edited

Loading

epuertat left a comment

tchaikov Jan 24, 2019

trociny Jan 24, 2019

tchaikov Jan 24, 2019

tchaikov Jan 24, 2019

trociny Jan 24, 2019

tchaikov Jan 24, 2019 •

edited

Loading

tchaikov Jan 24, 2019

trociny Jan 24, 2019

tchaikov Jan 24, 2019

trociny Jan 24, 2019

tchaikov Jan 24, 2019

trociny Jan 24, 2019

trociny commented Jan 24, 2019

dotnwat commented Jan 25, 2019

dotnwat commented Jan 25, 2019

dotnwat Jan 25, 2019

tchaikov commented Jan 27, 2019 •

edited

Loading

b-ranto left a comment

LenzGr commented Jan 30, 2019

tchaikov commented Jan 30, 2019

trociny commented Jan 30, 2019

trociny commented Jan 30, 2019 •

edited

Loading

liewegas commented Feb 1, 2019


		ceph_assert(modules.count(name) == 0);

		modules[name].reset(new StandbyPyModule(state, py_module, clog));

mgr: load modules in finisher to avoid potential lock cycles #26112

mgr: load modules in finisher to avoid potential lock cycles #26112

Conversation

trociny commented Jan 24, 2019

votdev left a comment • edited Loading

Choose a reason for hiding this comment

epuertat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trociny commented Jan 24, 2019

dotnwat commented Jan 25, 2019

dotnwat commented Jan 25, 2019

Choose a reason for hiding this comment

tchaikov commented Jan 27, 2019 • edited Loading

b-ranto left a comment

Choose a reason for hiding this comment

LenzGr commented Jan 30, 2019

tchaikov commented Jan 30, 2019

trociny commented Jan 30, 2019

trociny commented Jan 30, 2019 • edited Loading

liewegas commented Feb 1, 2019

votdev left a comment •

edited

Loading

tchaikov Jan 24, 2019 •

edited

Loading

tchaikov commented Jan 27, 2019 •

edited

Loading

trociny commented Jan 30, 2019 •

edited

Loading