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

@@ -268,18 +268,6 @@ def save(self, **kwargs):
             self.use_ephemeral_databases = settings.INSTANCE_EPHEMERAL_DATABASES
         super().save(**kwargs)
 
-    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()
-        super().delete(*args, **kwargs)
-
     def get_load_balancer_configuration(self):
         """
         Return the haproxy configuration fragment and backend map for this instance.
@@ -415,12 +403,25 @@ 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():
+        self.remove_dns_records()
+        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()
+        for appserver in self.appserver_set.iterator():
             appserver.terminate_vm()
+
+    def delete(self, *args, **kwargs):
+        """
+        Delete this Open edX Instance and its associated AppServers, and deprovision external databases and storage.
+        """
+        self.shut_down()
+        self.deprovision_mysql()
+        self.deprovision_mongo()
+        self.deprovision_swift()
+        super().delete(*args, **kwargs)

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

@@ -469,38 +469,35 @@ def test_get_load_balancer_config_ext_domains(self):
 
     @ddt.data(True, False)
     @patch_services
-    @patch('instance.models.openedx_instance.OpenEdXAppServer.terminate_vm')
+    @patch('instance.models.openedx_instance.OpenEdXInstance.shut_down')
     @patch('instance.models.mixins.database.MySQLInstanceMixin.deprovision_mysql')
     @patch('instance.models.mixins.database.MongoDBInstanceMixin.deprovision_mongo')
     @patch('instance.models.mixins.storage.SwiftContainerInstanceMixin.deprovision_swift')
-    def test_delete_instance(
-            self, mocks, delete_by_ref,
-            mock_deprovision_swift, mock_deprovision_mongo, mock_deprovision_mysql, mock_terminate_vm
-    ):
+    def test_delete_instance(self, mocks, delete_by_ref, *mock_methods):
         """
-        Test that an instance can be deleted directly or by its InstanceReference,
-        that the associated AppServers and VMs will be terminated,
-        and that external databases and storage will be deprovisioned.
+        Test that an instance can be deleted directly or by its InstanceReference.
         """
         instance = OpenEdXInstanceFactory(sub_domain='test.deletion', use_ephemeral_databases=True)
         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
-        ):
-            self.assertEqual(mocked_method.call_count, 0)
+        for method in mock_methods:
+            self.assertEqual(
+                method.call_count, 0,
+                '{} should not have been called'.format(method._mock_name)
+            )
 
         # Now delete the instance, either using InstanceReference or the OpenEdXInstance class:
         if delete_by_ref:
             instance_ref.delete()
         else:
             instance.delete()
 
-        for mocked_method in (
-                mock_terminate_vm, mock_deprovision_mysql, mock_deprovision_mongo, mock_deprovision_swift
-        ):
-            self.assertEqual(mocked_method.call_count, 1)
+        for method in mock_methods:
+            self.assertEqual(
+                method.call_count, 1,
+                '{} should have been called exactly once'.format(method._mock_name)
+            )
 
         with self.assertRaises(OpenEdXInstance.DoesNotExist):
             OpenEdXInstance.objects.get(pk=instance.pk)
@@ -696,8 +693,9 @@ def test_is_shut_down(
 
         self.assertEqual(instance.is_shut_down, expected_result)
 
-    @patch('instance.models.openedx_instance.OpenEdXMonitoringMixin.disable_monitoring')
-    def test_shut_down(self, mock_disable_monitoring):
+    @patch('instance.models.mixins.load_balanced.LoadBalancedInstance.remove_dns_records')
+    @patch('instance.models.openedx_instance.OpenEdXInstance.set_appserver_inactive')
+    def test_shut_down(self, mock_set_appserver_inactive, mock_remove_dns_records):
         """
         Test that `shut_down` method terminates all app servers belonging to an instance
         and disables monitoring.
@@ -720,10 +718,17 @@ def test_shut_down(self, mock_disable_monitoring):
         # Set single app server active
         instance.active_appserver = active_appserver
         instance.save()
