From 0fb88a959a156ef2a4be6cf6ef00b70ccabfe3b6 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 28 Jun 2017 03:26:23 -0700 Subject: [PATCH 1/5] Add retry to ns1 provider --- octodns/provider/ns1.py | 29 +++++++++++++++++++++++++---- requirements.txt | 2 +- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 2f0a024..3b8ad00 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -7,7 +7,8 @@ from __future__ import absolute_import, division, print_function, \ from logging import getLogger from nsone import NSONE -from nsone.rest.errors import ResourceException +from nsone.rest.errors import RateLimitException, ResourceException +from time import sleep from ..record import Record from .base import BaseProvider @@ -25,6 +26,7 @@ class Ns1Provider(BaseProvider): SUPPORTS = set(('A', 'AAAA', 'ALIAS', 'CNAME', 'MX', 'NAPTR', 'NS', 'PTR', 'SPF', 'SRV', 'TXT')) + RATE_LIMIT_DELAY = 1 ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' def __init__(self, id, api_key, *args, **kwargs): @@ -171,7 +173,14 @@ class Ns1Provider(BaseProvider): name = self._get_name(new) _type = new._type params = getattr(self, '_params_for_{}'.format(_type))(new) - getattr(nsone_zone, 'add_{}'.format(_type))(name, **params) + meth = getattr(nsone_zone, 'add_{}'.format(_type)) + try: + meth(name, **params) + except RateLimitException: + self.log.warn('_apply_Create: rate limit encountered, pausing ' + 'and trying again') + sleep(self.RATE_LIMIT_DELAY) + meth(name, **params) def _apply_Update(self, nsone_zone, change): existing = change.existing @@ -180,14 +189,26 @@ class Ns1Provider(BaseProvider): record = nsone_zone.loadRecord(name, _type) new = change.new params = getattr(self, '_params_for_{}'.format(_type))(new) - record.update(**params) + try: + record.update(**params) + except RateLimitException: + self.log.warn('_apply_Update: rate limit encountered, pausing ' + 'and trying again') + sleep(self.RATE_LIMIT_DELAY) + record.update(**params) def _apply_Delete(self, nsone_zone, change): existing = change.existing name = self._get_name(existing) _type = existing._type record = nsone_zone.loadRecord(name, _type) - record.delete() + try: + record.delete() + except RateLimitException: + self.log.warn('_apply_Delete: rate limit encountered, pausing ' + 'and trying again') + sleep(self.RATE_LIMIT_DELAY) + record.delete() def _apply(self, plan): desired = plan.desired diff --git a/requirements.txt b/requirements.txt index b10ca4c..93a8521 100644 --- a/requirements.txt +++ b/requirements.txt @@ -11,7 +11,7 @@ incf.countryutils==1.0 ipaddress==1.0.18 jmespath==0.9.0 natsort==5.0.3 -nsone==0.9.10 +nsone==0.9.14 python-dateutil==2.6.0 requests==2.13.0 s3transfer==0.1.10 From a44b82c2c79e6025c605f77fd797c2254c2939dd Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 28 Jun 2017 04:11:46 -0700 Subject: [PATCH 2/5] NS1 rate_limit_delay param, unit tests for rate limit handling --- octodns/provider/ns1.py | 13 ++++++----- tests/test_octodns_provider_ns1.py | 35 +++++++++++++++++++++++++----- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 3b8ad00..0f3db1f 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -26,14 +26,15 @@ class Ns1Provider(BaseProvider): SUPPORTS = set(('A', 'AAAA', 'ALIAS', 'CNAME', 'MX', 'NAPTR', 'NS', 'PTR', 'SPF', 'SRV', 'TXT')) - RATE_LIMIT_DELAY = 1 ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' - def __init__(self, id, api_key, *args, **kwargs): + def __init__(self, id, api_key, rate_limit_delay=1, *args, **kwargs): self.log = getLogger('Ns1Provider[{}]'.format(id)) - self.log.debug('__init__: id=%s, api_key=***', id) + self.log.debug('__init__: id=%s, api_key=***, rate_limit_delay=%d', id, + rate_limit_delay) super(Ns1Provider, self).__init__(id, *args, **kwargs) self._client = NSONE(apiKey=api_key) + self.rate_limit_delay = rate_limit_delay def _data_for_A(self, _type, record): return { @@ -179,7 +180,7 @@ class Ns1Provider(BaseProvider): except RateLimitException: self.log.warn('_apply_Create: rate limit encountered, pausing ' 'and trying again') - sleep(self.RATE_LIMIT_DELAY) + sleep(self.rate_limit_delay) meth(name, **params) def _apply_Update(self, nsone_zone, change): @@ -194,7 +195,7 @@ class Ns1Provider(BaseProvider): except RateLimitException: self.log.warn('_apply_Update: rate limit encountered, pausing ' 'and trying again') - sleep(self.RATE_LIMIT_DELAY) + sleep(self.rate_limit_delay) record.update(**params) def _apply_Delete(self, nsone_zone, change): @@ -207,7 +208,7 @@ class Ns1Provider(BaseProvider): except RateLimitException: self.log.warn('_apply_Delete: rate limit encountered, pausing ' 'and trying again') - sleep(self.RATE_LIMIT_DELAY) + sleep(self.rate_limit_delay) record.delete() def _apply(self, plan): diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index ecc107c..5e53cfd 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -6,7 +6,8 @@ from __future__ import absolute_import, division, print_function, \ unicode_literals from mock import Mock, call, patch -from nsone.rest.errors import AuthException, ResourceException +from nsone.rest.errors import AuthException, RateLimitException, \ + ResourceException from unittest import TestCase from octodns.record import Delete, Record, Update @@ -192,7 +193,7 @@ class TestNs1Provider(TestCase): @patch('nsone.NSONE.createZone') @patch('nsone.NSONE.loadZone') def test_sync(self, load_mock, create_mock): - provider = Ns1Provider('test', 'api-key') + provider = Ns1Provider('test', 'api-key', rate_limit_delay=0) desired = Zone('unit.tests.', []) desired.records.update(self.expected) @@ -225,7 +226,15 @@ class TestNs1Provider(TestCase): create_mock.reset_mock() load_mock.side_effect = \ ResourceException('server error: zone not found') - create_mock.side_effect = None + # ugh, need a mock zone with a mock prop since we're using getattr, we + # can actually control side effects on `meth` with that. + mock_zone = Mock() + mock_zone.add_SRV = Mock() + mock_zone.add_SRV.side_effect = [ + RateLimitException('boo'), + None, + ] + create_mock.side_effect = [mock_zone] got_n = provider.apply(plan) self.assertEquals(expected_n, got_n) @@ -245,12 +254,26 @@ class TestNs1Provider(TestCase): self.assertEquals(2, len(plan.changes)) self.assertIsInstance(plan.changes[0], Update) self.assertIsInstance(plan.changes[1], Delete) - + # ugh, we need a mock record that can be returned from loadRecord for + # the update and delete targets, we can add our side effects to that to + # trigger rate limit handling + mock_record = Mock() + mock_record.update.side_effect = [ + RateLimitException('one'), + None, + ] + mock_record.delete.side_effect = [ + RateLimitException('two'), + None, + ] + nsone_zone.loadRecord.side_effect = [mock_record, mock_record] got_n = provider.apply(plan) self.assertEquals(2, got_n) nsone_zone.loadRecord.assert_has_calls([ call('unit.tests', u'A'), - call().update(answers=[u'1.2.3.4'], ttl=32), call('delete-me', u'A'), - call().delete() + ]) + mock_record.assert_has_calls([ + call.update(answers=[u'1.2.3.4'], ttl=32), + call.delete() ]) From d9806e851f2ef7a2a8717cda1fa49cd8687e67e7 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sun, 2 Jul 2017 10:45:58 -0700 Subject: [PATCH 3/5] NS1 RateLimitException, just sleep for e.period --- octodns/provider/ns1.py | 18 ++++++++---------- tests/test_octodns_provider_ns1.py | 8 ++++---- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 0f3db1f..bca6118 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -28,13 +28,11 @@ class Ns1Provider(BaseProvider): ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' - def __init__(self, id, api_key, rate_limit_delay=1, *args, **kwargs): + def __init__(self, id, api_key, *args, **kwargs): self.log = getLogger('Ns1Provider[{}]'.format(id)) - self.log.debug('__init__: id=%s, api_key=***, rate_limit_delay=%d', id, - rate_limit_delay) + self.log.debug('__init__: id=%s, api_key=***', id) super(Ns1Provider, self).__init__(id, *args, **kwargs) self._client = NSONE(apiKey=api_key) - self.rate_limit_delay = rate_limit_delay def _data_for_A(self, _type, record): return { @@ -177,10 +175,10 @@ class Ns1Provider(BaseProvider): meth = getattr(nsone_zone, 'add_{}'.format(_type)) try: meth(name, **params) - except RateLimitException: + except RateLimitException as e: self.log.warn('_apply_Create: rate limit encountered, pausing ' 'and trying again') - sleep(self.rate_limit_delay) + sleep(e.period) meth(name, **params) def _apply_Update(self, nsone_zone, change): @@ -192,10 +190,10 @@ class Ns1Provider(BaseProvider): params = getattr(self, '_params_for_{}'.format(_type))(new) try: record.update(**params) - except RateLimitException: + except RateLimitException as e: self.log.warn('_apply_Update: rate limit encountered, pausing ' 'and trying again') - sleep(self.rate_limit_delay) + sleep(e.period) record.update(**params) def _apply_Delete(self, nsone_zone, change): @@ -205,10 +203,10 @@ class Ns1Provider(BaseProvider): record = nsone_zone.loadRecord(name, _type) try: record.delete() - except RateLimitException: + except RateLimitException as e: self.log.warn('_apply_Delete: rate limit encountered, pausing ' 'and trying again') - sleep(self.rate_limit_delay) + sleep(e.period) record.delete() def _apply(self, plan): diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 5e53cfd..0398459 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -193,7 +193,7 @@ class TestNs1Provider(TestCase): @patch('nsone.NSONE.createZone') @patch('nsone.NSONE.loadZone') def test_sync(self, load_mock, create_mock): - provider = Ns1Provider('test', 'api-key', rate_limit_delay=0) + provider = Ns1Provider('test', 'api-key') desired = Zone('unit.tests.', []) desired.records.update(self.expected) @@ -231,7 +231,7 @@ class TestNs1Provider(TestCase): mock_zone = Mock() mock_zone.add_SRV = Mock() mock_zone.add_SRV.side_effect = [ - RateLimitException('boo'), + RateLimitException('boo', period=0), None, ] create_mock.side_effect = [mock_zone] @@ -259,11 +259,11 @@ class TestNs1Provider(TestCase): # trigger rate limit handling mock_record = Mock() mock_record.update.side_effect = [ - RateLimitException('one'), + RateLimitException('one', period=0), None, ] mock_record.delete.side_effect = [ - RateLimitException('two'), + RateLimitException('two', period=0), None, ] nsone_zone.loadRecord.side_effect = [mock_record, mock_record] From 06fb57855015a372b60aeea5342023247d9fd8a2 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sun, 2 Jul 2017 10:47:13 -0700 Subject: [PATCH 4/5] Include sleep duration in ns1 RateLimitException log --- octodns/provider/ns1.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index bca6118..65db64c 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -177,7 +177,7 @@ class Ns1Provider(BaseProvider): meth(name, **params) except RateLimitException as e: self.log.warn('_apply_Create: rate limit encountered, pausing ' - 'and trying again') + 'for %ds and trying again', e.period) sleep(e.period) meth(name, **params) @@ -192,7 +192,7 @@ class Ns1Provider(BaseProvider): record.update(**params) except RateLimitException as e: self.log.warn('_apply_Update: rate limit encountered, pausing ' - 'and trying again') + 'for %ds and trying again', e.period) sleep(e.period) record.update(**params) @@ -205,7 +205,7 @@ class Ns1Provider(BaseProvider): record.delete() except RateLimitException as e: self.log.warn('_apply_Delete: rate limit encountered, pausing ' - 'and trying again') + 'for %ds and trying again', e.period) sleep(e.period) record.delete() From bdceac42beb485ed30417a1ae85db1f6e45673b2 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sun, 2 Jul 2017 18:40:58 -0700 Subject: [PATCH 5/5] Fix stacktraces on MainThreadExecutor --- octodns/manager.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/octodns/manager.py b/octodns/manager.py index e6fe253..8439eb6 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -6,7 +6,7 @@ from __future__ import absolute_import, division, print_function, \ unicode_literals from StringIO import StringIO -from concurrent.futures import Future, ThreadPoolExecutor +from concurrent.futures import ThreadPoolExecutor from importlib import import_module from os import environ import logging @@ -38,6 +38,17 @@ class _AggregateTarget(object): return True +class MakeThreadFuture(object): + + def __init__(self, func, args, kwargs): + self.func = func + self.args = args + self.kwargs = kwargs + + def result(self): + return self.func(*self.args, **self.kwargs) + + class MainThreadExecutor(object): ''' Dummy executor that runs things on the main thread during the involcation @@ -48,13 +59,7 @@ class MainThreadExecutor(object): ''' def submit(self, func, *args, **kwargs): - future = Future() - try: - future.set_result(func(*args, **kwargs)) - except Exception as e: - # TODO: get right stacktrace here - future.set_exception(e) - return future + return MakeThreadFuture(func, args, kwargs) class Manager(object):