Merge "Ensuring all instances must have ports/network. Fixing the addition of security groups during server instantiation."

3 files changed, 74 insertions, 58 deletions

diff --git a/snaps/openstack/create_instance.py b/snaps/openstack/create_instance.py
index caddc05..620483b 100644
--- a/snaps/openstack/create_instance.py
+++ b/snaps/openstack/create_instance.py
@@ -131,9 +131,10 @@ class OpenStackVmInstance:
                 image=image,
                 nics=nics,
                 key_name=keypair_name,
-                security_groups=list(self.instance_settings.security_group_names),
+                security_groups=self.instance_settings.security_group_names,
                 userdata=self.instance_settings.userdata,
                 availability_zone=self.instance_settings.availability_zone)
+
         else:
             raise Exception('Cannot create instance, image cannot be located with name ' + self.image_settings.name)
 
@@ -142,6 +143,11 @@ class OpenStackVmInstance:
         if block:
             self.vm_active(block=True)
 
+        # TODO - the call above should add security groups. The return object shows they exist but the association
+        # had never been made by OpenStack. This call is here to ensure they have been added
+        for sec_grp_name in self.instance_settings.security_group_names:
+            nova_utils.add_security_group(self.__nova, self.__vm, sec_grp_name)
+
         self.__apply_floating_ips()
 
     def __apply_floating_ips(self):
@@ -594,14 +600,14 @@ class OpenStackVmInstance:
         self.vm_active(block=True)
 
         if not security_group:
-            logger.warn('Security group object is None, cannot add')
+            logger.warn('Security group object is None, cannot remove')
             return False
 
         try:
-            nova_utils.remove_security_group(self.__nova, self.get_vm_inst(), security_group['security_group']['name'])
+            nova_utils.remove_security_group(self.__nova, self.get_vm_inst(), security_group)
             return True
         except NotFound as e:
-            logger.warn('Security group not added - ' + e.message)
+            logger.warn('Security group not removed - ' + e.message)
             return False
 
 
@@ -618,7 +624,7 @@ class VmInstanceSettings:
                        member's the key and overrides any of the other parameters.
         :param name: the name of the VM
         :param flavor: the VM's flavor
-        :param port_settings: the port configuration settings
+        :param port_settings: the port configuration settings (required)
         :param security_group_names: a set of names of the security groups to add to the VM
         :param floating_ip_settings: the floating IP configuration settings
         :param sudo_user: the sudo user of the VM that will override the instance_settings.image_user when trying to
@@ -698,6 +704,9 @@ class VmInstanceSettings:
         if not self.name or not self.flavor:
             raise Exception('Instance configuration requires the attributes: name, flavor')
 
