Merge "Consistently apply node error limiting rules in proxy"

swift/proxy/controllers/base.py

@@ -1034,6 +1034,14 @@ class Controller(object):
                     elif resp.status == HTTP_INSUFFICIENT_STORAGE:
                         self.app.error_limit(node,
                                              _('ERROR Insufficient Storage'))
+                    elif is_server_error(resp.status):
+                        self.app.error_occurred(
+                            node, _('ERROR %(status)d '
+                                    'Trying to %(method)s %(path)s'
+                                    'From Container Server') % {
+                                        'status': resp.status,
+                                        'method': method,
+                                        'path': path})
             except (Exception, Timeout):
                 self.app.exception_occurred(
                     node, self.server_type,

swift/proxy/controllers/obj.py

@@ -47,10 +47,11 @@ from swift.common import constraints
 from swift.common.exceptions import ChunkReadTimeout, \
     ChunkWriteTimeout, ConnectionTimeout, ListingIterNotFound, \
     ListingIterNotAuthorized, ListingIterError
-from swift.common.http import is_success, is_client_error, HTTP_CONTINUE, \
+from swift.common.http import (
-    HTTP_CREATED, HTTP_MULTIPLE_CHOICES, HTTP_NOT_FOUND, \
+    is_success, is_client_error, is_server_error, HTTP_CONTINUE, HTTP_CREATED,
-    HTTP_INTERNAL_SERVER_ERROR, HTTP_SERVICE_UNAVAILABLE, \
+    HTTP_MULTIPLE_CHOICES, HTTP_NOT_FOUND, HTTP_INTERNAL_SERVER_ERROR,
-    HTTP_INSUFFICIENT_STORAGE, HTTP_PRECONDITION_FAILED
+    HTTP_SERVICE_UNAVAILABLE, HTTP_INSUFFICIENT_STORAGE,
+    HTTP_PRECONDITION_FAILED)
 from swift.proxy.controllers.base import Controller, delay_denial, \
     cors_validation
 from swift.common.swob import HTTPAccepted, HTTPBadRequest, HTTPNotFound, \
@@ -350,6 +351,11 @@ class ObjectController(Controller):
                     return conn
                 elif resp.status == HTTP_INSUFFICIENT_STORAGE:
                     self.app.error_limit(node, _('ERROR Insufficient Storage'))
+                elif is_server_error(resp.status):
+                    self.app.error_occurred(
+                        node, _('ERROR %(status)d Expect: 100-continue '
+                                'From Object Server') % {
+                                    'status': resp.status})
             except (Exception, Timeout):
                 self.app.exception_occurred(
                     node, _('Object'),
@@ -382,7 +388,10 @@ class ObjectController(Controller):
             statuses.append(response.status)
             reasons.append(response.reason)
             bodies.append(response.read())
-            if response.status >= HTTP_INTERNAL_SERVER_ERROR:
+            if response.status == HTTP_INSUFFICIENT_STORAGE:
+                self.app.error_limit(conn.node,
+                                     _('ERROR Insufficient Storage'))
+            elif response.status >= HTTP_INTERNAL_SERVER_ERROR:
                 self.app.error_occurred(
                     conn.node,
                     _('ERROR %(status)d %(body)s From Object Server '

swift/proxy/server.py

@@ -452,6 +452,12 @@ class Application(object):
                           {'msg': msg, 'ip': node['ip'],
                           'port': node['port'], 'device': node['device']})
 
+    def _incr_node_errors(self, node):
+        node_key = self._error_limit_node_key(node)
+        error_stats = self._error_limiting.setdefault(node_key, {})
+        error_stats['errors'] = error_stats.get('errors', 0) + 1
+        error_stats['last_error'] = time()
+
     def error_occurred(self, node, msg):
         """
         Handle logging, and handling of errors.
@@ -459,10 +465,7 @@ class Application(object):
         :param node: dictionary of node to handle errors for
         :param msg: error message
         """
-        node_key = self._error_limit_node_key(node)
+        self._incr_node_errors(node)
-        error_stats = self._error_limiting.setdefault(node_key, {})
-        error_stats['errors'] = error_stats.get('errors', 0) + 1
-        error_stats['last_error'] = time()
         self.logger.error(_('%(msg)s %(ip)s:%(port)s/%(device)s'),
                           {'msg': msg, 'ip': node['ip'],
                           'port': node['port'], 'device': node['device']})
@@ -531,6 +534,7 @@ class Application(object):
         :param typ: server type
         :param additional_info: additional information to log
         """
+        self._incr_node_errors(node)
         self.logger.exception(
             _('ERROR with %(type)s server %(ip)s:%(port)s/%(device)s re: '
               '%(info)s'),

test/unit/__init__.py

@@ -776,7 +776,25 @@ def fake_http_connect(*code_iter, **kwargs):
 
 @contextmanager
 def mocked_http_conn(*args, **kwargs):
+    requests = []
+
+    def capture_requests(ip, port, method, path, headers, qs, ssl):
+        req = {
+            'ip': ip,
+            'port': port,
+            'method': method,
+            'path': path,
+            'headers': headers,
+            'qs': qs,
+            'ssl': ssl,
+        }
+        requests.append(req)
+    kwargs.setdefault('give_connect', capture_requests)
     fake_conn = fake_http_connect(*args, **kwargs)
+    fake_conn.requests = requests
     with mocklib.patch('swift.common.bufferedhttp.http_connect_raw',
                        new=fake_conn):
         yield fake_conn
+        left_over_status = list(fake_conn.code_iter)
+        if left_over_status:
+            raise AssertionError('left over status %r' % left_over_status)

test/unit/proxy/controllers/test_container.py

@@ -16,6 +16,8 @@
 import mock
 import unittest
 
+from eventlet import Timeout
+
 from swift.common.swob import Request
 from swift.proxy import server as proxy_server
 from swift.proxy.controllers.base import headers_to_container_info
@@ -23,17 +25,48 @@ from test.unit import fake_http_connect, FakeRing, FakeMemcache
 from swift.common.storage_policy import StoragePolicy
 from swift.common.request_helpers import get_sys_meta_prefix
 
-from test.unit import patch_policies
+from test.unit import patch_policies, mocked_http_conn, debug_logger
 from test.unit.common.ring.test_ring import TestRingBase
+from test.unit.proxy.test_server import node_error_count
 
 
 @patch_policies([StoragePolicy(0, 'zero', True, object_ring=FakeRing())])
 class TestContainerController(TestRingBase):
     def setUp(self):
         TestRingBase.setUp(self)
+        self.logger = debug_logger()
+        self.container_ring = FakeRing(max_more_nodes=9)
         self.app = proxy_server.Application(None, FakeMemcache(),
+                                            logger=self.logger,
                                             account_ring=FakeRing(),
-                                            container_ring=FakeRing())
+                                            container_ring=self.container_ring)
+
+        self.account_info = {
+            'status': 200,
+            'container_count': '10',
+            'total_object_count': '100',
+            'bytes': '1000',
+            'meta': {},
+            'sysmeta': {},
+        }
+
+        class FakeAccountInfoContainerController(
+                proxy_server.ContainerController):
+
+            def account_info(controller, *args, **kwargs):
+                patch_path = 'swift.proxy.controllers.base.get_info'
+                with mock.patch(patch_path) as mock_get_info:
+                    mock_get_info.return_value = dict(self.account_info)
+                    return super(FakeAccountInfoContainerController,
+                                 controller).account_info(
+                                     *args, **kwargs)
+        _orig_get_controller = self.app.get_controller
+
+        def wrapped_get_controller(*args, **kwargs):
+            with mock.patch('swift.proxy.server.ContainerController',
+                            new=FakeAccountInfoContainerController):
+                return _orig_get_controller(*args, **kwargs)
+        self.app.get_controller = wrapped_get_controller
 
     def test_container_info_in_response_env(self):
         controller = proxy_server.ContainerController(self.app, 'a', 'c')
@@ -123,6 +156,53 @@ class TestContainerController(TestRingBase):
         self.assertEqual(context['headers'][user_meta_key], 'bar')
         self.assertNotEqual(context['headers']['x-timestamp'], '1.0')
 
+    def test_node_errors(self):
+        self.app.sort_nodes = lambda n: n
+
+        for method in ('PUT', 'DELETE', 'POST'):
+            def test_status_map(statuses, expected):
+                self.app._error_limiting = {}
+                req = Request.blank('/v1/a/c', method=method)
+                with mocked_http_conn(*statuses) as fake_conn:
+                    print 'a' * 50
+                    resp = req.get_response(self.app)
+                self.assertEqual(resp.status_int, expected)
+                for req in fake_conn.requests:
+                    self.assertEqual(req['method'], method)
+                    self.assert_(req['path'].endswith('/a/c'))
+
+            base_status = [201] * 3
+            # test happy path
+            test_status_map(list(base_status), 201)
+            for i in range(3):
+                self.assertEqual(node_error_count(
+                    self.app, self.container_ring.devs[i]), 0)
+            # single node errors and test isolation
+            for i in range(3):
+                status_list = list(base_status)
+                status_list[i] = 503
+                status_list.append(201)
+                test_status_map(status_list, 201)
+                for j in range(3):
+                    expected = 1 if j == i else 0
+                    self.assertEqual(node_error_count(
+                        self.app, self.container_ring.devs[j]), expected)
+            # timeout
+            test_status_map((201, Timeout(), 201, 201), 201)
+            self.assertEqual(node_error_count(
+                self.app, self.container_ring.devs[1]), 1)
+
+            # exception
+            test_status_map((Exception('kaboom!'), 201, 201, 201), 201)
+            self.assertEqual(node_error_count(
+                self.app, self.container_ring.devs[0]), 1)
+
+            # insufficient storage
+            test_status_map((201, 201, 507, 201), 201)
+            self.assertEqual(node_error_count(
+                self.app, self.container_ring.devs[2]),
+                self.app.error_suppression_limit + 1)
+
 
 if __name__ == '__main__':
     unittest.main()

test/unit/proxy/controllers/test_obj.py

@@ -21,6 +21,7 @@ import unittest
 from contextlib import contextmanager
 
 import mock
+from eventlet import Timeout
 
 import swift
 from swift.common import utils, swob
@@ -29,6 +30,7 @@ from swift.common.storage_policy import StoragePolicy, POLICIES
 
 from test.unit import FakeRing, FakeMemcache, fake_http_connect, \
     debug_logger, patch_policies
+from test.unit.proxy.test_server import node_error_count
 
 
 @contextmanager
@@ -52,6 +54,11 @@ def set_http_connect(*args, **kwargs):
 
 
 class PatchedObjControllerApp(proxy_server.Application):
+    """
+    This patch is just a hook over handle_request to ensure that when
+    get_controller is called the ObjectController class is patched to
+    return a (possibly stubbed) ObjectController class.
+    """
 
     object_controller = proxy_server.ObjectController
 
@@ -155,6 +162,10 @@ class TestObjController(unittest.TestCase):
                     return super(FakeContainerInfoObjController,
                                  controller).container_info(*args, **kwargs)
 
+        # this is taking advantage of the fact that self.app is a
+        # PachedObjControllerApp, so handle_response will route into an
+        # instance of our FakeContainerInfoObjController just by
+        # overriding the class attribute for object_controller
         self.app.object_controller = FakeContainerInfoObjController
 
     def test_PUT_simple(self):
@@ -188,6 +199,59 @@ class TestObjController(unittest.TestCase):
             resp = req.get_response(self.app)
         self.assertEquals(resp.status_int, 400)
 
+    def test_PUT_connect_exceptions(self):
+        object_ring = self.app.get_object_ring(None)
+        self.app.sort_nodes = lambda n: n  # disable shuffle
+
+        def test_status_map(statuses, expected):
+            self.app._error_limiting = {}
+            req = swob.Request.blank('/v1/a/c/o.jpg', method='PUT',
+                                     body='test body')
+            with set_http_connect(*statuses):
+                resp = req.get_response(self.app)
+            self.assertEqual(resp.status_int, expected)
+
+        base_status = [201] * 3
+        # test happy path
+        test_status_map(list(base_status), 201)
+        for i in range(3):
+            self.assertEqual(node_error_count(
+                self.app, object_ring.devs[i]), 0)
+        # single node errors and test isolation
+        for i in range(3):
+            status_list = list(base_status)
+            status_list[i] = 503
+            test_status_map(status_list, 201)
+            for j in range(3):
+                self.assertEqual(node_error_count(
+                    self.app, object_ring.devs[j]), 1 if j == i else 0)
+        # connect errors
+        test_status_map((201, Timeout(), 201, 201), 201)
+        self.assertEqual(node_error_count(
+            self.app, object_ring.devs[1]), 1)
+        test_status_map((Exception('kaboom!'), 201, 201, 201), 201)
+        self.assertEqual(node_error_count(
+            self.app, object_ring.devs[0]), 1)
+        # expect errors
+        test_status_map((201, 201, (503, None), 201), 201)
+        self.assertEqual(node_error_count(
+            self.app, object_ring.devs[2]), 1)
+        test_status_map(((507, None), 201, 201, 201), 201)
+        self.assertEqual(
+            node_error_count(self.app, object_ring.devs[0]),
+            self.app.error_suppression_limit + 1)
+        # response errors
+        test_status_map(((100, Timeout()), 201, 201), 201)
+        self.assertEqual(
+            node_error_count(self.app, object_ring.devs[0]), 1)
+        test_status_map((201, 201, (100, Exception())), 201)
+        self.assertEqual(
+            node_error_count(self.app, object_ring.devs[2]), 1)
+        test_status_map((201, (100, 507), 201), 201)
+        self.assertEqual(
+            node_error_count(self.app, object_ring.devs[1]),
+            self.app.error_suppression_limit + 1)
+
     def test_GET_simple(self):
         req = swift.common.swob.Request.blank('/v1/a/c/o')
         with set_http_connect(200):