From 8dd99170bfae0cb52ce349c7dab3eecc3ff590e3 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Sun, 24 May 2015 15:40:36 +0200 Subject: [PATCH] Refactor exceptions handling in etcd.py and ha.py update_leader can throw EtcdError exception unused HealthiestMemberError exception is removed --- helpers/errors.py | 4 ---- helpers/etcd.py | 52 ++++++++++++++++++++++++++++++---------------- helpers/ha.py | 4 +--- tests/test_etcd.py | 44 ++++++++++++++++++++++++++++++++++----- tests/test_ha.py | 1 + 5 files changed, 75 insertions(+), 30 deletions(-) diff --git a/helpers/errors.py b/helpers/errors.py index dd1bc698..b86bd0f0 100644 --- a/helpers/errors.py +++ b/helpers/errors.py @@ -9,7 +9,3 @@ class EtcdError(Exception): class CurrentLeaderError(EtcdError): pass - - -class HealthiestMemberError(EtcdError): - pass diff --git a/helpers/etcd.py b/helpers/etcd.py index e808f8d4..966f881b 100644 --- a/helpers/etcd.py +++ b/helpers/etcd.py @@ -2,12 +2,12 @@ import logging import requests import time +from requests.exceptions import RequestException from collections import namedtuple from helpers.errors import CurrentLeaderError, EtcdError logger = logging.getLogger(__name__) - Member = namedtuple('Member', 'hostname,address,ttl') @@ -35,7 +35,7 @@ class Etcd: response = requests.get(self.client_url(path)) if response.status_code == 200: break - except Exception as e: + except RequestException as e: logger.exception('get_client_path') ex = e @@ -54,15 +54,15 @@ class Etcd: try: response = requests.put(self.client_url(path), data=data) return response.status_code in [200, 201, 202, 204] - except: + except RequestException: logger.exception('PUT %s data=%s', path, data) - return False + raise EtcdError('Etcd is not responding properly') def delete_client_path(self, path): try: response = requests.delete(self.client_url(path)) return response.status_code in [200, 202, 204] - except: + except RequestException: logger.exception('DELETE %s', path) return False @@ -108,7 +108,7 @@ class Etcd: leader = m break if not leader: - leader = Member(leader['value'], None, None) + leader = Member(node['value'], None, None) return Cluster(leader, last_leader_operation, members) elif status_code == 404: @@ -122,27 +122,43 @@ class Etcd: try: cluster = self.get_cluster() return None if cluster.is_unlocked() else cluster.leader - except: - raise CurrentLeaderError("Etcd is not responding properly") + except EtcdError: + raise CurrentLeaderError('Etcd is not responding properly') - def touch_member(self, member, connection_string): - return self.put_client_path('/members/' + member, value=connection_string, ttl=self.member_ttl) + def touch_member(self, member, connection_string, ttl=None): + try: + return self.put_client_path('/members/' + member, value=connection_string, ttl=ttl or self.member_ttl) + except EtcdError: + return False def take_leader(self, value): - return self.put_client_path('/leader', value=value, ttl=self.ttl) + try: + return self.put_client_path('/leader', value=value, ttl=self.ttl) + except EtcdError: + return False def attempt_to_acquire_leader(self, value): - ret = self.put_client_path('/leader', value=value, ttl=self.ttl, prevExist=False) - ret or logger.info('Could not take out TTL lock') - return ret + try: + ret = self.put_client_path('/leader', value=value, ttl=self.ttl, prevExist=False) + ret or logger.info('Could not take out TTL lock') + return ret + except EtcdError: + return False def update_leader(self, state_handler): - ret = self.put_client_path('/leader', value=state_handler.name, ttl=self.ttl, prevValue=state_handler.name) - ret and self.put_client_path('/optime/leader', value=state_handler.last_operation()) - return ret + if self.put_client_path('/leader', value=state_handler.name, ttl=self.ttl, prevValue=state_handler.name): + try: + self.put_client_path('/optime/leader', value=state_handler.last_operation()) + except EtcdError: + pass + return True + return False def race(self, path, value): - return self.put_client_path(path, value=value, prevExist=False) + try: + return self.put_client_path(path, value=value, prevExist=False) + except EtcdError: + return False def delete_member(self, member): return self.delete_client_path('/members/' + member) diff --git a/helpers/ha.py b/helpers/ha.py index 773c9b57..29979363 100644 --- a/helpers/ha.py +++ b/helpers/ha.py @@ -1,6 +1,6 @@ import logging -from helpers.errors import EtcdError, HealthiestMemberError +from helpers.errors import EtcdError from psycopg2 import InterfaceError, OperationalError logger = logging.getLogger(__name__) @@ -94,5 +94,3 @@ class Ha: return 'demoted self because etcd is not accessible and i was a leader' except (InterfaceError, OperationalError): logger.error('Error communicating with Postgresql. Will try again') - except HealthiestMemberError: - logger.error('failed to determine healthiest member fromt etcd') diff --git a/tests/test_etcd.py b/tests/test_etcd.py index c358e89e..8b3d60b2 100644 --- a/tests/test_etcd.py +++ b/tests/test_etcd.py @@ -17,28 +17,39 @@ class MockResponse: return json.loads(self.content) +class MockPostgresql: + name = '' + + def last_operation(self): + return 0 + + def requests_get(url, **kwargs): if url.startswith('http://local'): - raise Exception() + raise requests.exceptions.RequestException() response = MockResponse() if url.startswith('http://remote'): response.content = '{"action":"get","node":{"key":"/service/batman5","dir":true,"nodes":[{"key":"/service/batman5/initialize","value":"postgresql0","modifiedIndex":1582,"createdIndex":1582},{"key":"/service/batman5/leader","value":"postgresql1","expiration":"2015-05-15T09:11:00.037397538Z","ttl":21,"modifiedIndex":20728,"createdIndex":20434},{"key":"/service/batman5/optime","dir":true,"nodes":[{"key":"/service/batman5/optime/leader","value":"2164261704","modifiedIndex":20729,"createdIndex":20729}],"modifiedIndex":20437,"createdIndex":20437},{"key":"/service/batman5/members","dir":true,"nodes":[{"key":"/service/batman5/members/postgresql1","value":"postgres://replicator:rep-pass@127.0.0.1:5434/postgres","expiration":"2015-05-15T09:10:59.949384522Z","ttl":21,"modifiedIndex":20727,"createdIndex":20727},{"key":"/service/batman5/members/postgresql0","value":"postgres://replicator:rep-pass@127.0.0.1:5433/postgres","expiration":"2015-05-15T09:11:09.611860899Z","ttl":30,"modifiedIndex":20730,"createdIndex":20730}],"modifiedIndex":1581,"createdIndex":1581}],"modifiedIndex":1581,"createdIndex":1581}}' elif url.startswith('http://other'): response.status_code = 404 + elif url.startswith('http://noleader'): + response.content = '{"action":"get","node":{"key":"/service/batman5","dir":true,"nodes":[{"key":"/service/batman5/initialize","value":"postgresql0","modifiedIndex":1582,"createdIndex":1582},{"key":"/service/batman5/leader","value":"postgresql1","expiration":"2015-05-15T09:11:00.037397538Z","ttl":21,"modifiedIndex":20728,"createdIndex":20434},{"key":"/service/batman5/optime","dir":true,"nodes":[{"key":"/service/batman5/optime/leader","value":"2164261704","modifiedIndex":20729,"createdIndex":20729}],"modifiedIndex":20437,"createdIndex":20437},{"key":"/service/batman5/members","dir":true,"nodes":[{"key":"/service/batman5/members/postgresql0","value":"postgres://replicator:rep-pass@127.0.0.1:5433/postgres","expiration":"2015-05-15T09:11:09.611860899Z","ttl":30,"modifiedIndex":20730,"createdIndex":20730}],"modifiedIndex":1581,"createdIndex":1581}],"modifiedIndex":1581,"createdIndex":1581}}' return response def requests_put(url, **kwargs): - if url.startswith('http://local'): - raise Exception() + if url.startswith('http://local') or '/optime/leader' in url: + raise requests.exceptions.RequestException() response = MockResponse() response.status_code = 201 + if url.startswith('http://other'): + response.status_code = 404 return response def requests_delete(url): if url.startswith('http://local'): - raise Exception() + raise requests.exceptions.RequestException() response = MockResponse() response.status_code = 204 return response @@ -65,7 +76,7 @@ class TestEtcd(unittest.TestCase): self.assertRaises(Exception, self.etcd.get_client_path, '', 2) def test_put_client_path(self): - self.assertFalse(self.etcd.put_client_path('')) + self.assertRaises(EtcdError, self.etcd.put_client_path, '') def test_delete_client_path(self): self.assertFalse(self.etcd.delete_client_path('')) @@ -77,6 +88,29 @@ class TestEtcd(unittest.TestCase): self.assertIsInstance(cluster, Cluster) self.etcd.base_client_url = self.etcd.base_client_url.replace('remote', 'other') self.etcd.get_cluster() + self.etcd.base_client_url = self.etcd.base_client_url.replace('other', 'noleader') + self.etcd.get_cluster() def test_current_leader(self): self.assertRaises(CurrentLeaderError, self.etcd.current_leader) + + def test_touch_member(self): + self.assertFalse(self.etcd.touch_member('', '')) + + def test_take_leader(self): + self.assertFalse(self.etcd.take_leader('')) + + def test_attempt_to_acquire_leader(self): + self.assertFalse(self.etcd.attempt_to_acquire_leader('')) + + def test_update_leader(self): + self.etcd.base_client_url = self.etcd.base_client_url.replace('local', 'remote') + self.assertTrue(self.etcd.update_leader(MockPostgresql())) + self.etcd.base_client_url = self.etcd.base_client_url.replace('remote', 'other') + self.assertFalse(self.etcd.update_leader(MockPostgresql())) + + def test_race(self): + self.assertFalse(self.etcd.race('', '')) + + def test_delete_member(self): + self.assertFalse(self.etcd.delete_member('')) diff --git a/tests/test_ha.py b/tests/test_ha.py index cabdf7d5..c23244da 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -72,6 +72,7 @@ class TestHa(unittest.TestCase): self.p = MockPostgresql() self.e = Etcd({'ttl': 30, 'host': 'remotehost', 'scope': 'test'}) self.ha = Ha(self.p, self.e) + self.ha.load_cluster_from_etcd() self.ha.cluster = Cluster(None, None, []) self.ha.load_cluster_from_etcd = nop