+        if len(self.port_settings) == 0:
+            raise Exception('Instance configuration requires port settings (aka. NICS)')
+
 
 class FloatingIpSettings:
     """
diff --git a/snaps/openstack/tests/create_instance_tests.py b/snaps/openstack/tests/create_instance_tests.py
index ccdf45e..64053ff 100644
--- a/snaps/openstack/tests/create_instance_tests.py
+++ b/snaps/openstack/tests/create_instance_tests.py
@@ -59,10 +59,21 @@ class VmInstanceSettingsUnitTests(unittest.TestCase):
             VmInstanceSettings(config={'name': 'foo'})
 
     def test_name_flavor_only(self):
-        settings = VmInstanceSettings(name='foo', flavor='bar')
+        with self.assertRaises(Exception):
+            VmInstanceSettings(name='foo', flavor='bar')
+
+    def test_config_with_name_flavor_only(self):
+        with self.assertRaises(Exception):
+            VmInstanceSettings(config={'name': 'foo', 'flavor': 'bar'})
+
+    def test_name_flavor_port_only(self):
+        port_settings = PortSettings(name='foo-port', network_name='bar-net')
+        settings = VmInstanceSettings(name='foo', flavor='bar', port_settings=[port_settings])
         self.assertEquals('foo', settings.name)
         self.assertEquals('bar', settings.flavor)
-        self.assertEquals(0, len(settings.port_settings))
+        self.assertEquals(1, len(settings.port_settings))
+        self.assertEquals('foo-port', settings.port_settings[0].name)
+        self.assertEquals('bar-net', settings.port_settings[0].network_name)
         self.assertEquals(0, len(settings.security_group_names))
         self.assertEquals(0, len(settings.floating_ip_settings))
         self.assertIsNone(settings.sudo_user)
@@ -71,11 +82,14 @@ class VmInstanceSettingsUnitTests(unittest.TestCase):
         self.assertEquals(180, settings.ssh_connect_timeout)
         self.assertIsNone(settings.availability_zone)
 
-    def test_config_with_name_flavor_only(self):
-        settings = VmInstanceSettings(config={'name': 'foo', 'flavor': 'bar'})
+    def test_config_with_name_flavor_port_only(self):
+        port_settings = PortSettings(name='foo-port', network_name='bar-net')
+        settings = VmInstanceSettings(config={'name': 'foo', 'flavor': 'bar', 'ports': [port_settings]})
         self.assertEquals('foo', settings.name)
         self.assertEquals('bar', settings.flavor)
-        self.assertEquals(0, len(settings.port_settings))
+        self.assertEquals(1, len(settings.port_settings))
+        self.assertEquals('foo-port', settings.port_settings[0].name)
+        self.assertEquals('bar-net', settings.port_settings[0].network_name)
         self.assertEquals(0, len(settings.security_group_names))
         self.assertEquals(0, len(settings.floating_ip_settings))
         self.assertIsNone(settings.sudo_user)
@@ -347,20 +361,35 @@ class CreateInstanceSimpleTests(OSIntegrationTestCase):
         self.nova = nova_utils.nova_client(self.os_creds)
         self.os_image_settings = openstack_tests.cirros_url_image(name=guid + '-image')
 
+        net_config = openstack_tests.get_priv_net_config(
+            net_name=guid + '-pub-net', subnet_name=guid + '-pub-subnet',
+            router_name=guid + '-pub-router', external_net=self.ext_net_name)
+
         # Initialize for tearDown()
         self.image_creator = None
         self.flavor_creator = None
+
+        self.net_creator = None
         self.inst_creator = None
 
         try:
             # Create Image
             self.image_creator = OpenStackImage(self.os_creds, self.os_image_settings)
             self.image_creator.create()
+
             # Create Flavor
             self.flavor_creator = OpenStackFlavor(
                 self.admin_os_creds,
                 FlavorSettings(name=guid + '-flavor-name', ram=2048, disk=10, vcpus=2))
             self.flavor_creator.create()
+
+            # Create Network
+            self.network_creator = OpenStackNetwork(self.os_creds, net_config.network_settings)
+            self.network_creator.create()
+
+            self.port_settings = PortSettings(name=guid + '-port',
+                                              network_name=net_config.network_settings.name)
+
         except Exception as e:
             self.tearDown()
             raise e
@@ -381,6 +410,12 @@ class CreateInstanceSimpleTests(OSIntegrationTestCase):
             except Exception as e:
                 logger.error('Unexpected exception cleaning flavor with message - ' + e.message)
 
+        if self.net_creator:
+            try:
+                self.net_creator.clean()
+            except Exception as e:
+                logger.error('Unexpected exception cleaning network with message - ' + e.message)
+
         if self.image_creator:
             try:
                 self.image_creator.clean()
@@ -393,7 +428,8 @@ class CreateInstanceSimpleTests(OSIntegrationTestCase):
         """
         Tests the creation of an OpenStack instance with a single port with a static IP without a Floating IP.
         """
-        instance_settings = VmInstanceSettings(name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name)
+        instance_settings = VmInstanceSettings(name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name,
+                                               port_settings=[self.port_settings])
 
         self.inst_creator = OpenStackVmInstance(
             self.os_creds, instance_settings, self.image_creator.image_settings)
@@ -1204,21 +1240,17 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
         self.nova = nova_utils.nova_client(self.os_creds)
         self.os_image_settings = openstack_tests.cirros_url_image(name=self.guid + '-image')
 
-        self.keypair_priv_filepath = 'tmp/' + self.guid
-        self.keypair_pub_filepath = self.keypair_priv_filepath + '.pub'
-        self.keypair_name = self.guid + '-kp'
         self.vm_inst_name = self.guid + '-inst'
         self.port_1_name = self.guid + 'port-1'
         self.port_2_name = self.guid + 'port-2'
         self.floating_ip_name = self.guid + 'fip1'
 
-        self.pub_net_config = openstack_tests.get_pub_net_config(
+        net_config = openstack_tests.get_priv_net_config(
             net_name=self.guid + '-pub-net', subnet_name=self.guid + '-pub-subnet',
             router_name=self.guid + '-pub-router', external_net=self.ext_net_name)
 
         # Initialize for tearDown()
         self.image_creator = None
-        self.keypair_creator = None
         self.flavor_creator = None
         self.network_creator = None
         self.router_creator = None
@@ -1231,24 +1263,17 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
             self.image_creator.create()
 
             # Create Network
-            self.network_creator = OpenStackNetwork(self.os_creds, self.pub_net_config.network_settings)
+            self.network_creator = OpenStackNetwork(self.os_creds, net_config.network_settings)
             self.network_creator.create()
 
-            # Create Router
-            self.router_creator = OpenStackRouter(self.os_creds, self.pub_net_config.router_settings)
-            self.router_creator.create()
-
             # Create Flavor
             self.flavor_creator = OpenStackFlavor(
                 self.admin_os_creds,
                 FlavorSettings(name=self.guid + '-flavor-name', ram=2048, disk=10, vcpus=2))
             self.flavor_creator.create()
 
-            self.keypair_creator = OpenStackKeypair(
-                self.os_creds, KeypairSettings(
-                    name=self.keypair_name, public_filepath=self.keypair_pub_filepath,
-                    private_filepath=self.keypair_priv_filepath))
-            self.keypair_creator.create()
+            self.port_settings = PortSettings(name=self.guid + '-port',
+                                              network_name=net_config.network_settings.name)
         except Exception as e:
             self.tearDown()
             raise e
@@ -1269,30 +1294,12 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
             except Exception as e:
                 logger.error('Unexpected exception cleaning security group with message - ' + e.message)
 
-        if self.keypair_creator:
-            try:
-                self.keypair_creator.clean()
-            except Exception as e:
-                logger.error('Unexpected exception cleaning keypair with message - ' + e.message)
-
-        if os.path.isfile(self.keypair_pub_filepath):
-            os.remove(self.keypair_pub_filepath)
-
-        if os.path.isfile(self.keypair_priv_filepath):
-            os.remove(self.keypair_priv_filepath)
-
         if self.flavor_creator:
             try:
                 self.flavor_creator.clean()
             except Exception as e:
                 logger.error('Unexpected exception cleaning flavor with message - ' + e.message)
 
-        if self.router_creator:
-            try:
-                self.router_creator.clean()
-            except Exception as e:
-                logger.error('Unexpected exception cleaning router with message - ' + e.message)
-
         if self.network_creator:
             try:
                 self.network_creator.clean()
@@ -1313,9 +1320,9 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
         """
         # Create instance
         instance_settings = VmInstanceSettings(
-            name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name)
+            name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name, port_settings=[self.port_settings])
         self.inst_creator = OpenStackVmInstance(self.os_creds, instance_settings, self.image_creator.image_settings)
