Reorganize clean-up code and delete DNS records when shutting down in…

…stances.

instance/gandi.py

@@ -99,28 +99,61 @@ def set_zone_version(self, zone_id, zone_version_id):
         """
         return self.client_zone.version.set(self.api_key, zone_id, zone_version_id)
 
-    def set_dns_record(self, domain, attempts=4, retry_delay=1, **record):
+    def _dns_operation(self, callback, domain, log_msg, attempts=4, retry_delay=1):
         """
-        Set a DNS record - Automatically create a new version, update with the change & activate
-        This method takes the mandatory `domain` parameter to be able to support multiple domains,
-        handled by the same Gandi account
+        Encapsulate logic that is common to high-level DNS operations: grab the global lock, get the
+        zone_id for a domain, create a new zone version, activate the zone version after successful
+        update, and retry the whole procedure multiple times if necessary.
         """
-        if 'ttl' not in record.keys():
-            record['ttl'] = 1200
-
         with cache.lock('gandi_set_dns_record'): # Only do one DNS update at a time
             for i in range(1, attempts + 1):
                 try:
-                    logger.info('Setting DNS record: %s (attempt %d out of %d)', record, i, attempts)
+                    logger.info('%s (attempt %d out of %d)', log_msg, i, attempts)
                     zone_id = self.get_zone_id(domain)
                     new_zone_version = self.create_new_zone_version(zone_id)
-                    self.delete_dns_record(zone_id, new_zone_version, record['name'])
-                    returned_record = self.add_dns_record(zone_id, new_zone_version, record)
+                    result = callback(zone_id, new_zone_version)
                     self.set_zone_version(zone_id, new_zone_version)
                     break
                 except xmlrpc.client.Fault:
                     if i == attempts:
                         raise
                     time.sleep(retry_delay)
                     retry_delay *= 2
-        return returned_record
+        return result
+
+    def set_dns_record(self, domain, **record):
+        """
+        Set a DNS record. This method takes the mandatory `domain` parameter to be able to support
+        multiple domains, handled by the same Gandi account.
+        """
+        if 'ttl' not in record.keys():
+            record['ttl'] = 1200
+
+        def set_dns_record_callback(zone_id, zone_version):
+            """
+            Callback to be passed to _dns_operation().
+            """
+            self.delete_dns_record(zone_id, zone_version, record['name'])
+            return self.add_dns_record(zone_id, zone_version, record)
+
+        self._dns_operation(
+            callback=set_dns_record_callback,
+            domain=domain,
+            log_msg='Setting DNS record: {}'.format(record),
+        )
+
+    def remove_dns_record(self, domain, name):
+        """
+        Remove the given name for the domain.
+        """
+        def remove_dns_record_callback(zone_id, zone_version):
+            """
+            Callback to be passed to _dns_operation().
+            """
+            self.delete_dns_record(zone_id, zone_version, name)
+
+        self._dns_operation(
+            callback=remove_dns_record_callback,
+            domain=domain,
+            log_msg='Deleting DNS record: {}'.format(name),
+        )

instance/models/load_balancer.py

@@ -210,3 +210,10 @@ def deconfigure(self):
         self.run_playbook(
             "FRAGMENT_NAME: {fragment_name}\nREMOVE_FRAGMENT: True".format(fragment_name=fragment_name)
         )
+
+    def delete(self, *args, **kwargs):
+        """
+        Delete the LoadBalancingServer from the database.
+        """
+        self.deconfigure()
+        super().delete(*args, **kwargs)  # pylint: disable=no-member

instance/models/mixins/load_balanced.py

@@ -68,7 +68,6 @@ def set_dns_records(self):
         """
         load_balancer_domain = self.load_balancing_server.domain.rstrip(".") + "."
         for domain in self.get_managed_domains():
-            self.logger.info("Updating DNS: %s...", domain)  # pylint: disable=no-member
             domain_parts = tldextract(domain)
             gandi.set_dns_record(
                 domain_parts.registered_domain,
@@ -77,6 +76,14 @@ def set_dns_records(self):
                 value=load_balancer_domain,
             )
 
