From 633998983c2bd69c003bc343ff65872440be5439 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Thu, 12 Oct 2017 10:45:12 -0700 Subject: [PATCH] Use "poll" or "selects" Eventlet hub for all Swift daemons. Previously, Swift's WSGI servers, the object replicator, and the object reconstructor were setting Eventlet's hub to either "poll" or "selects", depending on availability. Other daemons were letting Eventlet use its default hub, which is "epoll". In any daemons that fork, we really don't want to use epoll. Epoll instances end up shared between the parent and all children, and you get some awful messes when file descriptors are shared. Here's an example where two processes are trying to wait on the same file descriptor using the same epoll instance, and everything goes wrong: [proc A] epoll_ctl(6, EPOLL_CTL_ADD, 3, ...) = 0 [proc B] epoll_ctl(6, EPOLL_CTL_ADD, 3, ...) = -1 EEXIST (File exists) [proc B] epoll_wait(6, ...) = 1 [proc B] epoll_ctl(6, EPOLL_CTL_DEL, 3, ...) = 0 [proc A] epoll_wait(6, ...) This primarily affects the container updater and object updater since they fork. I've decided to change the hub for all Swift daemons so that we don't add multiprocessing support to some other daemon someday and suffer through this same bug again. This problem was made more apparent by commit 6d16079, which made our logging mutex use file descriptors. However, it could have struck on any shared file descriptor on which a read or write returned EAGAIN. Change-Id: Ic2c1178ac918c88b0b901e581eb4fab3b2666cfe Closes-Bug: 1722951 --- swift/common/daemon.py | 3 +++ swift/common/utils.py | 19 +++++++++++++++++++ swift/obj/reconstructor.py | 8 ++------ swift/obj/replicator.py | 6 ++---- test/unit/common/test_daemon.py | 5 ++++- 5 files changed, 30 insertions(+), 11 deletions(-) diff --git a/swift/common/daemon.py b/swift/common/daemon.py index 39fc66c805..7127b6c58e 100644 --- a/swift/common/daemon.py +++ b/swift/common/daemon.py @@ -20,6 +20,7 @@ import signal from re import sub import eventlet.debug +from eventlet.hubs import use_hub from swift.common import utils @@ -83,6 +84,8 @@ def run_daemon(klass, conf_file, section_name='', once=False, **kwargs): # and results in an exit code of 1. sys.exit(e) + use_hub(utils.get_hub()) + # once on command line (i.e. daemonize=false) will over-ride config once = once or not utils.config_true_value(conf.get('daemonize', 'true')) diff --git a/swift/common/utils.py b/swift/common/utils.py index 3add489a9d..eb8bb76d0a 100644 --- a/swift/common/utils.py +++ b/swift/common/utils.py @@ -1938,6 +1938,25 @@ def get_hub(): getting swallowed somewhere. Then when that file descriptor was re-used, eventlet would freak right out because it still thought it was waiting for activity from it in some other coro. + + Another note about epoll: it's hard to use when forking. epoll works + like so: + + * create an epoll instance: efd = epoll_create(...) + + * register file descriptors of interest with epoll_ctl(efd, + EPOLL_CTL_ADD, fd, ...) + + * wait for events with epoll_wait(efd, ...) + + If you fork, you and all your child processes end up using the same + epoll instance, and everyone becomes confused. It is possible to use + epoll and fork and still have a correct program as long as you do the + right things, but eventlet doesn't do those things. Really, it can't + even try to do those things since it doesn't get notified of forks. + + In contrast, both poll() and select() specify the set of interesting + file descriptors with each call, so there's no problem with forking. """ try: import select diff --git a/swift/obj/reconstructor.py b/swift/obj/reconstructor.py index 35d70b01df..711f87be97 100644 --- a/swift/obj/reconstructor.py +++ b/swift/obj/reconstructor.py @@ -25,14 +25,13 @@ import six import six.moves.cPickle as pickle import shutil -from eventlet import (GreenPile, GreenPool, Timeout, sleep, hubs, tpool, - spawn) +from eventlet import (GreenPile, GreenPool, Timeout, sleep, tpool, spawn) from eventlet.support.greenlets import GreenletExit from swift import gettext_ as _ from swift.common.utils import ( whataremyips, unlink_older_than, compute_eta, get_logger, - dump_recon_cache, mkdirs, config_true_value, list_from_csv, get_hub, + dump_recon_cache, mkdirs, config_true_value, list_from_csv, tpool_reraise, GreenAsyncPile, Timestamp, remove_file) from swift.common.header_key_dict import HeaderKeyDict from swift.common.bufferedhttp import http_connect @@ -50,9 +49,6 @@ from swift.common.exceptions import ConnectionTimeout, DiskFileError, \ SYNC, REVERT = ('sync_only', 'sync_revert') -hubs.use_hub(get_hub()) - - def _get_partners(frag_index, part_nodes): """ Returns the left and right partners of the node whose index is diff --git a/swift/obj/replicator.py b/swift/obj/replicator.py index fc7f353f0f..0b016c30f5 100644 --- a/swift/obj/replicator.py +++ b/swift/obj/replicator.py @@ -25,7 +25,7 @@ import six.moves.cPickle as pickle from swift import gettext_ as _ import eventlet -from eventlet import GreenPool, tpool, Timeout, sleep, hubs +from eventlet import GreenPool, tpool, Timeout, sleep from eventlet.green import subprocess from eventlet.support.greenlets import GreenletExit @@ -33,7 +33,7 @@ from swift.common.ring.utils import is_local_device from swift.common.utils import whataremyips, unlink_older_than, \ compute_eta, get_logger, dump_recon_cache, ismount, \ rsync_module_interpolation, mkdirs, config_true_value, list_from_csv, \ - get_hub, tpool_reraise, config_auto_int_value, storage_directory + tpool_reraise, config_auto_int_value, storage_directory from swift.common.bufferedhttp import http_connect from swift.common.daemon import Daemon from swift.common.http import HTTP_OK, HTTP_INSUFFICIENT_STORAGE @@ -43,8 +43,6 @@ from swift.common.storage_policy import POLICIES, REPL_POLICY DEFAULT_RSYNC_TIMEOUT = 900 -hubs.use_hub(get_hub()) - def _do_listdir(partition, replication_cycle): return (((partition + replication_cycle) % 10) == 0) diff --git a/test/unit/common/test_daemon.py b/test/unit/common/test_daemon.py index 92376ad8b2..a8bd9ddae6 100644 --- a/test/unit/common/test_daemon.py +++ b/test/unit/common/test_daemon.py @@ -102,11 +102,14 @@ class TestRunDaemon(unittest.TestCase): def test_run_daemon(self): sample_conf = "[my-daemon]\nuser = %s\n" % getuser() - with tmpfile(sample_conf) as conf_file: + with tmpfile(sample_conf) as conf_file, \ + mock.patch('swift.common.daemon.use_hub') as mock_use_hub: with mock.patch.dict('os.environ', {'TZ': ''}): daemon.run_daemon(MyDaemon, conf_file) self.assertEqual(MyDaemon.forever_called, True) self.assertTrue(os.environ['TZ'] is not '') + self.assertEqual(mock_use_hub.mock_calls, + [mock.call(utils.get_hub())]) daemon.run_daemon(MyDaemon, conf_file, once=True) self.assertEqual(MyDaemon.once_called, True)