diff --git a/instance/gandi.py b/instance/gandi.py index 7c8f940bf..a42e7f70c 100644 --- a/instance/gandi.py +++ b/instance/gandi.py @@ -99,23 +99,19 @@ 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: @@ -123,4 +119,41 @@ def set_dns_record(self, domain, attempts=4, retry_delay=1, **record): 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), + ) diff --git a/instance/models/load_balancer.py b/instance/models/load_balancer.py index fce890168..912f76c79 100644 --- a/instance/models/load_balancer.py +++ b/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 diff --git a/instance/models/mixins/load_balanced.py b/instance/models/mixins/load_balanced.py index 5e9aa7465..78f48e6d6 100644 --- a/instance/models/mixins/load_balanced.py +++ b/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. diff --git a/instance/models/openedx_instance.py b/instance/models/openedx_instance.py index 10a1e8ffc..11881d314 100644 --- a/instance/models/openedx_instance.py +++ b/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) diff --git a/instance/tests/integration/base.py b/instance/tests/integration/base.py index 1f8374605..b22cc51c5 100644 --- a/instance/tests/integration/base.py +++ b/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: diff --git a/instance/tests/models/test_openedx_instance.py b/instance/tests/models/test_openedx_instance.py index 8187f436a..8d80a1126 100644 --- a/instance/tests/models/test_openedx_instance.py +++ b/instance/tests/models/test_openedx_instance.py @@ -469,27 +469,23 @@ 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: @@ -497,10 +493,11 @@ def test_delete_instance( 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,11 +749,10 @@ 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. @@ -756,6 +760,9 @@ def test_shut_down_monitors_not_found(self): 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) diff --git a/instance/tests/models/test_openedx_monitoring_mixins.py b/instance/tests/models/test_openedx_monitoring_mixins.py index cfde4179d..a415bd99e 100644 --- a/instance/tests/models/test_openedx_monitoring_mixins.py +++ b/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): """ diff --git a/instance/tests/test_gandi.py b/instance/tests/test_gandi.py index 3e711aa75..a23d073b4 100644 --- a/instance/tests/test_gandi.py +++ b/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)])