Repository: Duke-GCB/lando

Issue:
Floating IP address exhaustion
The current implementation allocates a new floating IP address.
Once these are exhausted the VM will not launch.
Need to re-use existing floating IP addresses or delete them when we are done.
We don't technically need to have floating IPs for running jobs but it makes debugging easier.

Discussion:

Patch:
diff --git a/README.md b/README.md
index 8130b3b..4adc5b5 100644
--- a/README.md
+++ b/README.md
@@ -87,6 +87,7 @@ vm_settings:
   worker_image_name: lando_worker    # Name of the image that has lando installed and lando_worker setup to run as a service
   ssh_key_name: jpb67                # Name of the openstack SSH key to install on the worker
   network_name: selfservice          # Openstack network name to add the vm onto
+  allocate_floating_ips: false       # Should each worker VM get allocated a floating IP defaults to false
   floating_ip_pool_name: ext-net     # Pool of floating IP's that will have one assigned to the VM
   default_favor_name: m1.small       # Flavor of image to use by default when creating a VM
 
@@ -97,7 +98,7 @@ cloud_settings:
   password: secret3  
   user_domain_name: Default               
   project_name: jpb67               # name of the project we will add VMs to
-  project_domain_name: Default    
+  project_domain_name: Default
 
 # Bespin job API settings
 bespin_api:
diff --git a/lando/server/cloudservice.py b/lando/server/cloudservice.py
index 01be99f..c5f4e8a 100644
--- a/lando/server/cloudservice.py
+++ b/lando/server/cloudservice.py
@@ -3,6 +3,7 @@ Allows launching and terminating openstack virtual machines.
 """
 from keystoneauth1 import session
 from keystoneauth1 import loading
+import novaclient.exceptions
 from novaclient import client as nvclient
 from time import sleep
 import logging
@@ -67,13 +68,28 @@ class NovaClient(object):
         instance.add_floating_ip(floating_ip)
         return floating_ip.ip
 
-    def terminate_instance(self, server_name):
+    def _delete_floating_ip(self, instance):
+        """
+        Delete floating ip address associated with the instance with server_name.
+        If not found will log as as warning.
+        :param instance: Server: VM we want to delete a floating IP from
+        """
+        try:
+            floating_ip = self.nova.floating_ips.find(instance_id=instance.id)
+            floating_ip.delete()
+        except novaclient.exceptions.NotFound:
+            logging.warn('No floating ip address found for {} ({})'.format(instance.name, instance.id))
+
+    def terminate_instance(self, server_name, delete_floating_ip):
         """
         Terminate a VM based on name.
         :param server_name: str: name of the VM to terminate
+        :param delete_floating_ip: bool: should we try to delete an attached floating ip address
         """
-        s = self.nova.servers.find(name=server_name)
-        self.nova.servers.delete(s)
+        instance = self.nova.servers.find(name=server_name)
+        if delete_floating_ip:
+            self._delete_floating_ip(instance)
+        self.nova.servers.delete(instance)
 
 
 class CloudService(object):
@@ -99,18 +115,23 @@ class CloudService(object):
         """
         vm_settings = self.config.vm_settings
         instance = self.nova_client.launch_instance(vm_settings, server_name, flavor_name, script_contents)
-        sleep(WAIT_BEFORE_ATTACHING_IP)
-        ip_address = self.nova_client.attach_floating_ip(instance, vm_settings.floating_ip_pool_name)
-        logging.info('launched {} on ip {}'.format(server_name, ip_address))
+        ip_address = None
+        if vm_settings.allocate_floating_ips:
+            sleep(WAIT_BEFORE_ATTACHING_IP)
+            ip_address = self.nova_client.attach_floating_ip(instance, vm_settings.floating_ip_pool_name)
+            logging.info('launched {} on floating ip {}'.format(server_name, ip_address))
+        else:
+            logging.info('launched {} with id'.format(server_name, instance.id))
         return instance, ip_address
 
     def terminate_instance(self, server_name):
         """
-        Terminate the VM with server_name.
+        Terminate the VM with server_name and deletes attached floating ip address
         :param server_name: str: name of the VM to terminate
         """
+        vm_settings = self.config.vm_settings
         logging.info('terminating instance {}'.format(server_name))