-        vm_inst = self.inst_creator.create()
+        vm_inst = self.inst_creator.create(block=True)
         self.assertIsNotNone(vm_inst)
 
         # Create security group object to add to instance
@@ -1339,9 +1346,9 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
         """
         # Create instance
         instance_settings = VmInstanceSettings(
-            name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name)
+            name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name, port_settings=[self.port_settings])
         self.inst_creator = OpenStackVmInstance(self.os_creds, instance_settings, self.image_creator.image_settings)
-        vm_inst = self.inst_creator.create()
+        vm_inst = self.inst_creator.create(block=True)
         self.assertIsNotNone(vm_inst)
 
         # Create security group object to add to instance
@@ -1373,9 +1380,9 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
         # Create instance
         instance_settings = VmInstanceSettings(
             name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name,
-            security_group_names=[sec_grp_settings.name])
+            security_group_names=[sec_grp_settings.name], port_settings=[self.port_settings])
         self.inst_creator = OpenStackVmInstance(self.os_creds, instance_settings, self.image_creator.image_settings)
-        vm_inst = self.inst_creator.create()
+        vm_inst = self.inst_creator.create(block=True)
         self.assertIsNotNone(vm_inst)
 
         # Check that group has been added
@@ -1399,13 +1406,13 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
 
         # Create instance
         instance_settings = VmInstanceSettings(
-            name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name)
+            name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name, port_settings=[self.port_settings])
         self.inst_creator = OpenStackVmInstance(self.os_creds, instance_settings, self.image_creator.image_settings)
-        vm_inst = self.inst_creator.create()
+        vm_inst = self.inst_creator.create(block=True)
         self.assertIsNotNone(vm_inst)
 
         # Check that group has been added
-        self.assertFalse(inst_has_sec_grp(vm_inst, sec_grp_settings.name))
+        self.assertFalse(inst_has_sec_grp(self.inst_creator.get_vm_inst(), sec_grp_settings.name))
 
         # Add security group to instance after activated
         self.assertFalse(self.inst_creator.remove_security_group(sec_grp))
@@ -1426,13 +1433,13 @@ class InstanceSecurityGroupTests(OSIntegrationTestCase):
         # Create instance
         instance_settings = VmInstanceSettings(
             name=self.vm_inst_name, flavor=self.flavor_creator.flavor_settings.name,
-            security_group_names=[sec_grp_settings.name])
+            security_group_names=[sec_grp_settings.name], port_settings=[self.port_settings])
         self.inst_creator = OpenStackVmInstance(self.os_creds, instance_settings, self.image_creator.image_settings)
-        vm_inst = self.inst_creator.create()
+        vm_inst = self.inst_creator.create(block=True)
         self.assertIsNotNone(vm_inst)
 
         # Check that group has been added
-        self.assertTrue(inst_has_sec_grp(vm_inst, sec_grp_settings.name))
+        self.assertTrue(inst_has_sec_grp(self.inst_creator.get_vm_inst(), sec_grp_settings.name))
 
         # Add security group to instance after activated
         self.assertTrue(self.inst_creator.add_security_group(sec_grp))
diff --git a/snaps/openstack/utils/nova_utils.py b/snaps/openstack/utils/nova_utils.py
index 9d0f70f..e7428ed 100644
--- a/snaps/openstack/utils/nova_utils.py
+++ b/snaps/openstack/utils/nova_utils.py
@@ -269,7 +269,7 @@ def add_security_group(nova, vm, security_group_name):
     :param vm: the OpenStack server object (VM) to alter
     :param security_group_name: the name of the security group to add
     """
-    nova.servers.add_security_group(vm.id, security_group_name)
+    nova.servers.add_security_group(str(vm.id), security_group_name)
 
 
 def remove_security_group(nova, vm, security_group):
@@ -279,4 +279,4 @@ def remove_security_group(nova, vm, security_group):
     :param vm: the OpenStack server object (VM) to alter
     :param security_group: the OpenStack security group object to add
     """
-    nova.servers.remove_security_group(vm.id, security_group)
+    nova.servers.remove_security_group(str(vm.id), security_group['security_group']['name'])