From 5ab238c611c1d46efe9e8439e9910d4e78450e7e Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 20 Aug 2021 13:41:04 -0700 Subject: [PATCH 01/12] Cache NS1 zones and records for faster re-retrival --- octodns/provider/ns1.py | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index f87538d..bab7c7b 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -85,6 +85,8 @@ class Ns1Client(object): self._feeds_for_monitors = None self._monitors_cache = None self._notifylists_cache = None + self._zones_cache = {} + self._records_cache = {} @property def datasource_id(self): @@ -196,19 +198,43 @@ class Ns1Client(object): return self._try(self._records.create, zone, domain, _type, **params) def records_delete(self, zone, domain, _type): + try: + # remove record from cache + del self._records_cache[zone][domain][_type] + # remove record's zone from cache + del self._zones_cache[zone] + except KeyError: + # never mind if record is not found in cache + pass return self._try(self._records.delete, zone, domain, _type) def records_retrieve(self, zone, domain, _type): - return self._try(self._records.retrieve, zone, domain, _type) + cached = self._records_cache.setdefault(zone, {}) \ + .setdefault(domain, {}) + if _type not in cached: + cached[_type] = self._try(self._records.retrieve, zone, domain, + _type) + return cached[_type] def records_update(self, zone, domain, _type, **params): + try: + # remove record from cache + del self._records_cache[zone][domain][_type] + # remove record's zone from cache + del self._zones_cache[zone] + except KeyError: + # never mind if record is not found in cache + pass return self._try(self._records.update, zone, domain, _type, **params) def zones_create(self, name): return self._try(self._zones.create, name) def zones_retrieve(self, name): - return self._try(self._zones.retrieve, name) + if name not in self._zones_cache: + self._zones_cache[name] = self._try(self._zones.retrieve, name) + print(f'insert {name} to cache with val {self._zones_cache[name]}') + return self._zones_cache[name] def _try(self, method, *args, **kwargs): tries = self.retry_count From 9522da210dad1b077f0596606d005411edebc867 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 23 Aug 2021 14:32:10 -0700 Subject: [PATCH 02/12] implement & use NS1Client.reset_caches --- octodns/provider/ns1.py | 4 +++- tests/test_octodns_provider_ns1.py | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index bab7c7b..793a59f 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -80,8 +80,10 @@ class Ns1Client(object): self._datasource = client.datasource() self._datafeed = client.datafeed() - self._datasource_id = None + self.reset_caches() + def reset_caches(self): + self._datasource_id = None self._feeds_for_monitors = None self._monitors_cache = None self._notifylists_cache = None diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 6243348..02e70d1 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -198,6 +198,7 @@ class TestNs1Provider(TestCase): provider = Ns1Provider('test', 'api-key') # Bad auth + provider._client.reset_caches() zone_retrieve_mock.side_effect = AuthException('unauthorized') zone = Zone('unit.tests.', []) with self.assertRaises(AuthException) as ctx: @@ -205,6 +206,7 @@ class TestNs1Provider(TestCase): self.assertEquals(zone_retrieve_mock.side_effect, ctx.exception) # General error + provider._client.reset_caches() zone_retrieve_mock.reset_mock() zone_retrieve_mock.side_effect = ResourceException('boom') zone = Zone('unit.tests.', []) @@ -214,6 +216,7 @@ class TestNs1Provider(TestCase): self.assertEquals(('unit.tests',), zone_retrieve_mock.call_args[0]) # Non-existent zone doesn't populate anything + provider._client.reset_caches() zone_retrieve_mock.reset_mock() zone_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') @@ -224,6 +227,7 @@ class TestNs1Provider(TestCase): self.assertFalse(exists) # Existing zone w/o records + provider._client.reset_caches() zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() ns1_zone = { @@ -255,6 +259,7 @@ class TestNs1Provider(TestCase): 'geo.unit.tests', 'A')]) # Existing zone w/records + provider._client.reset_caches() zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() ns1_zone = { @@ -286,6 +291,7 @@ class TestNs1Provider(TestCase): 'geo.unit.tests', 'A')]) # Test skipping unsupported record type + provider._client.reset_caches() zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() ns1_zone = { @@ -341,6 +347,7 @@ class TestNs1Provider(TestCase): self.assertTrue(plan.exists) # Fails, general error + provider._client.reset_caches() zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() @@ -350,6 +357,7 @@ class TestNs1Provider(TestCase): self.assertEquals(zone_retrieve_mock.side_effect, ctx.exception) # Fails, bad auth + provider._client.reset_caches() zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() @@ -361,6 +369,7 @@ class TestNs1Provider(TestCase): self.assertEquals(zone_create_mock.side_effect, ctx.exception) # non-existent zone, create + provider._client.reset_caches() zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() @@ -395,6 +404,7 @@ class TestNs1Provider(TestCase): ]) # Update & delete + provider._client.reset_caches() zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() @@ -1304,6 +1314,7 @@ class TestNs1ProviderDynamic(TestCase): # provider._params_for_A() calls provider._monitors_for() and # provider._monitor_sync(). Mock their return values so that we don't # make NS1 API calls during tests + provider._client.reset_caches() monitors_for_mock.reset_mock() monitor_sync_mock.reset_mock() monitors_for_mock.side_effect = [{ @@ -1944,6 +1955,7 @@ class TestNs1ProviderDynamic(TestCase): monitors_for_mock.assert_not_called() # Non-existent zone. No changes + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') @@ -1952,6 +1964,7 @@ class TestNs1ProviderDynamic(TestCase): self.assertFalse(extra) # Unexpected exception message + provider._client.reset_caches() zones_retrieve_mock.reset_mock() zones_retrieve_mock.side_effect = ResourceException('boom') with self.assertRaises(ResourceException) as ctx: @@ -1959,6 +1972,7 @@ class TestNs1ProviderDynamic(TestCase): self.assertEquals(zones_retrieve_mock.side_effect, ctx.exception) # Simple record, ignored, filter update lookups ignored + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2006,6 +2020,7 @@ class TestNs1ProviderDynamic(TestCase): desired.add_record(dynamic) # untouched, but everything in sync so no change needed + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2026,6 +2041,7 @@ class TestNs1ProviderDynamic(TestCase): # If we don't have a notify list we're broken and we'll expect to see # an Update + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2042,6 +2058,7 @@ class TestNs1ProviderDynamic(TestCase): # Add notify_list back and change the healthcheck protocol, we'll still # expect to see an update + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2059,6 +2076,7 @@ class TestNs1ProviderDynamic(TestCase): monitors_for_mock.assert_has_calls([call(dynamic)]) # If it's in the changed list, it'll be ignored + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2069,6 +2087,7 @@ class TestNs1ProviderDynamic(TestCase): # Test changes in filters # No change in filters + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2088,6 +2107,7 @@ class TestNs1ProviderDynamic(TestCase): self.assertFalse(extra) # filters need an update + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2107,6 +2127,7 @@ class TestNs1ProviderDynamic(TestCase): self.assertTrue(extra) # Mixed disabled in filters. Raise Ns1Exception + provider._client.reset_caches() monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() @@ -2234,12 +2255,14 @@ class TestNs1Client(TestCase): client = Ns1Client('dummy-key') # No retry required, just calls and is returned + client.reset_caches() zone_retrieve_mock.reset_mock() zone_retrieve_mock.side_effect = ['foo'] self.assertEquals('foo', client.zones_retrieve('unit.tests')) zone_retrieve_mock.assert_has_calls([call('unit.tests')]) # One retry required + client.reset_caches() zone_retrieve_mock.reset_mock() zone_retrieve_mock.side_effect = [ RateLimitException('boo', period=0), @@ -2249,6 +2272,7 @@ class TestNs1Client(TestCase): zone_retrieve_mock.assert_has_calls([call('unit.tests')]) # Two retries required + client.reset_caches() zone_retrieve_mock.reset_mock() zone_retrieve_mock.side_effect = [ RateLimitException('boo', period=0), @@ -2258,6 +2282,7 @@ class TestNs1Client(TestCase): zone_retrieve_mock.assert_has_calls([call('unit.tests')]) # Exhaust our retries + client.reset_caches() zone_retrieve_mock.reset_mock() zone_retrieve_mock.side_effect = [ RateLimitException('first', period=0), From 886ab89decd51249e03f2193e31ea10ac4228d6a Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 23 Aug 2021 14:43:38 -0700 Subject: [PATCH 03/12] Clean up NS1 mock resetting --- tests/test_octodns_provider_ns1.py | 192 +++++++++++------------------ 1 file changed, 70 insertions(+), 122 deletions(-) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 02e70d1..d220381 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -197,8 +197,14 @@ class TestNs1Provider(TestCase): def test_populate(self, zone_retrieve_mock, record_retrieve_mock): provider = Ns1Provider('test', 'api-key') + def reset(): + provider._client.reset_caches() + zone_retrieve_mock.reset_mock() + record_retrieve_mock.reset_mock() + + # Bad auth - provider._client.reset_caches() + reset() zone_retrieve_mock.side_effect = AuthException('unauthorized') zone = Zone('unit.tests.', []) with self.assertRaises(AuthException) as ctx: @@ -206,8 +212,7 @@ class TestNs1Provider(TestCase): self.assertEquals(zone_retrieve_mock.side_effect, ctx.exception) # General error - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() + reset() zone_retrieve_mock.side_effect = ResourceException('boom') zone = Zone('unit.tests.', []) with self.assertRaises(ResourceException) as ctx: @@ -216,8 +221,7 @@ class TestNs1Provider(TestCase): self.assertEquals(('unit.tests',), zone_retrieve_mock.call_args[0]) # Non-existent zone doesn't populate anything - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() + reset() zone_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') zone = Zone('unit.tests.', []) @@ -227,9 +231,7 @@ class TestNs1Provider(TestCase): self.assertFalse(exists) # Existing zone w/o records - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() - record_retrieve_mock.reset_mock() + reset() ns1_zone = { 'records': [{ "domain": "geo.unit.tests", @@ -259,9 +261,7 @@ class TestNs1Provider(TestCase): 'geo.unit.tests', 'A')]) # Existing zone w/records - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() - record_retrieve_mock.reset_mock() + reset() ns1_zone = { 'records': self.ns1_records + [{ "domain": "geo.unit.tests", @@ -291,9 +291,7 @@ class TestNs1Provider(TestCase): 'geo.unit.tests', 'A')]) # Test skipping unsupported record type - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() - record_retrieve_mock.reset_mock() + reset() ns1_zone = { 'records': self.ns1_records + [{ 'type': 'UNSUPPORTED', @@ -346,21 +344,21 @@ class TestNs1Provider(TestCase): self.assertEquals(expected_n, len(plan.changes)) self.assertTrue(plan.exists) + def reset(): + provider._client.reset_caches() + record_retrieve_mock.reset_mock() + zone_create_mock.reset_mock() + zone_retrieve_mock.reset_mock() + # Fails, general error - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() - record_retrieve_mock.reset_mock() - zone_create_mock.reset_mock() + reset() zone_retrieve_mock.side_effect = ResourceException('boom') with self.assertRaises(ResourceException) as ctx: provider.apply(plan) self.assertEquals(zone_retrieve_mock.side_effect, ctx.exception) # Fails, bad auth - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() - record_retrieve_mock.reset_mock() - zone_create_mock.reset_mock() + reset() zone_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') zone_create_mock.side_effect = AuthException('unauthorized') @@ -369,10 +367,7 @@ class TestNs1Provider(TestCase): self.assertEquals(zone_create_mock.side_effect, ctx.exception) # non-existent zone, create - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() - record_retrieve_mock.reset_mock() - zone_create_mock.reset_mock() + reset() zone_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') @@ -404,10 +399,7 @@ class TestNs1Provider(TestCase): ]) # Update & delete - provider._client.reset_caches() - zone_retrieve_mock.reset_mock() - record_retrieve_mock.reset_mock() - zone_create_mock.reset_mock() + reset() ns1_zone = { 'records': self.ns1_records + [{ @@ -947,11 +939,14 @@ class TestNs1ProviderDynamic(TestCase): 'mon-id': 'feed-id', } + def reset(): + feed_create_mock.reset_mock() + monitor_create_mock.reset_mock() + monitor_gen_mock.reset_mock() + monitors_update_mock.reset_mock() + # No existing monitor - monitor_gen_mock.reset_mock() - monitor_create_mock.reset_mock() - monitors_update_mock.reset_mock() - feed_create_mock.reset_mock() + reset() monitor_gen_mock.side_effect = [{'key': 'value'}] monitor_create_mock.side_effect = [('mon-id', 'feed-id')] value = '1.2.3.4' @@ -965,10 +960,7 @@ class TestNs1ProviderDynamic(TestCase): feed_create_mock.assert_not_called() # Existing monitor that doesn't need updates - monitor_gen_mock.reset_mock() - monitor_create_mock.reset_mock() - monitors_update_mock.reset_mock() - feed_create_mock.reset_mock() + reset() monitor = { 'id': 'mon-id', 'key': 'value', @@ -985,10 +977,7 @@ class TestNs1ProviderDynamic(TestCase): feed_create_mock.assert_not_called() # Existing monitor that doesn't need updates, but is missing its feed - monitor_gen_mock.reset_mock() - monitor_create_mock.reset_mock() - monitors_update_mock.reset_mock() - feed_create_mock.reset_mock() + reset() monitor = { 'id': 'mon-id2', 'key': 'value', @@ -1006,10 +995,7 @@ class TestNs1ProviderDynamic(TestCase): feed_create_mock.assert_has_calls([call(monitor)]) # Existing monitor that needs updates - monitor_gen_mock.reset_mock() - monitor_create_mock.reset_mock() - monitors_update_mock.reset_mock() - feed_create_mock.reset_mock() + reset() monitor = { 'id': 'mon-id', 'key': 'value', @@ -1043,11 +1029,14 @@ class TestNs1ProviderDynamic(TestCase): 'mon-id': 'feed-id', } + def reset(): + datafeed_delete_mock.reset_mock() + monitors_delete_mock.reset_mock() + monitors_for_mock.reset_mock() + notifylists_delete_mock.reset_mock() + # No active monitors and no existing, nothing will happen - monitors_for_mock.reset_mock() - datafeed_delete_mock.reset_mock() - monitors_delete_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() monitors_for_mock.side_effect = [{}] record = self.record() provider._monitors_gc(record) @@ -1057,10 +1046,7 @@ class TestNs1ProviderDynamic(TestCase): notifylists_delete_mock.assert_not_called() # No active monitors and one existing, delete all the things - monitors_for_mock.reset_mock() - datafeed_delete_mock.reset_mock() - monitors_delete_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() monitors_for_mock.side_effect = [{ 'x': { 'id': 'mon-id', @@ -1080,10 +1066,7 @@ class TestNs1ProviderDynamic(TestCase): notifylists_delete_mock.assert_has_calls([call('nl-id')]) # Same existing, this time in active list, should be noop - monitors_for_mock.reset_mock() - datafeed_delete_mock.reset_mock() - monitors_delete_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() monitors_for_mock.side_effect = [{ 'x': { 'id': 'mon-id', @@ -1098,10 +1081,7 @@ class TestNs1ProviderDynamic(TestCase): # Non-active monitor w/o a feed, and another monitor that's left alone # b/c it's active - monitors_for_mock.reset_mock() - datafeed_delete_mock.reset_mock() - monitors_delete_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() monitors_for_mock.side_effect = [{ 'x': { 'id': 'mon-id', @@ -1130,10 +1110,7 @@ class TestNs1ProviderDynamic(TestCase): # Non-active monitor w/o a notifylist, generally shouldn't happen, but # code should handle it just in case someone gets clicky in the UI - monitors_for_mock.reset_mock() - datafeed_delete_mock.reset_mock() - monitors_delete_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() monitors_for_mock.side_effect = [{ 'y': { 'id': 'mon-id2', @@ -1158,11 +1135,8 @@ class TestNs1ProviderDynamic(TestCase): # Non-active monitor with a shared notifylist, monitor deleted, but # notifylist is left alone + reset() provider.shared_notifylist = True - monitors_for_mock.reset_mock() - datafeed_delete_mock.reset_mock() - monitors_delete_mock.reset_mock() - notifylists_delete_mock.reset_mock() monitors_for_mock.side_effect = [{ 'y': { 'id': 'mon-id2', @@ -1945,37 +1919,35 @@ class TestNs1ProviderDynamic(TestCase): desired = Zone('unit.tests.', []) + def reset(): + monitors_for_mock.reset_mock() + provider._client.reset_caches() + records_retrieve_mock.reset_mock() + zones_retrieve_mock.reset_mock() + # Empty zone and no changes - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() extra = provider._extra_changes(desired, []) self.assertFalse(extra) monitors_for_mock.assert_not_called() # Non-existent zone. No changes - provider._client.reset_caches() - monitors_for_mock.reset_mock() + reset() zones_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') - records_retrieve_mock.reset_mock() extra = provider._extra_changes(desired, []) self.assertFalse(extra) # Unexpected exception message - provider._client.reset_caches() - zones_retrieve_mock.reset_mock() + reset() zones_retrieve_mock.side_effect = ResourceException('boom') with self.assertRaises(ResourceException) as ctx: extra = provider._extra_changes(desired, []) self.assertEquals(zones_retrieve_mock.side_effect, ctx.exception) # Simple record, ignored, filter update lookups ignored - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() zones_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') @@ -2020,10 +1992,7 @@ class TestNs1ProviderDynamic(TestCase): desired.add_record(dynamic) # untouched, but everything in sync so no change needed - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() # Generate what we expect to have gend = provider._monitor_gen(dynamic, '1.2.3.4') gend.update({ @@ -2041,10 +2010,7 @@ class TestNs1ProviderDynamic(TestCase): # If we don't have a notify list we're broken and we'll expect to see # an Update - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() del gend['notify_list'] monitors_for_mock.side_effect = [{ '1.2.3.4': gend, @@ -2058,10 +2024,7 @@ class TestNs1ProviderDynamic(TestCase): # Add notify_list back and change the healthcheck protocol, we'll still # expect to see an update - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() gend['notify_list'] = 'xyz' dynamic._octodns['healthcheck']['protocol'] = 'HTTPS' del gend['notify_list'] @@ -2076,10 +2039,7 @@ class TestNs1ProviderDynamic(TestCase): monitors_for_mock.assert_has_calls([call(dynamic)]) # If it's in the changed list, it'll be ignored - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() extra = provider._extra_changes(desired, [update]) self.assertFalse(extra) monitors_for_mock.assert_not_called() @@ -2087,10 +2047,7 @@ class TestNs1ProviderDynamic(TestCase): # Test changes in filters # No change in filters - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() ns1_zone = { 'records': [{ "domain": "dyn.unit.tests", @@ -2107,10 +2064,7 @@ class TestNs1ProviderDynamic(TestCase): self.assertFalse(extra) # filters need an update - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() ns1_zone = { 'records': [{ "domain": "dyn.unit.tests", @@ -2127,10 +2081,7 @@ class TestNs1ProviderDynamic(TestCase): self.assertTrue(extra) # Mixed disabled in filters. Raise Ns1Exception - provider._client.reset_caches() - monitors_for_mock.reset_mock() - zones_retrieve_mock.reset_mock() - records_retrieve_mock.reset_mock() + reset() ns1_zone = { 'records': [{ "domain": "dyn.unit.tests", @@ -2494,9 +2445,12 @@ class TestNs1Client(TestCase): notifylists_delete_mock): client = Ns1Client('dummy-key') - notifylists_list_mock.reset_mock() - notifylists_create_mock.reset_mock() - notifylists_delete_mock.reset_mock() + def reset(): + notifylists_create_mock.reset_mock() + notifylists_delete_mock.reset_mock() + notifylists_list_mock.reset_mock() + + reset() notifylists_list_mock.side_effect = [{}] expected = { 'id': 'nl-id', @@ -2518,9 +2472,7 @@ class TestNs1Client(TestCase): ]) notifylists_delete_mock.assert_not_called() - notifylists_list_mock.reset_mock() - notifylists_create_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() client.notifylists_delete('nlid') notifylists_list_mock.assert_not_called() notifylists_create_mock.assert_not_called() @@ -2528,9 +2480,7 @@ class TestNs1Client(TestCase): # Delete again, this time with a cache item that needs cleaned out and # another that needs to be ignored - notifylists_list_mock.reset_mock() - notifylists_create_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() client._notifylists_cache = { 'another': { 'id': 'notid', @@ -2549,9 +2499,7 @@ class TestNs1Client(TestCase): # Only another left self.assertEquals(['another'], list(client._notifylists_cache.keys())) - notifylists_list_mock.reset_mock() - notifylists_create_mock.reset_mock() - notifylists_delete_mock.reset_mock() + reset() expected = ['one', 'two', 'three'] notifylists_list_mock.side_effect = [expected] nls = client.notifylists_list() From 64072f9f43df9da95b6dcf9223ac0aa6d47bb938 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 23 Aug 2021 15:36:57 -0700 Subject: [PATCH 04/12] Coverage test for NS1 client caching behaviors --- octodns/provider/ns1.py | 9 ++- tests/test_octodns_provider_ns1.py | 109 ++++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 793a59f..f46bca9 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -197,7 +197,11 @@ class Ns1Client(object): return self._try(self._notifylists.list) def records_create(self, zone, domain, _type, **params): - return self._try(self._records.create, zone, domain, _type, **params) + cached = self._records_cache.setdefault(zone, {}) \ + .setdefault(domain, {}) + cached[_type] = self._try(self._records.create, zone, domain, _type, + **params) + return cached[_type] def records_delete(self, zone, domain, _type): try: @@ -230,7 +234,8 @@ class Ns1Client(object): return self._try(self._records.update, zone, domain, _type, **params) def zones_create(self, name): - return self._try(self._zones.create, name) + self._zones_cache[name] = self._try(self._zones.create, name) + return self._zones_cache[name] def zones_retrieve(self, name): if name not in self._zones_cache: diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index d220381..3b4fc6b 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -202,7 +202,6 @@ class TestNs1Provider(TestCase): zone_retrieve_mock.reset_mock() record_retrieve_mock.reset_mock() - # Bad auth reset() zone_retrieve_mock.side_effect = AuthException('unauthorized') @@ -2507,3 +2506,111 @@ class TestNs1Client(TestCase): notifylists_list_mock.assert_has_calls([call()]) notifylists_create_mock.assert_not_called() notifylists_delete_mock.assert_not_called() + + @patch('ns1.rest.records.Records.delete') + @patch('ns1.rest.records.Records.update') + @patch('ns1.rest.records.Records.create') + @patch('ns1.rest.records.Records.retrieve') + @patch('ns1.rest.zones.Zones.create') + @patch('ns1.rest.zones.Zones.delete') + @patch('ns1.rest.zones.Zones.retrieve') + def test_client_caching(self, zone_retrieve_mock, zone_delete_mock, + zone_create_mock, record_retrieve_mock, + record_create_mock, record_update_mock, + record_delete_mock): + client = Ns1Client('dummy-key') + + def reset(): + zone_retrieve_mock.reset_mock() + zone_delete_mock.reset_mock() + zone_create_mock.reset_mock() + record_retrieve_mock.reset_mock() + record_create_mock.reset_mock() + record_update_mock.reset_mock() + record_delete_mock.reset_mock() + # Testing caches so we don't reset those + + # Initial zone get fetches and caches + reset() + zone_retrieve_mock.side_effect = ['foo'] + self.assertEquals('foo', client.zones_retrieve('unit.tests')) + zone_retrieve_mock.assert_has_calls([call('unit.tests')]) + self.assertEquals({ + 'unit.tests': 'foo', + }, client._zones_cache) + + # Subsequent zone get does not fetch and returns from cache + reset() + self.assertEquals('foo', client.zones_retrieve('unit.tests')) + zone_retrieve_mock.assert_not_called() + + # Zone create stores in cache + reset() + zone_create_mock.side_effect = ['bar'] + self.assertEquals('bar', client.zones_create('sub.unit.tests')) + zone_create_mock.assert_has_calls([call('sub.unit.tests')]) + self.assertEquals({ + 'sub.unit.tests': 'bar', + 'unit.tests': 'foo', + }, client._zones_cache) + + # Initial record get fetches and caches + reset() + record_retrieve_mock.side_effect = ['baz'] + self.assertEquals('baz', client.records_retrieve('unit.tests', + 'a.unit.tests', 'A')) + record_retrieve_mock.assert_has_calls([call('unit.tests', + 'a.unit.tests', 'A')]) + self.assertEquals({ + 'unit.tests': { + 'a.unit.tests': { + 'A': 'baz' + } + } + }, client._records_cache) + + # Subsequent record get does not fetch and returns from cache + reset() + self.assertEquals('baz', client.records_retrieve('unit.tests', + 'a.unit.tests', 'A')) + record_retrieve_mock.assert_not_called() + + # Record create stores in cache + reset() + record_create_mock.side_effect = ['boo'] + self.assertEquals('boo', client.records_create('unit.tests', + 'aaaa.unit.tests', + 'AAAA', key='val')) + record_create_mock.assert_has_calls([call('unit.tests', + 'aaaa.unit.tests', 'AAAA', + key='val')]) + self.assertEquals({ + 'unit.tests': { + 'a.unit.tests': { + 'A': 'baz' + }, + 'aaaa.unit.tests': { + 'AAAA': 'boo' + }, + } + }, client._records_cache) + + # Record delete removes from cache and removes zone + reset() + record_delete_mock.side_effect = ['hoo'] + self.assertEquals('hoo', client.records_delete('unit.tests', + 'aaaa.unit.tests', + 'AAAA')) + record_delete_mock.assert_has_calls([call('unit.tests', + 'aaaa.unit.tests', 'AAAA')]) + self.assertEquals({ + 'unit.tests': { + 'a.unit.tests': { + 'A': 'baz' + }, + 'aaaa.unit.tests': {}, + } + }, client._records_cache) + self.assertEquals({ + 'sub.unit.tests': 'bar', + }, client._zones_cache) From efdb4866c019c6b35dddba06f1f3c7e2355f16cb Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 23 Aug 2021 15:37:02 -0700 Subject: [PATCH 05/12] Remove a couple of stray prints --- octodns/provider/ns1.py | 1 - tests/test_octodns_processor_acme.py | 1 - 2 files changed, 2 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index f46bca9..32a6b3e 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -240,7 +240,6 @@ class Ns1Client(object): def zones_retrieve(self, name): if name not in self._zones_cache: self._zones_cache[name] = self._try(self._zones.retrieve, name) - print(f'insert {name} to cache with val {self._zones_cache[name]}') return self._zones_cache[name] def _try(self, method, *args, **kwargs): diff --git a/tests/test_octodns_processor_acme.py b/tests/test_octodns_processor_acme.py index c927608..02177f7 100644 --- a/tests/test_octodns_processor_acme.py +++ b/tests/test_octodns_processor_acme.py @@ -71,7 +71,6 @@ class TestAcmeMangingProcessor(TestCase): ], sorted([r.name for r in got.records])) managed = None for record in got.records: - print(record.name) if record.name.endswith('managed'): managed = record break From aa88b877c4c4e08d0336a01b532d9d5b21dbedb3 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 23 Aug 2021 17:53:07 -0700 Subject: [PATCH 06/12] Clear NS1 zone cache before record cache --- octodns/provider/ns1.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 32a6b3e..1072245 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -205,10 +205,11 @@ class Ns1Client(object): def records_delete(self, zone, domain, _type): try: - # remove record from cache - del self._records_cache[zone][domain][_type] # remove record's zone from cache del self._zones_cache[zone] + # remove record from cache, after zone since we may not have + # fetched the record details + del self._records_cache[zone][domain][_type] except KeyError: # never mind if record is not found in cache pass @@ -224,10 +225,11 @@ class Ns1Client(object): def records_update(self, zone, domain, _type, **params): try: - # remove record from cache - del self._records_cache[zone][domain][_type] # remove record's zone from cache del self._zones_cache[zone] + # remove record from cache, after zone since we may not have + # fetched the record details + del self._records_cache[zone][domain][_type] except KeyError: # never mind if record is not found in cache pass From 025180ac3f80e358e233e0ee52d7632cfc3b0609 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 23 Aug 2021 18:00:24 -0700 Subject: [PATCH 07/12] NS1Client.records_update result caching & tests --- octodns/provider/ns1.py | 8 ++++++-- tests/test_octodns_provider_ns1.py | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 1072245..b93d3b6 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -224,16 +224,20 @@ class Ns1Client(object): return cached[_type] def records_update(self, zone, domain, _type, **params): + cached = self._records_cache.setdefault(zone, {}) \ + .setdefault(domain, {}) try: # remove record's zone from cache del self._zones_cache[zone] # remove record from cache, after zone since we may not have # fetched the record details - del self._records_cache[zone][domain][_type] + del cached[_type] except KeyError: # never mind if record is not found in cache pass - return self._try(self._records.update, zone, domain, _type, **params) + cached[_type] = self._try(self._records.update, zone, domain, _type, + **params) + return cached[_type] def zones_create(self, name): self._zones_cache[name] = self._try(self._zones.create, name) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 3b4fc6b..7a36fb9 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -2614,3 +2614,26 @@ class TestNs1Client(TestCase): self.assertEquals({ 'sub.unit.tests': 'bar', }, client._zones_cache) + + # Record update removes zone and caches result + record_update_mock.side_effect = ['done'] + self.assertEquals('done', client.records_update('sub.unit.tests', + 'aaaa.sub.unit.tests', + 'AAAA', key='val')) + record_update_mock.assert_has_calls([call('sub.unit.tests', + 'aaaa.sub.unit.tests', + 'AAAA', key='val')]) + self.assertEquals({ + 'unit.tests': { + 'a.unit.tests': { + 'A': 'baz' + }, + 'aaaa.unit.tests': {}, + }, + 'sub.unit.tests': { + 'aaaa.sub.unit.tests': { + 'AAAA': 'done', + }, + } + }, client._records_cache) + self.assertEquals({}, client._zones_cache) From c9fc8feae2101f19aa46896f552acecfc666a2a2 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Tue, 24 Aug 2021 00:01:49 -0700 Subject: [PATCH 08/12] Centralized NS1 record cache management with decorator --- octodns/provider/ns1.py | 61 ++++++++++++++++-------------- tests/test_octodns_provider_ns1.py | 8 ++-- 2 files changed, 37 insertions(+), 32 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index b93d3b6..242ebfb 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -90,6 +90,34 @@ class Ns1Client(object): self._zones_cache = {} self._records_cache = {} + def reset_record_cache(self, zone, domain, _type): + try: + # remove record's zone from cache + del self._zones_cache[zone] + except KeyError: + # never mind if zone is not found in cache + pass + + try: + # remove record from cache + del self._records_cache[zone][domain][_type] + except KeyError: + # never mind if record is not found in cache + pass + + def update_record_cache(func): + def call(self, zone, domain, _type, **params): + self.reset_record_cache(zone, domain, _type) + new_record = func(self, zone, domain, _type, **params) + if new_record: + cached = self._records_cache.setdefault(zone, {}) \ + .setdefault(domain, {}) + cached[_type] = new_record + + return new_record + + return call + @property def datasource_id(self): if self._datasource_id is None: @@ -196,23 +224,12 @@ class Ns1Client(object): def notifylists_list(self): return self._try(self._notifylists.list) + @update_record_cache def records_create(self, zone, domain, _type, **params): - cached = self._records_cache.setdefault(zone, {}) \ - .setdefault(domain, {}) - cached[_type] = self._try(self._records.create, zone, domain, _type, - **params) - return cached[_type] + return self._try(self._records.create, zone, domain, _type, **params) + @update_record_cache def records_delete(self, zone, domain, _type): - try: - # remove record's zone from cache - del self._zones_cache[zone] - # remove record from cache, after zone since we may not have - # fetched the record details - del self._records_cache[zone][domain][_type] - except KeyError: - # never mind if record is not found in cache - pass return self._try(self._records.delete, zone, domain, _type) def records_retrieve(self, zone, domain, _type): @@ -223,21 +240,9 @@ class Ns1Client(object): _type) return cached[_type] + @update_record_cache def records_update(self, zone, domain, _type, **params): - cached = self._records_cache.setdefault(zone, {}) \ - .setdefault(domain, {}) - try: - # remove record's zone from cache - del self._zones_cache[zone] - # remove record from cache, after zone since we may not have - # fetched the record details - del cached[_type] - except KeyError: - # never mind if record is not found in cache - pass - cached[_type] = self._try(self._records.update, zone, domain, _type, - **params) - return cached[_type] + return self._try(self._records.update, zone, domain, _type, **params) def zones_create(self, name): self._zones_cache[name] = self._try(self._zones.create, name) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 7a36fb9..bd13e97 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -2597,10 +2597,10 @@ class TestNs1Client(TestCase): # Record delete removes from cache and removes zone reset() - record_delete_mock.side_effect = ['hoo'] - self.assertEquals('hoo', client.records_delete('unit.tests', - 'aaaa.unit.tests', - 'AAAA')) + record_delete_mock.side_effect = [{}] + self.assertEquals({}, client.records_delete('unit.tests', + 'aaaa.unit.tests', + 'AAAA')) record_delete_mock.assert_has_calls([call('unit.tests', 'aaaa.unit.tests', 'AAAA')]) self.assertEquals({ From 3754c1677497e299ed4391038c5f7b3e3dd2ea5a Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Tue, 24 Aug 2021 00:19:53 -0700 Subject: [PATCH 09/12] Cleaner and more compact NS1 record cache management --- octodns/provider/ns1.py | 47 ++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 242ebfb..3044c85 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -90,34 +90,37 @@ class Ns1Client(object): self._zones_cache = {} self._records_cache = {} - def reset_record_cache(self, zone, domain, _type): - try: - # remove record's zone from cache - del self._zones_cache[zone] - except KeyError: - # never mind if zone is not found in cache - pass - - try: - # remove record from cache - del self._records_cache[zone][domain][_type] - except KeyError: - # never mind if record is not found in cache - pass - def update_record_cache(func): def call(self, zone, domain, _type, **params): - self.reset_record_cache(zone, domain, _type) + if zone in self._zones_cache: + # remove record's zone from cache + del self._zones_cache[zone] + + cached = self._records_cache.setdefault(zone, {}) \ + .setdefault(domain, {}) new_record = func(self, zone, domain, _type, **params) if new_record: - cached = self._records_cache.setdefault(zone, {}) \ - .setdefault(domain, {}) + # record is created/updated cached[_type] = new_record + elif _type in cached: + # record is deleted + del cached[_type] return new_record return call + def read_or_set_record_cache(func): + def call(self, zone, domain, _type): + cached = self._records_cache.setdefault(zone, {}) \ + .setdefault(domain, {}) + if _type not in cached: + cached[_type] = func(self, zone, domain, _type) + + return cached[_type] + + return call + @property def datasource_id(self): if self._datasource_id is None: @@ -232,13 +235,9 @@ class Ns1Client(object): def records_delete(self, zone, domain, _type): return self._try(self._records.delete, zone, domain, _type) + @read_or_set_record_cache def records_retrieve(self, zone, domain, _type): - cached = self._records_cache.setdefault(zone, {}) \ - .setdefault(domain, {}) - if _type not in cached: - cached[_type] = self._try(self._records.retrieve, zone, domain, - _type) - return cached[_type] + return self._try(self._records.retrieve, zone, domain, _type) @update_record_cache def records_update(self, zone, domain, _type, **params): From 106971853cf7d98036f2f333e788cb606c31d4f9 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Tue, 24 Aug 2021 00:23:35 -0700 Subject: [PATCH 10/12] add comment --- octodns/provider/ns1.py | 1 + 1 file changed, 1 insertion(+) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 3044c85..fec5c47 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -96,6 +96,7 @@ class Ns1Client(object): # remove record's zone from cache del self._zones_cache[zone] + # write to (or delete) record cache cached = self._records_cache.setdefault(zone, {}) \ .setdefault(domain, {}) new_record = func(self, zone, domain, _type, **params) From 2914f52ff3612741177c4078b3160253c9689e42 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Tue, 24 Aug 2021 00:29:56 -0700 Subject: [PATCH 11/12] evict NS1 record from cache before an operation --- octodns/provider/ns1.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index fec5c47..80e9603 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -96,16 +96,17 @@ class Ns1Client(object): # remove record's zone from cache del self._zones_cache[zone] - # write to (or delete) record cache cached = self._records_cache.setdefault(zone, {}) \ .setdefault(domain, {}) + + if _type in cached: + # remove record from cache + del cached[_type] + + # write record to cache if its not a delete new_record = func(self, zone, domain, _type, **params) if new_record: - # record is created/updated cached[_type] = new_record - elif _type in cached: - # record is deleted - del cached[_type] return new_record From 56b8b23391cd59a47da3cd751b67bb8b176d3dc0 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 24 Aug 2021 09:18:14 -0700 Subject: [PATCH 12/12] Delete second ns1 record to make sure cache clears w/o zone --- tests/test_octodns_provider_ns1.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index bd13e97..1d3f49b 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -2615,6 +2615,24 @@ class TestNs1Client(TestCase): 'sub.unit.tests': 'bar', }, client._zones_cache) + # Delete the other record, no zone this time, record should still go + # away + reset() + record_delete_mock.side_effect = [{}] + self.assertEquals({}, client.records_delete('unit.tests', + 'a.unit.tests', 'A')) + record_delete_mock.assert_has_calls([call('unit.tests', 'a.unit.tests', + 'A')]) + self.assertEquals({ + 'unit.tests': { + 'a.unit.tests': {}, + 'aaaa.unit.tests': {}, + } + }, client._records_cache) + self.assertEquals({ + 'sub.unit.tests': 'bar', + }, client._zones_cache) + # Record update removes zone and caches result record_update_mock.side_effect = ['done'] self.assertEquals('done', client.records_update('sub.unit.tests', @@ -2625,9 +2643,7 @@ class TestNs1Client(TestCase): 'AAAA', key='val')]) self.assertEquals({ 'unit.tests': { - 'a.unit.tests': { - 'A': 'baz' - }, + 'a.unit.tests': {}, 'aaaa.unit.tests': {}, }, 'sub.unit.tests': {