From 8705f67bfbd733edf923003c0a5850e7a4a5735b Mon Sep 17 00:00:00 2001 From: damani42 Date: Tue, 7 May 2024 12:03:59 +0200 Subject: [PATCH] Remove the usage of the Eventlet debug feature from oslo.log. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We initially started to work on this topic with the intention to solve the problem of using the Eventlet hub_prevent_multiple_readers debug feature. This feature is a debug convenience. This feature is not supported by the new asyncio hub[1] of Eventlet. You can also check the documentation here[2] from eventlet. The problem with disabling this procedure is that it exposes you to risk. Deactivation applies to the entire stack. If a project uses oslo.log, for example nova, then it exposes all threads to concurrent access on the process file descriptors. As explained in the function's documentation[3], when several greenlets are reading from the same socket, it's difficult to predict which greenlet will receive which data. You really need to be sure of what you're doing to use this function. Also explained in the raise condition added by this commit[4] and we have a reproducer[5] that exposes the problem. According to our tests, removing this line does not change the fix provided. It also seems that cinder no longer uses[6] logging native thread. Related-Bug: #1983863 [1] https://review.opendev.org/c/openstack/governance/+/902585/17..18 [2] https://eventlet.readthedocs.io/en/latest/asyncio/migration.html#known-limitations-and-work-in-progress [3] https://github.com/eventlet/eventlet/blob/master/eventlet/debug.py#L149 [4] https://github.com/eventlet/eventlet/commit/cb7c8c0196ed70665b0382909141ac743d7633a2 [5] https://github.com/4383/oslo.log/blob/reproducer2/reproducer.py [6] https://bugs.launchpad.net/oslo.log/+bug/1983863/comments/18 Co-authored-by: Hervé Beraud Change-Id: Ia4c8e82f957c4ef3c176246fea0bc0adab4f4dc4 --- oslo_log/pipe_mutex.py | 15 --------------- oslo_log/tests/unit/test_pipe_mutex.py | 14 ++++---------- ...t_multiple_readers-usage-b23cea69a05cd6d4.yaml | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 25 deletions(-) create mode 100644 releasenotes/notes/remove-eventlet-hub_prevent_multiple_readers-usage-b23cea69a05cd6d4.yaml diff --git a/oslo_log/pipe_mutex.py b/oslo_log/pipe_mutex.py index 825735ed..8fbab699 100644 --- a/oslo_log/pipe_mutex.py +++ b/oslo_log/pipe_mutex.py @@ -43,21 +43,6 @@ class PipeMutex(object): self.owner = None self.recursion_depth = 0 - # Usually, it's an error to have multiple greenthreads all waiting - # to read the same file descriptor. It's often a sign of inadequate - # concurrency control; for example, if you have two greenthreads - # trying to use the same memcache connection, they'll end up writing - # interleaved garbage to the socket or stealing part of each others' - # responses. - # - # In this case, we have multiple greenthreads waiting on the same - # file descriptor by design. This lets greenthreads in real thread A - # wait with greenthreads in real thread B for the same mutex. - # Therefore, we must turn off eventlet's multiple-reader detection. - # - # It would be better to turn off multiple-reader detection for only - # our calls to trampoline(), but eventlet does not support that. - eventlet.debug.hub_prevent_multiple_readers(False) def acquire(self, blocking=True): """Acquire the mutex. diff --git a/oslo_log/tests/unit/test_pipe_mutex.py b/oslo_log/tests/unit/test_pipe_mutex.py index c7b75915..4ff5d4de 100644 --- a/oslo_log/tests/unit/test_pipe_mutex.py +++ b/oslo_log/tests/unit/test_pipe_mutex.py @@ -20,6 +20,7 @@ from unittest import mock import eventlet from eventlet import debug as eventlet_debug from eventlet import greenpool +from eventlet import tpool from oslo_log import pipe_mutex @@ -35,7 +36,7 @@ def quiet_eventlet_exceptions(): class TestPipeMutex(unittest.TestCase): - """From Swift's test/unit/common/test_utils.py""" + """From Swift's test/unit/common/test_utils.py""" def setUp(self): self.mutex = pipe_mutex.PipeMutex() @@ -141,18 +142,11 @@ class TestPipeMutex(unittest.TestCase): greenthread1 = eventlet.spawn(do_stuff) greenthread2 = eventlet.spawn(do_stuff) - real_thread1 = eventlet.patcher.original('threading').Thread( - target=do_stuff) - real_thread1.start() - - real_thread2 = eventlet.patcher.original('threading').Thread( - target=do_stuff) - real_thread2.start() + _ = tpool.execute(do_stuff) + _ = tpool.execute(do_stuff) greenthread1.wait() greenthread2.wait() - real_thread1.join() - real_thread2.join() self.assertEqual(''.join(sequence), "<>" * 40) diff --git a/releasenotes/notes/remove-eventlet-hub_prevent_multiple_readers-usage-b23cea69a05cd6d4.yaml b/releasenotes/notes/remove-eventlet-hub_prevent_multiple_readers-usage-b23cea69a05cd6d4.yaml new file mode 100644 index 00000000..958805b1 --- /dev/null +++ b/releasenotes/notes/remove-eventlet-hub_prevent_multiple_readers-usage-b23cea69a05cd6d4.yaml @@ -0,0 +1,14 @@ +--- +fixes: + - | + Remove the usage of the Eventlet debug feature from oslo.log + Then hub_prevent_multiple_readers is a debug convenience. This + feature is a debug convenience. The problem with disabling this + procedure is that it exposes you to risk. Deactivation applies + to the entire stack. If a project uses oslo.log, for example nova, + then it exposes all threads to concurrent access on the process file + descriptors. When several greenlets are reading from the same socket, + it's difficult to predict which greenlet will receive which data. That + services using oslo.log should prefer using tpool rather than un-monkey + patched version of stdlib threading module which is not compatible with + eventlet.