From c767ec9a37faa4414702aa9e39231afe0ba3b097 Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Wed, 9 Jul 2014 16:04:20 -0700 Subject: [PATCH 01/19] Let eventlet.wsgi.server log tracebacks when eventlet_debug is enabled The "logging" available in eventlet.wsgi.server/BaseHTTPServer doesn't generally suite our needs, so it should be bypassed using a NullLogger in production. But in development it can be useful if tracebacks generated from inside eventlet.wsgi (say a NameError in DiskFile.__iter__) end up in logs. Since we already have eventlet_debug parsed inside of run_server we can skip the NullLogger bypass and let stuff blast out to STDERR when configured for development/debug logging. Change-Id: I20a9e82c7fed8948bf649f1f8571b4145fca201d --- swift/common/wsgi.py | 8 +++-- test/unit/common/test_wsgi.py | 59 ++++++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index 5291746e6b..b1e1f5ea73 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -380,6 +380,10 @@ def run_server(conf, logger, sock, global_conf=None): eventlet.patcher.monkey_patch(all=False, socket=True) eventlet_debug = config_true_value(conf.get('eventlet_debug', 'no')) eventlet.debug.hub_exceptions(eventlet_debug) + wsgi_logger = NullLogger() + if eventlet_debug: + # let eventlet.wsgi.server log to stderr + wsgi_logger = None # utils.LogAdapter stashes name in server; fallback on unadapted loggers if not global_conf: if hasattr(logger, 'server'): @@ -395,10 +399,10 @@ def run_server(conf, logger, sock, global_conf=None): # necessary for the AWS SDK to work with swift3 middleware. argspec = inspect.getargspec(wsgi.server) if 'capitalize_response_headers' in argspec.args: - wsgi.server(sock, app, NullLogger(), custom_pool=pool, + wsgi.server(sock, app, wsgi_logger, custom_pool=pool, capitalize_response_headers=False) else: - wsgi.server(sock, app, NullLogger(), custom_pool=pool) + wsgi.server(sock, app, wsgi_logger, custom_pool=pool) except socket.error as err: if err[0] != errno.EINVAL: raise diff --git a/test/unit/common/test_wsgi.py b/test/unit/common/test_wsgi.py index aa10264898..67142decdd 100644 --- a/test/unit/common/test_wsgi.py +++ b/test/unit/common/test_wsgi.py @@ -317,7 +317,6 @@ class TestWSGI(unittest.TestCase): def test_run_server(self): config = """ [DEFAULT] - eventlet_debug = yes client_timeout = 30 max_clients = 1000 swift_dir = TEMPDIR @@ -354,7 +353,7 @@ class TestWSGI(unittest.TestCase): _eventlet.hubs.use_hub.assert_called_with(utils.get_hub()) _eventlet.patcher.monkey_patch.assert_called_with(all=False, socket=True) - _eventlet.debug.hub_exceptions.assert_called_with(True) + _eventlet.debug.hub_exceptions.assert_called_with(False) _wsgi.server.assert_called() args, kwargs = _wsgi.server.call_args server_sock, server_app, server_logger = args @@ -414,7 +413,6 @@ class TestWSGI(unittest.TestCase): """, 'proxy-server.conf.d/default.conf': """ [DEFAULT] - eventlet_debug = yes client_timeout = 30 """ } @@ -443,7 +441,7 @@ class TestWSGI(unittest.TestCase): _eventlet.hubs.use_hub.assert_called_with(utils.get_hub()) _eventlet.patcher.monkey_patch.assert_called_with(all=False, socket=True) - _eventlet.debug.hub_exceptions.assert_called_with(True) + _eventlet.debug.hub_exceptions.assert_called_with(False) _wsgi.server.assert_called() args, kwargs = _wsgi.server.call_args server_sock, server_app, server_logger = args @@ -452,6 +450,59 @@ class TestWSGI(unittest.TestCase): self.assert_(isinstance(server_logger, wsgi.NullLogger)) self.assert_('custom_pool' in kwargs) + def test_run_server_debug(self): + config = """ + [DEFAULT] + eventlet_debug = yes + client_timeout = 30 + max_clients = 1000 + swift_dir = TEMPDIR + + [pipeline:main] + pipeline = proxy-server + + [app:proxy-server] + use = egg:swift#proxy + # while "set" values normally override default + set client_timeout = 20 + # this section is not in conf during run_server + set max_clients = 10 + """ + + contents = dedent(config) + with temptree(['proxy-server.conf']) as t: + conf_file = os.path.join(t, 'proxy-server.conf') + with open(conf_file, 'w') as f: + f.write(contents.replace('TEMPDIR', t)) + _fake_rings(t) + with mock.patch('swift.proxy.server.Application.' + 'modify_wsgi_pipeline'): + with mock.patch('swift.common.wsgi.wsgi') as _wsgi: + mock_server = _wsgi.server + _wsgi.server = lambda *args, **kwargs: mock_server( + *args, **kwargs) + with mock.patch('swift.common.wsgi.eventlet') as _eventlet: + conf = wsgi.appconfig(conf_file) + logger = logging.getLogger('test') + sock = listen(('localhost', 0)) + wsgi.run_server(conf, logger, sock) + self.assertEquals('HTTP/1.0', + _wsgi.HttpProtocol.default_request_version) + self.assertEquals(30, _wsgi.WRITE_TIMEOUT) + _eventlet.hubs.use_hub.assert_called_with(utils.get_hub()) + _eventlet.patcher.monkey_patch.assert_called_with(all=False, + socket=True) + _eventlet.debug.hub_exceptions.assert_called_with(True) + mock_server.assert_called() + args, kwargs = mock_server.call_args + server_sock, server_app, server_logger = args + self.assertEquals(sock, server_sock) + self.assert_(isinstance(server_app, swift.proxy.server.Application)) + self.assertEquals(20, server_app.client_timeout) + self.assertEqual(server_logger, None) + self.assert_('custom_pool' in kwargs) + self.assertEquals(1000, kwargs['custom_pool'].size) + def test_appconfig_dir_ignores_hidden_files(self): config_dir = { 'server.conf.d/01.conf': """ From 6354e2da57a8d487caf3605d4005134f584cf935 Mon Sep 17 00:00:00 2001 From: Gerry Drudy Date: Mon, 15 Sep 2014 11:52:14 +0100 Subject: [PATCH 02/19] direct_client not passing args between some functions The call to _get_direct_account_container in direct_get_account has several of its args =None instead of set to the value passed to direct_get_account. The same applies to _get_direct_account_container in direct_get_container. The direct_get_container is only called by the account-reaper and this bug will have limited impact on it. The marker, maintained in reap_container, is ignored by direct_get_container. This is not as bad as it sounds, if the account-reaper successfully deletes the first 10K objects, assuming the container has > 10K objects, the next call to direct_get_container will in fact return the next 10K objects even though it sets marker=None (assuming the first 10K objects were successfully deleted). This patch also updates test_direct_get_account and test_direct_get_container to ensure the appropriate args are included in the connection query_string. Closes-Bug: #1369558 Change-Id: If1c8aa1240d38354ebc9b1ebca92dc1c8c36cb5f --- swift/common/direct_client.py | 24 ++++++++++++------------ test/unit/common/test_direct_client.py | 17 +++++++++++++++-- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/swift/common/direct_client.py b/swift/common/direct_client.py index 3fe2c236ae..35ca24a64c 100644 --- a/swift/common/direct_client.py +++ b/swift/common/direct_client.py @@ -108,7 +108,7 @@ def direct_get_account(node, part, account, marker=None, limit=None, :param marker: marker query :param limit: query limit :param prefix: prefix query - :param delimeter: delimeter for the query + :param delimiter: delimiter for the query :param conn_timeout: timeout in seconds for establishing the connection :param response_timeout: timeout in seconds for getting the response :returns: a tuple of (response headers, a list of containers) The response @@ -116,11 +116,11 @@ def direct_get_account(node, part, account, marker=None, limit=None, """ path = '/' + account return _get_direct_account_container(path, "Account", node, part, - account, marker=None, - limit=None, prefix=None, - delimiter=None, - conn_timeout=5, - response_timeout=15) + account, marker=marker, + limit=limit, prefix=prefix, + delimiter=delimiter, + conn_timeout=conn_timeout, + response_timeout=response_timeout) def direct_delete_account(node, part, account, conn_timeout=5, @@ -183,7 +183,7 @@ def direct_get_container(node, part, account, container, marker=None, :param marker: marker query :param limit: query limit :param prefix: prefix query - :param delimeter: delimeter for the query + :param delimiter: delimiter for the query :param conn_timeout: timeout in seconds for establishing the connection :param response_timeout: timeout in seconds for getting the response :returns: a tuple of (response headers, a list of objects) The response @@ -191,11 +191,11 @@ def direct_get_container(node, part, account, container, marker=None, """ path = '/%s/%s' % (account, container) return _get_direct_account_container(path, "Container", node, - part, account, marker=None, - limit=None, prefix=None, - delimiter=None, - conn_timeout=5, - response_timeout=15) + part, account, marker=marker, + limit=limit, prefix=prefix, + delimiter=delimiter, + conn_timeout=conn_timeout, + response_timeout=response_timeout) def direct_delete_container(node, part, account, container, conn_timeout=5, diff --git a/test/unit/common/test_direct_client.py b/test/unit/common/test_direct_client.py index 535bc991d8..d41a7c9672 100644 --- a/test/unit/common/test_direct_client.py +++ b/test/unit/common/test_direct_client.py @@ -161,13 +161,19 @@ class TestDirectClient(unittest.TestCase): with mocked_http_conn(200, stub_headers, body) as conn: resp_headers, resp = direct_client.direct_get_account( - self.node, self.part, self.account) + self.node, self.part, self.account, marker='marker', + prefix='prefix', delimiter='delimiter', limit=1000) self.assertEqual(conn.method, 'GET') self.assertEqual(conn.path, self.account_path) self.assertEqual(conn.req_headers['user-agent'], self.user_agent) self.assertEqual(resp_headers, stub_headers) self.assertEqual(json.loads(body), resp) + self.assertTrue('marker=marker' in conn.query_string) + self.assertTrue('delimiter=delimiter' in conn.query_string) + self.assertTrue('limit=1000' in conn.query_string) + self.assertTrue('prefix=prefix' in conn.query_string) + self.assertTrue('format=json' in conn.query_string) def test_direct_client_exception(self): stub_headers = {'X-Trans-Id': 'txb5f59485c578460f8be9e-0053478d09'} @@ -302,12 +308,19 @@ class TestDirectClient(unittest.TestCase): with mocked_http_conn(200, headers, body) as conn: resp_headers, resp = direct_client.direct_get_container( - self.node, self.part, self.account, self.container) + self.node, self.part, self.account, self.container, + marker='marker', prefix='prefix', delimiter='delimiter', + limit=1000) self.assertEqual(conn.req_headers['user-agent'], 'direct-client %s' % os.getpid()) self.assertEqual(headers, resp_headers) self.assertEqual(json.loads(body), resp) + self.assertTrue('marker=marker' in conn.query_string) + self.assertTrue('delimiter=delimiter' in conn.query_string) + self.assertTrue('limit=1000' in conn.query_string) + self.assertTrue('prefix=prefix' in conn.query_string) + self.assertTrue('format=json' in conn.query_string) def test_direct_get_container_no_content_does_not_decode_body(self): headers = {} From 23c2d95e98ee5edf317da55e3ee4c303a18991b6 Mon Sep 17 00:00:00 2001 From: Samuel Merritt Date: Tue, 23 Sep 2014 12:32:42 -0700 Subject: [PATCH 03/19] Remove unused option from ObjectReplicator.__init__ disk_chunk_size is unused by the ObjectReplicator class. Note that it *is* used by the object replicator process; it's just that DiskFileManager is the one that pulls it out of the conf and pays attention to it. Keeping it as an attribute on ObjectReplicator is unnecessary. Change-Id: I1eeef7b33873b4c8bb269ca02dcb067098b6fded --- swift/obj/replicator.py | 1 - 1 file changed, 1 deletion(-) diff --git a/swift/obj/replicator.py b/swift/obj/replicator.py index 6994ca39cb..e5b5536cb6 100644 --- a/swift/obj/replicator.py +++ b/swift/obj/replicator.py @@ -83,7 +83,6 @@ class ObjectReplicator(Daemon): self.node_timeout = float(conf.get('node_timeout', 10)) self.sync_method = getattr(self, conf.get('sync_method') or 'rsync') self.network_chunk_size = int(conf.get('network_chunk_size', 65536)) - self.disk_chunk_size = int(conf.get('disk_chunk_size', 65536)) self.headers = { 'Content-Length': '0', 'user-agent': 'object-replicator %s' % os.getpid()} From 781a1620048edbbc057b15c94da390847b34d9df Mon Sep 17 00:00:00 2001 From: Michael Barton Date: Thu, 25 Sep 2014 16:29:57 +0000 Subject: [PATCH 04/19] use get_container_info in ratelimit Everyone else cooperates to store the container_info cache in the env the first time it's requested. This should save a duplicate memcache hit on requests that ratelimiting looks at. Change-Id: Ic6411d4619db6b53fafe9fdbf1d0a370d1258c38 --- swift/common/middleware/ratelimit.py | 16 +++----- test/unit/common/middleware/test_ratelimit.py | 37 ++++++++++--------- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/swift/common/middleware/ratelimit.py b/swift/common/middleware/ratelimit.py index 6e8dd87298..06823cd775 100644 --- a/swift/common/middleware/ratelimit.py +++ b/swift/common/middleware/ratelimit.py @@ -18,8 +18,7 @@ from swift import gettext_ as _ import eventlet from swift.common.utils import cache_from_env, get_logger, register_swift_info -from swift.proxy.controllers.base import get_container_memcache_key, \ - get_account_info +from swift.proxy.controllers.base import get_account_info, get_container_info from swift.common.memcached import MemcacheConnectionError from swift.common.swob import Request, Response @@ -118,11 +117,10 @@ class RateLimitMiddleware(object): self.container_listing_ratelimits = interpret_conf_limits( conf, 'container_listing_ratelimit_') - def get_container_size(self, account_name, container_name): + def get_container_size(self, env): rv = 0 - memcache_key = get_container_memcache_key(account_name, - container_name) - container_info = self.memcache_client.get(memcache_key) + container_info = get_container_info( + env, self.app, swift_source='RL') if isinstance(container_info, dict): rv = container_info.get( 'object_count', container_info.get('container_size', 0)) @@ -149,8 +147,7 @@ class RateLimitMiddleware(object): if account_name and container_name and obj_name and \ req.method in ('PUT', 'DELETE', 'POST', 'COPY'): - container_size = self.get_container_size( - account_name, container_name) + container_size = self.get_container_size(req.environ) container_rate = get_maxrate( self.container_ratelimits, container_size) if container_rate: @@ -160,8 +157,7 @@ class RateLimitMiddleware(object): if account_name and container_name and not obj_name and \ req.method == 'GET': - container_size = self.get_container_size( - account_name, container_name) + container_size = self.get_container_size(req.environ) container_rate = get_maxrate( self.container_listing_ratelimits, container_size) if container_rate: diff --git a/test/unit/common/middleware/test_ratelimit.py b/test/unit/common/middleware/test_ratelimit.py index a502bafc99..12940eac85 100644 --- a/test/unit/common/middleware/test_ratelimit.py +++ b/test/unit/common/middleware/test_ratelimit.py @@ -189,7 +189,7 @@ class TestRateLimit(unittest.TestCase): the_app = ratelimit.filter_factory(conf_dict)(FakeApp()) the_app.memcache_client = fake_memcache req = lambda: None - req.environ = {} + req.environ = {'swift.cache': fake_memcache, 'PATH_INFO': '/v1/a/c/o'} with mock.patch('swift.common.middleware.ratelimit.get_account_info', lambda *args, **kwargs: {}): req.method = 'DELETE' @@ -243,7 +243,7 @@ class TestRateLimit(unittest.TestCase): the_app.memcache_client = fake_memcache req = lambda: None req.method = 'PUT' - req.environ = {} + req.environ = {'PATH_INFO': '/v1/a/c/o', 'swift.cache': fake_memcache} with mock.patch('swift.common.middleware.ratelimit.get_account_info', lambda *args, **kwargs: {}): tuples = the_app.get_ratelimitable_key_tuples(req, 'a', 'c', 'o') @@ -255,20 +255,23 @@ class TestRateLimit(unittest.TestCase): conf_dict = {'account_ratelimit': current_rate} self.test_ratelimit = ratelimit.filter_factory(conf_dict)(FakeApp()) ratelimit.http_connect = mock_http_connect(204) - with mock.patch('swift.common.middleware.ratelimit.get_account_info', + with mock.patch('swift.common.middleware.ratelimit.get_container_info', lambda *args, **kwargs: {}): - for meth, exp_time in [ - ('DELETE', 9.8), ('GET', 0), ('POST', 0), ('PUT', 9.8)]: - req = Request.blank('/v/a%s/c' % meth) - req.method = meth - req.environ['swift.cache'] = FakeMemcache() - make_app_call = lambda: self.test_ratelimit(req.environ, - start_response) - begin = time.time() - self._run(make_app_call, num_calls, current_rate, - check_time=bool(exp_time)) - self.assertEquals(round(time.time() - begin, 1), exp_time) - self._reset_time() + with mock.patch( + 'swift.common.middleware.ratelimit.get_account_info', + lambda *args, **kwargs: {}): + for meth, exp_time in [('DELETE', 9.8), ('GET', 0), + ('POST', 0), ('PUT', 9.8)]: + req = Request.blank('/v/a%s/c' % meth) + req.method = meth + req.environ['swift.cache'] = FakeMemcache() + make_app_call = lambda: self.test_ratelimit(req.environ, + start_response) + begin = time.time() + self._run(make_app_call, num_calls, current_rate, + check_time=bool(exp_time)) + self.assertEquals(round(time.time() - begin, 1), exp_time) + self._reset_time() def test_ratelimit_set_incr(self): current_rate = 5 @@ -403,7 +406,7 @@ class TestRateLimit(unittest.TestCase): req.method = 'PUT' req.environ['swift.cache'] = FakeMemcache() req.environ['swift.cache'].set( - ratelimit.get_container_memcache_key('a', 'c'), + get_container_memcache_key('a', 'c'), {'container_size': 1}) time_override = [0, 0, 0, 0, None] @@ -437,7 +440,7 @@ class TestRateLimit(unittest.TestCase): req.method = 'GET' req.environ['swift.cache'] = FakeMemcache() req.environ['swift.cache'].set( - ratelimit.get_container_memcache_key('a', 'c'), + get_container_memcache_key('a', 'c'), {'container_size': 1}) time_override = [0, 0, 0, 0, None] From e567722c4ea125f4607093112396e16ae6c6dd42 Mon Sep 17 00:00:00 2001 From: John Dickinson Date: Mon, 22 Sep 2014 09:46:34 -0700 Subject: [PATCH 05/19] updated hacking rules 1) Added comment for H231, which we were already enforcing. H231 is for Python 3.x compatible except statements. 2) Added check for H201, which we were enforcing in reviews but waiting on hacking checks to be updated. H201 is for bare except statements, and the update in upstream hacking is to support the " # noqa" flag on it. The H201 check catches some existing bare excepts that are fixed. Change-Id: I68638aa9ea925ef62f9035a426548c2c804911a8 --- .../middleware/x_profile/html_viewer.py | 96 +++++++++---------- swift/common/middleware/xprofile.py | 19 ++-- swift/container/reconciler.py | 2 +- test/unit/cli/test_recon.py | 2 +- tox.ini | 5 +- 5 files changed, 60 insertions(+), 64 deletions(-) diff --git a/swift/common/middleware/x_profile/html_viewer.py b/swift/common/middleware/x_profile/html_viewer.py index 416ba01165..91ad8f66a2 100644 --- a/swift/common/middleware/x_profile/html_viewer.py +++ b/swift/common/middleware/x_profile/html_viewer.py @@ -313,55 +313,55 @@ class HTMLViewer(object): return empty_description, headers try: stats = Stats2(*log_files) - if not fulldirs: - stats.strip_dirs() - stats.sort_stats(sort) - nfl_filter_esc =\ - nfl_filter.replace('(', '\(').replace(')', '\)') - amount = [nfl_filter_esc, limit] if nfl_filter_esc else [limit] - profile_html = self.generate_stats_html(stats, self.app_path, - profile_id, *amount) - description = "Profiling information is generated by using\ - '%s' profiler." % self.profile_module - sort_repl = '' % (p, p) - for p in self.profile_log.get_all_pids()]) - profile_element = string.Template(profile_tmpl).substitute( - {'profile_list': plist}) - profile_repl = '' % (p, p) + for p in self.profile_log.get_all_pids()]) + profile_element = string.Template(profile_tmpl).substitute( + {'profile_list': plist}) + profile_repl = '