diff --git a/Makefile b/Makefile index 674f00076..910935b98 100644 --- a/Makefile +++ b/Makefile @@ -94,8 +94,7 @@ test_unit: clean static_external honcho -e .env.test run coverage run --source='.' --omit='*/tests/*' ./manage.py test --noinput coverage html @echo -e "\nCoverage HTML report at file://`pwd`/build/coverage/index.html\n" - # Temporarily relax required coverage to force all tests to run on CircleCI - @coverage report --fail-under 93 || (echo "\nERROR: Coverage is below 94%\n" && exit 2) + @coverage report --fail-under 94 || (echo "\nERROR: Coverage is below 94%\n" && exit 2) # Check whether migrations need to be generated test_migrations_missing: clean diff --git a/instance/models/load_balancer.py b/instance/models/load_balancer.py index 5a2c17912..fce890168 100644 --- a/instance/models/load_balancer.py +++ b/instance/models/load_balancer.py @@ -201,3 +201,12 @@ def reconfigure(self): """ self.logger.info("Reconfiguring load-balancing server %s", self.domain) self.run_playbook(self.get_ansible_vars()) + + def deconfigure(self): + """ + Remove the configuration fragment from the load-balancing server. + """ + fragment_name = settings.LOAD_BALANCER_FRAGMENT_NAME_PREFIX + self.fragment_name_postfix + self.run_playbook( + "FRAGMENT_NAME: {fragment_name}\nREMOVE_FRAGMENT: True".format(fragment_name=fragment_name) + ) diff --git a/instance/models/mixins/load_balanced.py b/instance/models/mixins/load_balanced.py index d0babd861..5e9aa7465 100644 --- a/instance/models/mixins/load_balanced.py +++ b/instance/models/mixins/load_balanced.py @@ -68,17 +68,19 @@ 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 + self.logger.info("Updating DNS: %s...", domain) # pylint: disable=no-member domain_parts = tldextract(domain) gandi.set_dns_record( domain_parts.registered_domain, - type='CNAME', + type="CNAME", name=domain_parts.subdomain, value=load_balancer_domain, ) def get_preliminary_page_config(self, primary_key): - """Return a load balancer configuration for the preliminary page.""" + """ + Return a load balancer configuration for the preliminary page. + """ if not settings.PRELIMINARY_PAGE_SERVER_IP: return [], [] backend_name = "be-preliminary-page-{}".format(primary_key) diff --git a/instance/tests/api/test_openedx_appserver.py b/instance/tests/api/test_openedx_appserver.py index 0fce83fc1..ae6516a28 100644 --- a/instance/tests/api/test_openedx_appserver.py +++ b/instance/tests/api/test_openedx_appserver.py @@ -165,7 +165,8 @@ def test_view_name(self): @patch('instance.models.openedx_instance.OpenEdXAppServer.provision', return_value=True) @patch('instance.models.mixins.load_balanced.gandi.set_dns_record') - def test_spawn_appserver(self, mock_set_dns_record, mock_provision): + @patch('instance.models.mixins.load_balanced.LoadBalancingServer.run_playbook') + def test_spawn_appserver(self, mock_run_playbook, mock_set_dns_record, mock_provision): """ POST /api/v1/openedx_appserver/ - Spawn a new OpenEdXAppServer for the given instance. diff --git a/instance/tests/integration/base.py b/instance/tests/integration/base.py index 72b73dbba..1f8374605 100644 --- a/instance/tests/integration/base.py +++ b/instance/tests/integration/base.py @@ -26,6 +26,7 @@ from huey.contrib import djhuey +from instance.models.load_balancer import LoadBalancingServer from instance.models.openedx_appserver import OpenEdXAppServer from instance.models.openedx_instance import OpenEdXInstance from instance.models.server import OpenStackServer @@ -56,6 +57,8 @@ def tearDown(self): instance.deprovision_swift() instance.deprovision_mongo() instance.deprovision_mysql() + for load_balancer in LoadBalancingServer.objects.iterator(): # pylint: disable=no-member + load_balancer.deconfigure() super().tearDown() # All VMs should be terminated at this point, but check just in case: diff --git a/instance/tests/models/factories/openedx_appserver.py b/instance/tests/models/factories/openedx_appserver.py index 09d6def46..0100064d5 100644 --- a/instance/tests/models/factories/openedx_appserver.py +++ b/instance/tests/models/factories/openedx_appserver.py @@ -22,6 +22,7 @@ # Imports ##################################################################### +from instance.models.load_balancer import LoadBalancingServer from instance.tests.models.factories.openedx_instance import OpenEdXInstanceFactory # Functions ################################################################### @@ -33,4 +34,7 @@ def make_test_appserver(instance=None): """ if not instance: instance = OpenEdXInstanceFactory() + if not instance.load_balancing_server: + instance.load_balancing_server = LoadBalancingServer.objects.select_random() + instance.save() return instance._create_owned_appserver() diff --git a/instance/tests/models/factories/openedx_instance.py b/instance/tests/models/factories/openedx_instance.py index 8a021bcc7..82da3deec 100644 --- a/instance/tests/models/factories/openedx_instance.py +++ b/instance/tests/models/factories/openedx_instance.py @@ -27,7 +27,6 @@ import factory from factory.django import DjangoModelFactory -from instance.models.load_balancer import LoadBalancingServer from instance.models.openedx_instance import OpenEdXInstance, generate_internal_lms_domain @@ -52,4 +51,3 @@ def create(cls, *args, **kwargs): return super(OpenEdXInstanceFactory, cls).create(*args, **kwargs) name = factory.Sequence('Test Instance {}'.format) - load_balancing_server = factory.LazyFunction(LoadBalancingServer.objects.select_random) diff --git a/instance/tests/models/test_load_balanced_mixin.py b/instance/tests/models/test_load_balanced_mixin.py index 830f32393..2b9e0abb6 100644 --- a/instance/tests/models/test_load_balanced_mixin.py +++ b/instance/tests/models/test_load_balanced_mixin.py @@ -24,6 +24,10 @@ from unittest.mock import patch, call +from django.test import override_settings + +from instance.models.load_balancer import LoadBalancingServer +from instance.models.mixins.load_balanced import LoadBalancedInstance from instance.tests.base import TestCase from instance.tests.models.factories.openedx_instance import OpenEdXInstanceFactory @@ -42,8 +46,10 @@ def test_set_dns_records(self, mock_set_dns_record): """ instance = OpenEdXInstanceFactory(internal_lms_domain='test.dns.opencraft.com', use_ephemeral_databases=True) + instance.load_balancing_server = LoadBalancingServer.objects.select_random() + instance.save() instance.set_dns_records() - lb_domain = instance.load_balancing_server.domain + "." # pylint: disable=no-member + lb_domain = instance.load_balancing_server.domain + "." self.assertEqual(mock_set_dns_record.mock_calls, [ call('opencraft.com', name='test.dns', type='CNAME', value=lb_domain), call('opencraft.com', name='preview-test.dns', type='CNAME', value=lb_domain), @@ -61,10 +67,28 @@ def test_set_dns_records_external_domain(self, mock_set_dns_record): external_lms_preview_domain='preview.myexternal.org', external_studio_domain='studio.myexternal.org', use_ephemeral_databases=True) + instance.load_balancing_server = LoadBalancingServer.objects.select_random() + instance.save() instance.set_dns_records() - lb_domain = instance.load_balancing_server.domain + "." # pylint: disable=no-member + lb_domain = instance.load_balancing_server.domain + "." self.assertEqual(mock_set_dns_record.mock_calls, [ call('opencraft.hosting', name='test.dns', type='CNAME', value=lb_domain), call('opencraft.hosting', name='preview-test.dns', type='CNAME', value=lb_domain), call('opencraft.hosting', name='studio-test.dns', type='CNAME', value=lb_domain), ]) + + def test_domains(self): + """ + Test the get_managed_domains() and get_load_balanced_domains() methods (for test coverage only). + """ + self.assertEqual(LoadBalancedInstance.get_managed_domains(None), []) + self.assertEqual(LoadBalancedInstance.get_load_balanced_domains(None), []) + + @override_settings(PRELIMINARY_PAGE_SERVER_IP=None) + def test_preliminary_page_not_configured(self): + """ + Test that get_preliminary_page_config() returns an empty configuration if + PRELIMINARY_PAGE_SERVER_IP is not set. + """ + instance = OpenEdXInstanceFactory() + self.assertEqual(instance.get_preliminary_page_config(instance.ref.pk), ([], [])) diff --git a/instance/tests/models/test_load_balancer.py b/instance/tests/models/test_load_balancer.py index aa1365dac..762a25036 100644 --- a/instance/tests/models/test_load_balancer.py +++ b/instance/tests/models/test_load_balancer.py @@ -22,6 +22,10 @@ import re from unittest.mock import patch, Mock +from django.core.exceptions import ImproperlyConfigured +from django.test import override_settings + +from instance.models.load_balancer import LoadBalancingServer, ReconfigurationFailed from instance.tests.base import TestCase from instance.tests.models.factories.load_balancer import LoadBalancingServerFactory @@ -37,40 +41,105 @@ def _make_mock_instance(domains, ip_address, backend_name): return instance +def mock_instances(): + """ + Patch out the get_instances() method. + """ + instances = [ + _make_mock_instance( + ["test1.lb.opencraft.hosting", "test2.lb.opencraft.hosting"], + "1.2.3.4", + "first-backend", + ), + _make_mock_instance( + ["test3.lb.opencraft.hosting"], + "5.6.7.8", + "second-backend", + ), + ] + return patch( + "instance.models.load_balancer.LoadBalancingServer.get_instances", + return_value=instances, + ) + + class LoadBalancingServerTest(TestCase): """ Test cases for the LoadBalancingServer model. """ + def setUp(self): + self.load_balancer = LoadBalancingServerFactory() def test_get_configuration(self): """ Test that the configuration gets rendered correctly. """ - load_balancer = LoadBalancingServerFactory() - mock_instances = [ - _make_mock_instance( - ["test1.lb.opencraft.hosting", "test2.lb.opencraft.hosting"], - "1.2.3.4", - "first-backend", - ), - _make_mock_instance( - ["test3.lb.opencraft.hosting"], - "5.6.7.8", - "second-backend", - ), - ] - with patch.object(load_balancer, "get_instances", return_value=mock_instances): - backend_map, backend_conf = load_balancer.get_configuration() + with mock_instances(): + backend_map, backend_conf = self.load_balancer.get_configuration() self.assertCountEqual( [line for line in backend_map.splitlines(False) if line], [ - "test1.lb.opencraft.hosting first-backend" + load_balancer.fragment_name_postfix, - "test2.lb.opencraft.hosting first-backend" + load_balancer.fragment_name_postfix, - "test3.lb.opencraft.hosting second-backend" + load_balancer.fragment_name_postfix, + "test1.lb.opencraft.hosting first-backend" + self.load_balancer.fragment_name_postfix, + "test2.lb.opencraft.hosting first-backend" + self.load_balancer.fragment_name_postfix, + "test3.lb.opencraft.hosting second-backend" + self.load_balancer.fragment_name_postfix, ], ) backends = [match.group(1) for match in re.finditer(r"^backend (\S*)", backend_conf, re.MULTILINE)] self.assertCountEqual(backends, [ - "first-backend" + load_balancer.fragment_name_postfix, - "second-backend" + load_balancer.fragment_name_postfix, + "first-backend" + self.load_balancer.fragment_name_postfix, + "second-backend" + self.load_balancer.fragment_name_postfix, ]) + + @patch("instance.ansible.poll_streams") + @patch("instance.ansible.run_playbook") + def test_reconfigure(self, mock_run_playbook, mock_poll_streams): + """ + Test that the reconfigure() method triggers a playbook run. + """ + mock_run_playbook.return_value.__enter__.return_value.returncode = 0 + with mock_instances(): + self.load_balancer.reconfigure() + self.assertEqual(mock_run_playbook.call_count, 1) + + @patch("instance.ansible.poll_streams") + @patch("instance.ansible.run_playbook") + def test_reconfigure_fails(self, mock_run_playbook, mock_poll_streams): + """ + Test that the reconfigure() method raises an exception if the playbook fails. + """ + mock_run_playbook.return_value.__enter__.return_value.returncode = 1 + with mock_instances(), self.assertRaises(ReconfigurationFailed): + self.load_balancer.reconfigure() + + @patch("instance.ansible.poll_streams") + @patch("instance.ansible.run_playbook") + def test_deconfigure(self, mock_run_playbook, mock_poll_streams): + """ + Test that the deconfigure() method triggers a playbook run. + """ + mock_run_playbook.return_value.__enter__.return_value.returncode = 0 + with mock_instances(): + self.load_balancer.deconfigure() + self.assertEqual(mock_run_playbook.call_count, 1) + + +class LoadBalancingServerManager(TestCase): + """ + Tests for LoadBalancingServerManager. + """ + @override_settings(DEFAULT_LOAD_BALANCING_SERVER=None) + def test_no_load_balancer_available(self): + """ + Test that get_random() raises an exception when no load balancers are available. + """ + with self.assertRaises(LoadBalancingServer.DoesNotExist): + LoadBalancingServer.objects.select_random() + + @override_settings(DEFAULT_LOAD_BALANCING_SERVER="domain.without.username") + def test_invalid_default_load_balancer(self): + """ + Verify that an exception gets raised when the username is missing from the setting for the + default load balancing server. + """ + with self.assertRaises(ImproperlyConfigured): + LoadBalancingServer.objects.select_random() diff --git a/instance/tests/models/test_openedx_instance.py b/instance/tests/models/test_openedx_instance.py index 0b0e231e7..a27ae59e3 100644 --- a/instance/tests/models/test_openedx_instance.py +++ b/instance/tests/models/test_openedx_instance.py @@ -217,7 +217,7 @@ def test_spawn_appserver(self, mocks, mock_provision): self.assertEqual(mocks.mock_provision_swift.call_count, 0) self.assertEqual(mocks.mock_set_dns_record.call_count, 3) # Three domains: LMS, LMS preview, Studio - lb_domain = instance.load_balancing_server.domain + "." # pylint: disable=no-member + lb_domain = instance.load_balancing_server.domain + "." self.assertEqual(mocks.mock_set_dns_record.mock_calls, [ call('example.com', name='test.spawn', type='CNAME', value=lb_domain), call('example.com', name='preview-test.spawn', type='CNAME', value=lb_domain), @@ -287,8 +287,13 @@ def test_set_appserver_active(self, mocks): use_ephemeral_databases=True) appserver_id = instance.spawn_appserver() instance.set_appserver_active(appserver_id) + instance.refresh_from_db() self.assertEqual(instance.active_appserver.pk, appserver_id) - self.assertEqual(mocks.mock_load_balancer_run_playbook.call_count, 1) + self.assertEqual(mocks.mock_load_balancer_run_playbook.call_count, 2) + instance.set_appserver_inactive() + instance.refresh_from_db() + self.assertIsNone(instance.active_appserver) + self.assertEqual(mocks.mock_load_balancer_run_playbook.call_count, 3) @patch_services @patch('instance.models.openedx_instance.OpenEdXAppServer.provision', return_value=True) @@ -397,7 +402,7 @@ def test_spawn_appserver_with_external_databases(self, mocks, mock_provision): 'EDXAPP_SWIFT_USERNAME', 'EDXAPP_SWIFT_KEY'): self.assertTrue(ansible_vars[setting]) - def check_load_balancer_configuration(self, backend_map, config, domain_names, ip_address): + def _check_load_balancer_configuration(self, backend_map, config, domain_names, ip_address): """ Verify the load balancer configuration given in backend_map and config. """ @@ -419,7 +424,7 @@ def test_get_load_balancer_configuration(self, mocks): # Test configuration for preliminary page backend_map, config = instance.get_load_balancer_configuration() - self.check_load_balancer_configuration( + self._check_load_balancer_configuration( backend_map, config, domain_names, settings.PRELIMINARY_PAGE_SERVER_IP ) @@ -427,7 +432,7 @@ def test_get_load_balancer_configuration(self, mocks): appserver_id = instance.spawn_appserver() instance.set_appserver_active(appserver_id) backend_map, config = instance.get_load_balancer_configuration() - self.check_load_balancer_configuration( + self._check_load_balancer_configuration( backend_map, config, domain_names, instance.active_appserver.server.public_ip ) @@ -449,7 +454,7 @@ def test_get_load_balancer_config_ext_domains(self): 'studio.myexternal.org', ] backend_map, config = instance.get_load_balancer_configuration() - self.check_load_balancer_configuration( + self._check_load_balancer_configuration( backend_map, config, domain_names, settings.PRELIMINARY_PAGE_SERVER_IP )