Improve test coverage and clean up load balancer configuration fragme…

…nts.

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

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)
+        )

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)

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.
 

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:

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()

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)

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), ([], []))

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()

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,15 +424,15 @@ 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
         )
 
         # Test configuration for active appserver
         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
         )