From cc1373d9bc5a813a8532b89560960231e6609a3b Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Thu, 19 Aug 2010 11:54:55 -0500 Subject: [PATCH 1/6] make it easier for subclasses of swift.common.client.Connection to override the behavior of reauthorization (get_auth) --- swift/common/client.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/swift/common/client.py b/swift/common/client.py index 30c376f89e..16a5acd1e1 100644 --- a/swift/common/client.py +++ b/swift/common/client.py @@ -629,6 +629,9 @@ class Connection(object): self.attempts = 0 self.snet = snet + def get_auth(self): + return get_auth(self.authurl, self.user, self.key, snet=self.snet) + def _retry(self, func, *args, **kwargs): kwargs['http_conn'] = self.http_conn self.attempts = 0 @@ -637,9 +640,7 @@ class Connection(object): self.attempts += 1 try: if not self.url or not self.token: - self.url, self.token = \ - get_auth(self.authurl, self.user, self.key, - snet=self.snet) + self.url, self.token = self.get_auth() self.http_conn = None if not self.http_conn: self.http_conn = http_connection(self.url) From fba7dd3de33d20ec546e4a3a082f080471ba89f8 Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Thu, 19 Aug 2010 14:27:10 -0500 Subject: [PATCH 2/6] added some tests for swift.common.client, coverage from 11% => ~66% --- test/unit/common/test_client.py | 260 +++++++++++++++++++++++++++++++- 1 file changed, 255 insertions(+), 5 deletions(-) diff --git a/test/unit/common/test_client.py b/test/unit/common/test_client.py index 24c9ca1982..ef2f089263 100644 --- a/test/unit/common/test_client.py +++ b/test/unit/common/test_client.py @@ -13,16 +13,266 @@ # See the License for the specific language governing permissions and # limitations under the License. -# TODO: Tests +# TODO: More tests import unittest -from swift.common import client -class TestAuditor(unittest.TestCase): +from swift.common import client as c - def test_placeholder(self): - pass +class TestHttpHelpers(unittest.TestCase): + def test_quote(self): + value = 'standard string' + self.assertEquals('standard%20string', c.quote(value)) + value = u'\u0075nicode string' + self.assertEquals('unicode%20string', c.quote(value)) + + def test_http_connection(self): + url = 'http://www.test.com' + _, conn = c.http_connection(url) + self.assertTrue(isinstance(conn, c.HTTPConnection)) + url = 'https://www.test.com' + _, conn = c.http_connection(url) + self.assertTrue(isinstance(conn, c.HTTPSConnection)) + url = 'ftp://www.test.com' + self.assertRaises(c.ClientException, c.http_connection, url) + +class TestClientException(unittest.TestCase): + + def test_is_exception(self): + self.assertTrue(issubclass(c.ClientException, Exception)) + + def test_format(self): + exc = c.ClientException('something failed') + self.assertTrue('something failed' in str(exc)) + test_kwargs = ( + 'scheme', + 'host', + 'port', + 'path', + 'query', + 'status', + 'reason', + 'device', + ) + for value in test_kwargs: + kwargs = { + 'http_%s' % value: value + } + exc = c.ClientException('test', **kwargs) + self.assertTrue(value in str(exc)) + +class TestJsonImport(unittest.TestCase): + + def tearDown(self): + try: + import json + except ImportError: + pass + else: + reload(json) + + try: + import simplejson + except ImportError: + pass + else: + reload(simplejson) + + def test_any(self): + self.assertTrue(hasattr(c, 'json_loads')) + + def test_no_simplejson(self): + # break simplejson + try: + import simplejson + except ImportError: + # that was easy + pass + else: + delattr(simplejson, 'loads') + reload(c) + + try: + from json import loads + except ImportError: + # this case is stested in _no_json + pass + else: + self.assertEquals(loads, c.json_loads) + + def test_no_json(self): + # first break simplejson + try: + import simplejson + except ImportError: + # that was easy + pass + else: + delattr(simplejson, 'loads') + + # then break json + try: + import json + except ImportError: + # that was easy + _orig_dumps = None + else: + _orig_dumps = json.dumps + delattr(json, 'loads') + reload(c) + + if _orig_dumps: + # thank goodness + data = { + 'string': 'value', + 'int': 0, + 'bool': True, + 'none': None, + } + json_string = _orig_dumps(data) + else: + # wow, I guess we really need this thing... + data = ['value1', 'value2'] + json_string = "['value1', 'value2']" + + self.assertEquals(data, c.json_loads(json_string)) + self.assertRaises(AttributeError, c.json_loads, self) + +class MockHttpTest(unittest.TestCase): + + def setUp(self): + # Yoink! + from test.unit.proxy.test_server import fake_http_connect + # TODO: mock http connection class with more control over headers + def fake_http_connection(*args, **kwargs): + _orig_http_connection = c.http_connection + def wrapper(url): + parsed, _conn = _orig_http_connection(url) + conn = fake_http_connect(*args, **kwargs)() + def request(*args, **kwargs): + return + conn.request = request + return parsed, conn + return wrapper + self.fake_http_connection = fake_http_connection + + def tearDown(self): + reload(c) + +# TODO: following tests cases are placeholders, need more tests, better coverage + +class TestGetAuth(MockHttpTest): + + def test_ok(self): + c.http_connection = self.fake_http_connection(200) + url, token = c.get_auth('http://www.test.com', 'asdf', 'asdf') + self.assertEquals(url, None) + self.assertEquals(token, None) + +class TestGetAccount(MockHttpTest): + + def test_no_content(self): + c.http_connection = self.fake_http_connection(204) + value = c.get_account('http://www.test.com', 'asdf') + self.assertEquals(value, []) + +class TestHeadAccount(MockHttpTest): + + def test_server_error(self): + c.http_connection = self.fake_http_connection(500) + self.assertRaises(c.ClientException, c.head_account, + 'http://www.tests.com', 'asdf') + +class TestGetContainer(MockHttpTest): + + def test_no_content(self): + c.http_connection = self.fake_http_connection(204) + value = c.get_container('http://www.test.com', 'asdf', 'asdf') + self.assertEquals(value, []) + +class TestHeadContainer(MockHttpTest): + + def test_server_error(self): + c.http_connection = self.fake_http_connection(500) + self.assertRaises(c.ClientException, c.head_container, + 'http://www.test.com', 'asdf', 'asdf', + ) + +class TestPutContainer(MockHttpTest): + + def test_ok(self): + c.http_connection = self.fake_http_connection(200) + value = c.put_container('http://www.test.com', 'asdf', 'asdf') + self.assertEquals(value, None) + +class TestDeleteContainer(MockHttpTest): + + def test_ok(self): + c.http_connection = self.fake_http_connection(200) + value = c.delete_container('http://www.test.com', 'asdf', 'asdf') + self.assertEquals(value, None) + +class TestGetObject(MockHttpTest): + + def test_server_error(self): + c.http_connection = self.fake_http_connection(500) + self.assertRaises(c.ClientException, c.get_object, 'http://www.test.com', 'asdf', 'asdf', 'asdf') + +class TestHeadObject(MockHttpTest): + + def test_server_error(self): + c.http_connection = self.fake_http_connection(500) + self.assertRaises(c.ClientException, c.head_object, 'http://www.test.com', 'asdf', 'asdf', 'asdf') + +class TestPutObject(MockHttpTest): + + def test_ok(self): + c.http_connection = self.fake_http_connection(200) + value = c.put_object('http://www.test.com', 'asdf', 'asdf', 'asdf', 'asdf') + self.assertTrue(isinstance(value, basestring)) + + def test_server_error(self): + c.http_connection = self.fake_http_connection(500) + self.assertRaises(c.ClientException, c.put_object, + 'http://www.test.com', 'asdf', 'asdf', 'asdf', 'asdf') + +class TestPostObject(MockHttpTest): + + def test_ok(self): + c.http_connection = self.fake_http_connection(200) + value = c.post_object('http://www.test.com', 'asdf', 'asdf', 'asdf', {}) + + def test_server_error(self): + c.http_connection = self.fake_http_connection(500) + self.assertRaises(c.ClientException, c.post_object, + 'http://www.test.com', 'asdf', 'asdf', 'asdf', {}) + +class TestDeleteObject(MockHttpTest): + + def test_ok(self): + c.http_connection = self.fake_http_connection(200) + value = c.delete_object('http://www.test.com', 'asdf', 'asdf', 'asdf') + + def test_server_error(self): + c.http_connection = self.fake_http_connection(500) + self.assertRaises(c.ClientException, c.delete_object, + 'http://www.test.com', 'asdf', 'asdf', 'asdf') + +class TestConnection(MockHttpTest): + + def test_instance(self): + conn = c.Connection('http://www.test.com', 'asdf', 'asdf') + self.assertEquals(conn.retries, 5) + + def test_retry(self): + c.http_connection = self.fake_http_connection(500) + def quick_sleep(*args): + pass + c.sleep = quick_sleep + conn = c.Connection('http://www.test.com', 'asdf', 'asdf') + self.assertRaises(c.ClientException, conn.head_account) + self.assertEquals(conn.attempts, conn.retries + 1) if __name__ == '__main__': unittest.main() From 7455b4d2385f82ddae6ef179a123617f04c1ca74 Mon Sep 17 00:00:00 2001 From: Clay Gerrard Date: Thu, 19 Aug 2010 17:09:20 -0500 Subject: [PATCH 3/6] found a error condition in swift.common.client where the resp would not be consumed, added tests and fixed --- swift/common/client.py | 2 + test/unit/common/test_client.py | 69 +++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/swift/common/client.py b/swift/common/client.py index 16a5acd1e1..21e42342b7 100644 --- a/swift/common/client.py +++ b/swift/common/client.py @@ -164,6 +164,7 @@ def get_auth(url, user, key, snet=False): conn.request('GET', parsed.path, '', {'X-Auth-User': user, 'X-Auth-Key': key}) resp = conn.getresponse() + resp.read() if resp.status < 200 or resp.status >= 300: raise ClientException('Auth GET failed', http_scheme=parsed.scheme, http_host=conn.host, http_port=conn.port, @@ -246,6 +247,7 @@ def head_account(url, token, http_conn=None): parsed, conn = http_connection(url) conn.request('HEAD', parsed.path, '', {'X-Auth-Token': token}) resp = conn.getresponse() + resp.read() if resp.status < 200 or resp.status >= 300: raise ClientException('Account HEAD failed', http_scheme=parsed.scheme, http_host=conn.host, http_port=conn.port, diff --git a/test/unit/common/test_client.py b/test/unit/common/test_client.py index ef2f089263..44a46240de 100644 --- a/test/unit/common/test_client.py +++ b/test/unit/common/test_client.py @@ -153,6 +153,14 @@ class MockHttpTest(unittest.TestCase): def request(*args, **kwargs): return conn.request = request + + conn.has_been_read = False + _orig_read = conn.read + def read(*args, **kwargs): + conn.has_been_read = True + return _orig_read(*args, **kwargs) + conn.read = read + return parsed, conn return wrapper self.fake_http_connection = fake_http_connection @@ -179,6 +187,11 @@ class TestGetAccount(MockHttpTest): class TestHeadAccount(MockHttpTest): + def test_ok(self): + c.http_connection = self.fake_http_connection(200) + value = c.head_account('http://www.tests.com', 'asdf') + self.assertEquals(value, (0, 0, 0)) + def test_server_error(self): c.http_connection = self.fake_http_connection(500) self.assertRaises(c.ClientException, c.head_account, @@ -274,5 +287,61 @@ class TestConnection(MockHttpTest): self.assertRaises(c.ClientException, conn.head_account) self.assertEquals(conn.attempts, conn.retries + 1) + def test_resp_read_on_server_error(self): + c.http_connection = self.fake_http_connection(500) + conn = c.Connection('http://www.test.com', 'asdf', 'asdf', retries=0) + def get_auth(*args, **kwargs): + return 'http://www.new.com', 'new' + conn.get_auth = get_auth + self.url, self.token = conn.get_auth() + + method_signatures = ( + (conn.head_account, []), + (conn.get_account, []), + (conn.head_container, ('asdf',)), + (conn.get_container, ('asdf',)), + (conn.put_container, ('asdf',)), + (conn.delete_container, ('asdf',)), + (conn.head_object, ('asdf', 'asdf')), + (conn.get_object, ('asdf', 'asdf')), + (conn.put_object, ('asdf', 'asdf', 'asdf')), + (conn.post_object, ('asdf', 'asdf', {})), + (conn.delete_object, ('asdf', 'asdf')), + ) + + for method, args in method_signatures: + self.assertRaises(c.ClientException, method, *args) + try: + self.assertTrue(conn.http_conn[1].has_been_read) + except AssertionError: + self.fail('%s did not read the resp on server error' % method.__name__) + + def test_reauth(self): + c.http_connection = self.fake_http_connection(401) + def get_auth(*args, **kwargs): + return 'http://www.new.com', 'new' + def swap_sleep(*args): + self.swap_sleep_called = True + c.get_auth = get_auth + c.http_connection = self.fake_http_connection(200) + c.sleep = swap_sleep + self.swap_sleep_called = False + + conn = c.Connection('http://www.test.com', 'asdf', 'asdf', + preauthurl = 'http://www.old.com', + preauthtoken = 'old' + ) + + self.assertEquals(conn.attempts, 0) + self.assertEquals(conn.url, 'http://www.old.com') + self.assertEquals(conn.token, 'old') + + value = conn.head_account() + + self.assertTrue(self.swap_sleep_called) + self.assertEquals(conn.attempts, 2) + self.assertEquals(conn.url, 'http://www.new.com') + self.assertEquals(conn.token, 'new') + if __name__ == '__main__': unittest.main() From 4d66a8cbe73831d3e575c939f86148ce09fb0503 Mon Sep 17 00:00:00 2001 From: Caleb Tennis Date: Fri, 20 Aug 2010 21:29:12 +0000 Subject: [PATCH 4/6] We can still do a write_ring even if nothing is present. Better than crashing anyway. --- swift/common/ring/builder.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/swift/common/ring/builder.py b/swift/common/ring/builder.py index 7c0b08adea..46ce76e149 100644 --- a/swift/common/ring/builder.py +++ b/swift/common/ring/builder.py @@ -100,9 +100,12 @@ class RingBuilder(object): continue devs[dev['id']] = dict((k, v) for k, v in dev.items() if k not in ('parts', 'parts_wanted')) - self._ring = \ - RingData([array('H', p2d) for p2d in self._replica2part2dev], - devs, 32 - self.part_power) + if not self._replica2part2dev: + self._ring = RingData([], devs, 32 - self.part_power) + else: + self._ring = \ + RingData([array('H', p2d) for p2d in self._replica2part2dev], + devs, 32 - self.part_power) return self._ring def add_dev(self, dev): From db90da2763cd3a2a364989d5965cfa6d08e88224 Mon Sep 17 00:00:00 2001 From: Caleb Tennis Date: Sat, 21 Aug 2010 18:21:59 +0000 Subject: [PATCH 5/6] Remove the exception from the unit test, since we don't bomb out anymore. Also, add a warning to swift-ring-builder if you're building an empty ring, or do a write_ring and you aren't rebalanced --- bin/swift-ring-builder | 10 ++++++++-- test/unit/common/ring/test_builder.py | 1 - 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/bin/swift-ring-builder b/bin/swift-ring-builder index 877590da2e..50353df256 100755 --- a/bin/swift-ring-builder +++ b/bin/swift-ring-builder @@ -533,10 +533,16 @@ Exit codes: 0 = ring changed, 1 = ring did not change, 2 = error exit(EXIT_RING_UNCHANGED) elif argv[2] == 'write_ring': - pickle.dump(builder.get_ring(), + ring_data = builder.get_ring() + if not ring_data._replica2part2dev_id: + if ring_data.devs: + print 'Warning: Writing a ring with no partition assignments but with devices; did you forget to run "rebalance"?' + else: + print 'Warning: Writing an empty ring' + pickle.dump(ring_data, GzipFile(pathjoin(backup_dir, '%d.' % time() + basename(ring_file)), 'wb'), protocol=2) - pickle.dump(builder.get_ring(), GzipFile(ring_file, 'wb'), protocol=2) + pickle.dump(ring_data, GzipFile(ring_file, 'wb'), protocol=2) exit(EXIT_RING_CHANGED) elif argv[2] == 'pretend_min_part_hours_passed': diff --git a/test/unit/common/ring/test_builder.py b/test/unit/common/ring/test_builder.py index e4be88b6dc..2928558c8b 100644 --- a/test/unit/common/ring/test_builder.py +++ b/test/unit/common/ring/test_builder.py @@ -43,7 +43,6 @@ class TestRingBuilder(unittest.TestCase): def test_get_ring(self): rb = ring.RingBuilder(8, 3, 1) - self.assertRaises(Exception, rb.get_ring) rb.add_dev({'id': 0, 'zone': 0, 'weight': 1, 'ip': '127.0.0.1', 'port': 10000, 'device': 'sda1'}) rb.add_dev({'id': 1, 'zone': 1, 'weight': 1, 'ip': '127.0.0.1', From ded2514a945c3299c8ec1913d4bbe216f95674cb Mon Sep 17 00:00:00 2001 From: Michael Barton Date: Sun, 22 Aug 2010 22:17:57 +0000 Subject: [PATCH 6/6] remove object replicator hard-coded replica count --- swift/obj/replicator.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/swift/obj/replicator.py b/swift/obj/replicator.py index de3bea3972..43fd8a9eaa 100644 --- a/swift/obj/replicator.py +++ b/swift/obj/replicator.py @@ -34,8 +34,6 @@ from swift.common.utils import whataremyips, unlink_older_than, lock_path, \ from swift.common.bufferedhttp import http_connect -REPLICAS = 3 -MAX_HANDOFFS = 5 PICKLE_PROTOCOL = 2 ONE_WEEK = 604800 HASH_FILE = 'hashes.pkl' @@ -340,7 +338,8 @@ class ObjectReplicator(object): '/' + '-'.join(suffixes), headers={'Content-Length': '0'}).getresponse().read() responses.append(success) - if not suffixes or (len(responses) == REPLICAS and all(responses)): + if not suffixes or (len(responses) == \ + self.object_ring.replica_count and all(responses)): self.logger.info("Removing partition: %s" % job['path']) tpool.execute(shutil.rmtree, job['path'], ignore_errors=True) except (Exception, Timeout): @@ -364,7 +363,7 @@ class ObjectReplicator(object): successes = 0 nodes = itertools.chain(job['nodes'], self.object_ring.get_more_nodes(int(job['partition']))) - while successes < (REPLICAS - 1): + while successes < (self.object_ring.replica_count - 1): node = next(nodes) try: with Timeout(60):