+        active_appserver.instance.refresh_from_db()
+
+        self.assertEqual(mock_set_appserver_inactive.call_count, 0)
+        self.assertEqual(mock_remove_dns_records.call_count, 0)
 
         # Shut down instance
         instance.shut_down()
 
+        self.assertEqual(mock_set_appserver_inactive.call_count, 1)
+        self.assertEqual(mock_remove_dns_records.call_count, 1)
+
         # Check status of running app servers
         self._assert_status([
             (obsolete_appserver, AppServerStatus.Terminated, ServerStatus.Terminated),
@@ -744,18 +749,20 @@ def test_shut_down(self, mock_disable_monitoring):
             (newer_appserver_failed, AppServerStatus.ConfigurationFailed, ServerStatus.Terminated),
         ])
 
-        # Check if instance disabled monitoring
-        self.assertEqual(mock_disable_monitoring.call_count, 1)
-
+    @patch('instance.models.mixins.load_balanced.LoadBalancedInstance.remove_dns_records')
+    @patch('instance.models.load_balancer.LoadBalancingServer.reconfigure')
     @responses.activate
-    def test_shut_down_monitors_not_found(self):
+    def test_shut_down_monitors_not_found(self, *mock_methods):
         """
         Test that instance `is_shut_down` after calling `shut_down` on it,
         even if monitors associated with it no longer exist.
         """
         monitor_ids = [str(uuid4()) for i in range(3)]
         instance = OpenEdXInstanceFactory()
         appserver = self._create_running_appserver(instance)
+        instance.active_appserver = appserver
+        instance.save()
+        appserver.instance.refresh_from_db()
 
         for monitor_id in monitor_ids:
             instance.new_relic_availability_monitors.create(pk=monitor_id)

instance/tests/models/test_openedx_monitoring_mixins.py

@@ -54,15 +54,6 @@ def test_set_appserver_active(self, mocks, mock_enable_monitoring):
         instance.set_appserver_active(appserver_id)
         self.assertEqual(mock_enable_monitoring.call_count, 1)
 
-    @patch('instance.models.mixins.openedx_monitoring.OpenEdXMonitoringMixin.disable_monitoring')
-    def test_delete(self, mock_disable_monitoring):
-        """
-        Check that monitoring is disabled when an appserver is deleted.
-        """
-        instance = OpenEdXInstanceFactory()
-        instance.delete()
-        self.assertEqual(mock_disable_monitoring.call_count, 1)
-
     @patch('instance.models.mixins.openedx_monitoring.newrelic')
     def test_enable_monitoring(self, mock_newrelic):
         """

instance/tests/test_gandi.py

@@ -99,10 +99,10 @@ def test_set_dns_record_error_retry_and_succeed(self, sleep):
         self.api.client.domain.zone.version.new.side_effect = [fault, fault, 'new_zone_version']
         self.api.set_dns_record(
             'test.com',
-            type='A', name='sub.domain', value='192.168.99.99', attempts=3, retry_delay=3
+            type='A', name='sub.domain', value='192.168.99.99'
         )
         self.assert_set_dns_record_calls(attempts=3)
-        self.assertEqual(sleep.mock_calls, [call(3), call(6)])
+        self.assertEqual(sleep.mock_calls, [call(1), call(2)])
 
     @patch('time.sleep')
     def test_set_dns_record_error_retry_and_fail(self, sleep):
@@ -113,6 +113,6 @@ def test_set_dns_record_error_retry_and_fail(self, sleep):
         with self.assertRaises(xmlrpc.client.Fault):
             self.api.set_dns_record(
                 'test.com',
-                type='A', name='sub.domain', value='192.168.99.99', attempts=4, retry_delay=2
+                type='A', name='sub.domain', value='192.168.99.99'
             )
-        self.assertEqual(sleep.mock_calls, [call(2), call(4), call(8)])
+        self.assertEqual(sleep.mock_calls, [call(1), call(2), call(4)])