Merge pull request #85 from github/add-record-perf

Fix major performance issue with add_record O(N^2)

octodns/__init__.py

@@ -1,6 +1,4 @@
-'''
-OctoDNS: DNS as code - Tools for managing DNS across multiple providers
-'''
+'OctoDNS: DNS as code - Tools for managing DNS across multiple providers'
 
 from __future__ import absolute_import, division, print_function, \
     unicode_literals

octodns/zone.py

@@ -5,6 +5,7 @@
 from __future__ import absolute_import, division, print_function, \
     unicode_literals
 
+from collections import defaultdict
 from logging import getLogger
 import re
 
@@ -39,13 +40,19 @@ class Zone(object):
         # Force everyting to lowercase just to be safe
         self.name = str(name).lower() if name else name
         self.sub_zones = sub_zones
-        self.records = set()
+        # We're grouping by node, it allows us to efficently search for
+        # duplicates and detect when CNAMEs co-exist with other records
+        self._records = defaultdict(set)
         # optional leading . to match empty hostname
         # optional trailing . b/c some sources don't have it on their fqdn
         self._name_re = re.compile('\.?{}?$'.format(name))
 
         self.log.debug('__init__: zone=%s, sub_zones=%s', self, sub_zones)
 
+    @property
+    def records(self):
+        return set([r for _, node in self._records.items() for r in node])
+
     def hostname_from_fqdn(self, fqdn):
         return self._name_re.sub('', fqdn)
 
@@ -53,9 +60,6 @@ class Zone(object):
         name = record.name
         last = name.split('.')[-1]
 
-        if replace and record in self.records:
-            self.records.remove(record)
-
         if last in self.sub_zones:
             if name != last:
                 # it's a record for something under a sub-zone
@@ -67,19 +71,30 @@ class Zone(object):
                 raise SubzoneRecordException('Record {} a managed sub-zone '
                                              'and not of type NS'
                                              .format(record.fqdn))
-        # TODO: this is pretty inefficent
-        for existing in self.records:
-            if record == existing:
-                raise DuplicateRecordException('Duplicate record {}, type {}'
-                                               .format(record.fqdn,
-                                                       record._type))
-            elif name == existing.name and (record._type == 'CNAME' or
-                                            existing._type == 'CNAME'):
-                raise InvalidNodeException('Invalid state, CNAME at {} '
-                                           'cannot coexist with other records'
-                                           .format(record.fqdn))
-
-        self.records.add(record)
+
+        if replace:
+            # will remove it if it exists
+            self._records[name].discard(record)
+
+        node = self._records[name]
+        if record in node:
+            # We already have a record at this node of this type
+            raise DuplicateRecordException('Duplicate record {}, type {}'
+                                           .format(record.fqdn,
+                                                   record._type))
+        elif ((record._type == 'CNAME' and len(node) > 0) or
+              ('CNAME' in map(lambda r: r._type, node))):
+            # We're adding a CNAME to existing records or adding to an existing
+            # CNAME
+            raise InvalidNodeException('Invalid state, CNAME at {} cannot '
+                                       'coexist with other records'
+                                       .format(record.fqdn))
+
+        node.add(record)
+
+    def _remove_record(self, record):
+        'Only for use in tests'
+        self._records[record.name].discard(record)
 
     def changes(self, desired, target):
         self.log.debug('changes: zone=%s, target=%s', self, target)

tests/test_octodns_provider_cloudflare.py

@@ -33,7 +33,7 @@ class TestCloudflareProvider(TestCase):
     }))
     for record in list(expected.records):
         if record.name == 'sub' and record._type == 'NS':
-            expected.records.remove(record)
+            expected._remove_record(record)
             break
 
     empty = {'result': [], 'result_info': {'count': 0, 'per_page': 0}}

tests/test_octodns_provider_dnsimple.py

@@ -33,7 +33,7 @@ class TestDnsimpleProvider(TestCase):
     }))
     for record in list(expected.records):
         if record.name == 'sub' and record._type == 'NS':
-            expected.records.remove(record)
+            expected._remove_record(record)
             break
 
     def test_populate(self):

tests/test_octodns_provider_ns1.py

@@ -196,7 +196,8 @@ class TestNs1Provider(TestCase):
         provider = Ns1Provider('test', 'api-key')
 
         desired = Zone('unit.tests.', [])
-        desired.records.update(self.expected)
+        for r in self.expected:
+            desired.add_record(r)
 
         plan = provider.plan(desired)
         # everything except the root NS

tests/test_octodns_provider_powerdns.py

@@ -253,7 +253,7 @@ class TestPowerDnsProvider(TestCase):
             plan = provider.plan(expected)
             self.assertFalse(plan)
             # remove it now that we don't need the unrelated change any longer
-            expected.records.remove(unrelated_record)
+            expected._remove_record(unrelated_record)
 
         # ttl diff
         with requests_mock() as mock:

tests/test_octodns_provider_route53.py

@@ -372,11 +372,11 @@ class TestRoute53Provider(TestCase):
         # Delete by monkey patching in a populate that includes an extra record
         def add_extra_populate(existing, target, lenient):
             for record in self.expected.records:
-                existing.records.add(record)
+                existing.add_record(record)
             record = Record.new(existing, 'extra',
                                 {'ttl': 99, 'type': 'A',
                                  'values': ['9.9.9.9']})
-            existing.records.add(record)
+            existing.add_record(record)
 
         provider.populate = add_extra_populate
         change_resource_record_sets_params = {
@@ -409,7 +409,7 @@ class TestRoute53Provider(TestCase):
         def mod_geo_populate(existing, target, lenient):
             for record in self.expected.records:
                 if record._type != 'A' or not record.geo:
-                    existing.records.add(record)
+                    existing.add_record(record)
             record = Record.new(existing, '', {
                 'ttl': 61,
                 'type': 'A',
@@ -420,7 +420,7 @@ class TestRoute53Provider(TestCase):
                     'NA-US-KY': ['7.2.3.4']
                 }
             })
-            existing.records.add(record)
+            existing.add_record(record)
 
         provider.populate = mod_geo_populate
         change_resource_record_sets_params = {
@@ -505,7 +505,7 @@ class TestRoute53Provider(TestCase):
         def mod_add_geo_populate(existing, target, lenient):
             for record in self.expected.records:
                 if record._type != 'A' or record.geo:
-                    existing.records.add(record)
+                    existing.add_record(record)
             record = Record.new(existing, 'simple', {
                 'ttl': 61,
                 'type': 'A',
@@ -514,7 +514,7 @@ class TestRoute53Provider(TestCase):
                     'OC': ['3.2.3.4', '4.2.3.4'],
                 }
             })
-            existing.records.add(record)
+            existing.add_record(record)
 
         provider.populate = mod_add_geo_populate
         change_resource_record_sets_params = {

tests/test_octodns_zone.py

@@ -77,7 +77,7 @@ class TestZone(TestCase):
         # add a record, delete a record -> [Delete, Create]
         c = ARecord(before, 'c', {'ttl': 42, 'value': '1.1.1.1'})
         after.add_record(c)
-        after.records.remove(b)
+        after._remove_record(b)
         self.assertEquals(after.records, set([a, c]))
         changes = before.changes(after, target)
         self.assertEquals(2, len(changes))