+    def remove_dns_records(self):
+        """
+        Delete the DNS records for this instance.
+        """
+        for domain in self.get_managed_domains():
+            domain_parts = tldextract(domain)
+            gandi.remove_dns_record(domain_parts.registered_domain, domain_parts.subdomain)
+
     def get_preliminary_page_config(self, primary_key):
         """
         Return a load balancer configuration for the preliminary page.

instance/models/openedx_instance.py

@@ -272,12 +272,7 @@ def delete(self, *args, **kwargs):
         """
         Delete this Open edX Instance and its associated AppServers, and deprovision external databases and storage.
         """
-        self.disable_monitoring()
-        for appserver in self.appserver_set.all():
-            appserver.terminate_vm()
-        self.deprovision_mysql()
-        self.deprovision_mongo()
-        self.deprovision_swift()
+        self.shut_down()
         super().delete(*args, **kwargs)
 
     def get_load_balancer_configuration(self):
@@ -415,12 +410,18 @@ def terminate_obsolete_appservers(self, days=2):
     def shut_down(self):
         """
         Shut down this instance.
-
-        This process consists of two steps:
-
-        1) Disable New Relic monitors.
-        2) Terminate all app servers belonging to this instance.
         """
-        self.disable_monitoring()
-        for appserver in self.appserver_set.all():
+        if self.load_balancing_server is not None:
+            load_balancer = self.load_balancing_server
+            self.load_balancing_server = None
+            self.save()
+            if self.active_appserver is None:
+                # If an appserver is active, reconfiguring the load_balancer happens
+                # implicitly when terminate_vm() is called further down.
+                load_balancer.reconfigure()
+        self.remove_dns_records()
+        self.deprovision_mysql()
+        self.deprovision_mongo()
+        self.deprovision_swift()
+        for appserver in self.appserver_set.iterator():
             appserver.terminate_vm()

instance/tests/integration/base.py

@@ -51,14 +51,15 @@ def setUp(self):
         patcher.start()
 
     def tearDown(self):
-        for appserver in OpenEdXAppServer.objects.iterator():
-            appserver.terminate_vm()
+        # Trigger clean-up operations for load-balancers and instances.  To avoid reconfiguring the
+        # load balancing server multiple time, we first remove the configured load balancer from all
+        # instances, the delete the load balancers, the delete the instances.
         for instance in OpenEdXInstance.objects.iterator():
-            instance.deprovision_swift()
-            instance.deprovision_mongo()
-            instance.deprovision_mysql()
-        for load_balancer in LoadBalancingServer.objects.iterator():  # pylint: disable=no-member
-            load_balancer.deconfigure()
+            instance.load_balancing_server = None
+            instance.save()
+        LoadBalancingServer.objects.delete()  # pylint: disable=no-member
+        OpenEdXInstance.objects.delete()
+
         super().tearDown()
 
         # All VMs should be terminated at this point, but check just in case:

instance/tests/models/test_openedx_instance.py

@@ -473,9 +473,9 @@ def test_get_load_balancer_config_ext_domains(self):
     @patch('instance.models.mixins.database.MySQLInstanceMixin.deprovision_mysql')
     @patch('instance.models.mixins.database.MongoDBInstanceMixin.deprovision_mongo')
     @patch('instance.models.mixins.storage.SwiftContainerInstanceMixin.deprovision_swift')
+    @patch('instance.models.mixins.load_balanced.LoadBalancedInstance.remove_dns_records')
     def test_delete_instance(
-            self, mocks, delete_by_ref,
-            mock_deprovision_swift, mock_deprovision_mongo, mock_deprovision_mysql, mock_terminate_vm
+            self, mocks, delete_by_ref, mock_remove_dns_record, *mocked_methods
     ):
         """
         Test that an instance can be deleted directly or by its InstanceReference,
@@ -486,9 +486,7 @@ def test_delete_instance(
         instance_ref = instance.ref
         appserver = OpenEdXAppServer.objects.get(pk=instance.spawn_appserver())
 
-        for mocked_method in (
-                mock_terminate_vm, mock_deprovision_mysql, mock_deprovision_mongo, mock_deprovision_swift
-        ):
+        for mocked_method in mocked_methods:
             self.assertEqual(mocked_method.call_count, 0)
 
         # Now delete the instance, either using InstanceReference or the OpenEdXInstance class:
@@ -497,9 +495,7 @@ def test_delete_instance(
         else:
             instance.delete()
 
-        for mocked_method in (
-                mock_terminate_vm, mock_deprovision_mysql, mock_deprovision_mongo, mock_deprovision_swift
-        ):
+        for mocked_method in mocked_methods:
             self.assertEqual(mocked_method.call_count, 1)
 
         with self.assertRaises(OpenEdXInstance.DoesNotExist):