-        self.nova_client.terminate_instance(server_name)
+        self.nova_client.terminate_instance(server_name, delete_floating_ip=vm_settings.allocate_floating_ips)
 
     def make_vm_name(self, job_id):
         """
@@ -136,4 +157,4 @@ class FakeCloudService(object):
         print("Pretend we terminate: {}".format(server_name))
 
     def make_vm_name(self, job_id):
-        return 'local_worker'
\ No newline at end of file
+        return 'local_worker'
diff --git a/lando/server/config.py b/lando/server/config.py
index 8ed34d8..208c471 100644
--- a/lando/server/config.py
+++ b/lando/server/config.py
@@ -71,6 +71,7 @@ class VMSettings(object):
         self.worker_image_name = get_or_raise_config_exception(data, 'worker_image_name')
         self.ssh_key_name = get_or_raise_config_exception(data, 'ssh_key_name')
         self.network_name = get_or_raise_config_exception(data, 'network_name')
+        self.allocate_floating_ips = data.get('allocate_floating_ips', False)
         self.floating_ip_pool_name = get_or_raise_config_exception(data, 'floating_ip_pool_name')
         self.default_favor_name = get_or_raise_config_exception(data, 'default_favor_name')
 


Test patch:
diff --git a/lando/server/tests/test_cloudservice.py b/lando/server/tests/test_cloudservice.py
index c26704c..c8e8c58 100644
--- a/lando/server/tests/test_cloudservice.py
+++ b/lando/server/tests/test_cloudservice.py
@@ -1,14 +1,16 @@
 from __future__ import absolute_import
 from unittest import TestCase
+import novaclient.exceptions
 from lando.server.cloudservice import CloudService
 import mock
 
 
 class TestCwlWorkflow(TestCase):
+    @mock.patch('lando.server.cloudservice.sleep')
     @mock.patch('keystoneauth1.loading.get_plugin_loader')
     @mock.patch('keystoneauth1.session.Session')
     @mock.patch('novaclient.client.Client')
-    def test_that_flavor_overrides_default(self, mock_client, mock_session, mock_get_plugin_loader):
+    def test_that_flavor_overrides_default(self, mock_client, mock_session, mock_get_plugin_loader, mock_sleep):
         config = mock.MagicMock()
         config.vm_settings.default_favor_name = 'm1.xbig'
         cloud_service = CloudService(config, project_name='bespin_user1')
@@ -16,10 +18,11 @@ class TestCwlWorkflow(TestCase):
         find_flavor_method = mock_client().flavors.find
         find_flavor_method.assert_called_with(name='m1.GIANT')
 
+    @mock.patch('lando.server.cloudservice.sleep')
     @mock.patch('keystoneauth1.loading.get_plugin_loader')
     @mock.patch('keystoneauth1.session.Session')
     @mock.patch('novaclient.client.Client')
-    def test_that_no_flavor_chooses_default(self, mock_client, mock_session, mock_get_plugin_loader):
+    def test_that_no_flavor_chooses_default(self, mock_client, mock_session, mock_get_plugin_loader, mock_sleep):
         config = mock.MagicMock()
         config.vm_settings.default_favor_name = 'm1.xbig'
         cloud_service = CloudService(config, project_name='bespin_user1')
@@ -27,5 +30,69 @@ class TestCwlWorkflow(TestCase):
         find_flavor_method = mock_client().flavors.find
         find_flavor_method.assert_called_with(name='m1.xbig')
 
+    @mock.patch('lando.server.cloudservice.sleep')
+    @mock.patch('keystoneauth1.loading.get_plugin_loader')
+    @mock.patch('keystoneauth1.session.Session')
+    @mock.patch('novaclient.client.Client')
+    def test_launch_instance_no_floating_ip(self, mock_client, mock_session, mock_get_plugin_loader, mock_sleep):
+        config = mock.MagicMock()
+        config.vm_settings.allocate_floating_ips = False
+        config.vm_settings.default_favor_name = 'm1.large'
+        cloud_service = CloudService(config, project_name='bespin_user1')
+        instance, ip_address = cloud_service.launch_instance(server_name="worker1", flavor_name=None, script_contents="")
+        self.assertEqual(None, ip_address)
+        mock_client().servers.create.assert_called()
+        mock_client().floating_ips.create.assert_not_called()
 
+    @mock.patch('lando.server.cloudservice.sleep')
+    @mock.patch('keystoneauth1.loading.get_plugin_loader')
+    @mock.patch('keystoneauth1.session.Session')
+    @mock.patch('novaclient.client.Client')
+    def test_launch_instance_with_floating_ip(self, mock_client, mock_session, mock_get_plugin_loader, mock_sleep):
+        config = mock.MagicMock()
+        config.vm_settings.allocate_floating_ips = True
+        config.vm_settings.default_favor_name = 'm1.large'
+        cloud_service = CloudService(config, project_name='bespin_user1')
+        instance, ip_address = cloud_service.launch_instance(server_name="worker1", flavor_name=None,
+                                                             script_contents="")
+        self.assertNotEqual(None, ip_address)
+        mock_client().servers.create.assert_called()
+        mock_client().floating_ips.create.assert_called()
 
+    @mock.patch('keystoneauth1.loading.get_plugin_loader')
+    @mock.patch('keystoneauth1.session.Session')
+    @mock.patch('novaclient.client.Client')
+    def test_terminate_instance_no_floating_ip(self, mock_client, mock_session, mock_get_plugin_loader):
+        config = mock.MagicMock()
+        config.vm_settings.allocate_floating_ips = False
+        config.vm_settings.default_favor_name = 'm1.large'
+        cloud_service = CloudService(config, project_name='bespin_user1')
+        cloud_service.terminate_instance(server_name='worker1')
+        mock_client().servers.delete.assert_called()
+        mock_client().floating_ips.find.assert_not_called()
+
+    @mock.patch('keystoneauth1.loading.get_plugin_loader')
+    @mock.patch('keystoneauth1.session.Session')
+    @mock.patch('novaclient.client.Client')
+    def test_terminate_instance_with_floating_ip(self, mock_client, mock_session, mock_get_plugin_loader):
+        config = mock.MagicMock()
+        config.vm_settings.allocate_floating_ips = True
+        config.vm_settings.default_favor_name = 'm1.large'
+        cloud_service = CloudService(config, project_name='bespin_user1')
+        cloud_service.terminate_instance(server_name='worker1')
+        mock_client().servers.delete.assert_called()
+        mock_client().floating_ips.find.assert_called()
+        mock_client().floating_ips.find().delete.assert_called()
+
+    @mock.patch('keystoneauth1.loading.get_plugin_loader')
+    @mock.patch('keystoneauth1.session.Session')
+    @mock.patch('novaclient.client.Client')
+    def test_terminate_instance_with_missing_floating_ip(self, mock_client, mock_session, mock_get_plugin_loader):
+        mock_client().floating_ips.find.side_effect = novaclient.exceptions.NotFound(404)
+        config = mock.MagicMock()
+        config.vm_settings.allocate_floating_ips = True
+        config.vm_settings.default_favor_name = 'm1.large'
+        cloud_service = CloudService(config, project_name='bespin_user1')
+        cloud_service.terminate_instance(server_name='worker1')
+        mock_client().servers.delete.assert_called()
+        mock_client().floating_ips.find.assert_called()
diff --git a/lando/server/tests/test_config.py b/lando/server/tests/test_config.py
index 80f9c4c..4342ea9 100644
--- a/lando/server/tests/test_config.py
+++ b/lando/server/tests/test_config.py
@@ -19,6 +19,7 @@ vm_settings:
   network_name: selfservice
   floating_ip_pool_name: ext-net
   default_favor_name: m1.small
+{}
 
 cloud_settings:
   auth_url: http://10.109.252.9:5000/v3
@@ -36,7 +37,7 @@ bespin_api:
 
 class TestServerConfig(TestCase):
     def test_good_config(self):
-        filename = write_temp_return_filename(GOOD_CONFIG)
+        filename = write_temp_return_filename(GOOD_CONFIG.format(""))
         config = ServerConfig(filename)
         os.unlink(filename)
         self.assertEqual(False, config.fake_cloud_service)
@@ -46,6 +47,8 @@ class TestServerConfig(TestCase):
 
         self.assertEqual('lando_worker', config.vm_settings.worker_image_name)
         self.assertEqual('jpb67', config.vm_settings.ssh_key_name)
+        #  by default allocate_floating_ips is off
+        self.assertEqual(False, config.vm_settings.allocate_floating_ips)
 
         self.assertEqual("http://10.109.252.9:5000/v3", config.cloud_settings.auth_url)
         self.assertEqual("jpb67", config.cloud_settings.username)
@@ -53,8 +56,22 @@ class TestServerConfig(TestCase):
         self.assertEqual("http://localhost:8000/api", config.bespin_api_settings.url)
         self.assertEqual("10498124091240e", config.bespin_api_settings.token)
 
+    def test_allocate_floating_ip_true(self):
+        line = "  allocate_floating_ips: true"
+        filename = write_temp_return_filename(GOOD_CONFIG.format(line))
+        config = ServerConfig(filename)
+        os.unlink(filename)
+        self.assertEqual(True, config.vm_settings.allocate_floating_ips)
+
+    def test_allocate_floating_ip_false(self):
+        line = "  allocate_floating_ips: false"
+        filename = write_temp_return_filename(GOOD_CONFIG.format(line))
+        config = ServerConfig(filename)
+        os.unlink(filename)
+        self.assertEqual(False, config.vm_settings.allocate_floating_ips)
+
     def test_good_config_with_fake_cloud_service(self):
-        config_data = GOOD_CONFIG + "\nfake_cloud_service: True"
+        config_data = GOOD_CONFIG.format("") + "\nfake_cloud_service: True"
         filename = write_temp_return_filename(config_data)
         config = ServerConfig(filename)
         os.unlink(filename)
@@ -73,7 +90,7 @@ class TestServerConfig(TestCase):
         os.unlink(filename)
 
     def test_make_worker_config_yml(self):
-        filename = write_temp_return_filename(GOOD_CONFIG)
+        filename = write_temp_return_filename(GOOD_CONFIG.format(""))
         config = ServerConfig(filename)
         os.unlink(filename)
         expected = """