Merge ~newell-jensen/maas:lp1788756 into maas:master

[Newell Jensen]

LP: #1788756 -- Only raise no network error for network attached or non-specified interfaces.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1788756 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 19d0b693523255c2702859ba62c24269acfdd933

review: Approve

[Mike Pontillo (mpontillo)]

Looks good; one question inline below that I won't block you on. (I think the same issue happens in the block devices loop, too.)

review: Approve

[Newell Jensen (newell-jensen)]

> Looks good; one question inline below that I won't block you on. (I think the
> same issue happens in the block devices loop, too.)

All the block devices are created first (see top part of the create_domain method in provisioningserver.drivers.pod.virsh.py) and then attached after the domain has been defined within virsh. Good question though.

diff --git a/src/provisioningserver/drivers/pod/tests/test_virsh.py b/src/provisioningserver/drivers/pod/tests/test_virsh.py
index ff616c7..f6137c8 100644
--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
@@ -1256,11 +1256,12 @@ class TestVirshSSH(MAASTestCase):
         conn = self.configure_virshssh('')
         domain = factory.make_name('domain')
         network = factory.make_name('network')
+        self.patch(virsh.VirshSSH, "get_best_network").return_value = network
         mock_run = self.patch(virsh.VirshSSH, "run")
         fake_mac = factory.make_mac_address()
         interface = RequestedMachineInterface()
         self.patch(virsh, 'generate_mac_address').return_value = fake_mac
-        conn.attach_interface(interface, domain, network)
+        conn.attach_interface(interface, domain)
         self.assertThat(mock_run, MockCalledOnceWith([
             'attach-interface', domain, 'network', network,
             '--mac', fake_mac,
@@ -1270,12 +1271,13 @@ class TestVirshSSH(MAASTestCase):
         conn = self.configure_virshssh('')
         domain = factory.make_name('domain')
         network = factory.make_name('network')
+        self.patch(virsh.VirshSSH, "get_best_network").return_value = network
         mock_run = self.patch(virsh.VirshSSH, "run")
         fake_mac = factory.make_mac_address()
         interface = RequestedMachineInterface(
             attach_name=network, attach_type='network')
         self.patch(virsh, 'generate_mac_address').return_value = fake_mac
-        conn.attach_interface(interface, domain, network)
+        conn.attach_interface(interface, domain)
         self.assertThat(mock_run, MockCalledOnceWith([
             'attach-interface', domain, 'network', network,
             '--mac', fake_mac,
@@ -1284,7 +1286,6 @@ class TestVirshSSH(MAASTestCase):
     def test_attach_interface_attaches_macvlan(self):
         conn = self.configure_virshssh('')
         domain = factory.make_name('domain')
-        network = factory.make_name('network')
         mock_run = self.patch(virsh.VirshSSH, "run")
         fake_mac = factory.make_mac_address()
         interface = RequestedMachineInterface(
@@ -1296,7 +1297,7 @@ class TestVirshSSH(MAASTestCase):
         tmpfile = NamedTemporaryFile.return_value
         tmpfile.__enter__.return_value = tmpfile
         tmpfile.name = factory.make_name("filename")
-        conn.attach_interface(interface, domain, network)
+        conn.attach_interface(interface, domain)
 
         device_params = {
             'mac_address': fake_mac,
@@ -1318,7 +1319,6 @@ class TestVirshSSH(MAASTestCase):
     def test_attach_interface_attaches_bridge(self):
         conn = self.configure_virshssh('')
         domain = factory.make_name('domain')
-        network = factory.make_name('network')
         mock_run = self.patch(virsh.VirshSSH, "run")
         fake_mac = factory.make_mac_address()
         interface = RequestedMachineInterface(
@@ -1330,7 +1330,7 @@ class TestVirshSSH(MAASTestCase):
         tmpfile = NamedTemporaryFile.return_value
         tmpfile.__enter__.return_value = tmpfile
         tmpfile.name = factory.make_name("filename")
-        conn.attach_interface(interface, domain, network)
+        conn.attach_interface(interface, domain)
 
         device_params = {
             'mac_address': fake_mac,
@@ -1434,9 +1434,38 @@ class TestVirshSSH(MAASTestCase):
         request = make_requested_machine()
         self.patch(virsh.VirshSSH, "create_local_volume").return_value = None
         error = self.assertRaises(
-            PodInvalidResources, conn.create_domain, request, )
+            PodInvalidResources, conn.create_domain, request)
         self.assertEqual("not enough space for disk 0.", str(error))
 
+    def test_create_domain_handles_no_network_for_requested_interface(self):
+        conn = self.configure_virshssh('')
+        request = make_requested_machine()
+        request.block_devices = request.block_devices[:1]
+        request.interfaces = request.interfaces[:1]
+        disk_info = (factory.make_name('pool'), factory.make_name('vol'))
+        domain_params = {
+            "type": "kvm",
+            "emulator": "/usr/bin/qemu-system-x86_64",
+        }
+        self.patch(
+            virsh.VirshSSH, "create_local_volume").return_value = disk_info
+        self.patch(virsh.VirshSSH, "get_domain_capabilities").return_value = (
+            domain_params)
+        mock_uuid = self.patch(virsh_module, "uuid4")
+        mock_uuid.return_value = str(uuid4())
+        domain_params['name'] = request.hostname
+        domain_params['uuid'] = mock_uuid.return_value
+        domain_params['arch'] = ARCH_FIX_REVERSE[request.architecture]
+        domain_params['cores'] = str(request.cores)
+        domain_params['memory'] = str(request.memory)
+        NamedTemporaryFile = self.patch(virsh_module, "NamedTemporaryFile")
+        tmpfile = NamedTemporaryFile.return_value
+        tmpfile.__enter__.return_value = tmpfile
+        tmpfile.name = factory.make_name("filename")
+        self.patch(virsh.VirshSSH, "run")
+        self.patch(virsh.VirshSSH, "get_network_list").return_value = []
+        self.assertRaises(PodInvalidResources, conn.create_domain, request)
+
     def test_create_domain_calls_correct_methods_with_amd64_arch(self):
         conn = self.configure_virshssh('')
         request = make_requested_machine()
@@ -1462,7 +1491,6 @@ class TestVirshSSH(MAASTestCase):
         tmpfile = NamedTemporaryFile.return_value
         tmpfile.__enter__.return_value = tmpfile
         tmpfile.name = factory.make_name("filename")
-        self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1486,7 +1514,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
@@ -1521,7 +1549,6 @@ class TestVirshSSH(MAASTestCase):
         tmpfile = NamedTemporaryFile.return_value
         tmpfile.__enter__.return_value = tmpfile
         tmpfile.name = factory.make_name("filename")
-        self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1545,7 +1572,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
@@ -1580,7 +1607,6 @@ class TestVirshSSH(MAASTestCase):
         tmpfile = NamedTemporaryFile.return_value
         tmpfile.__enter__.return_value = tmpfile
         tmpfile.name = factory.make_name("filename")
-        self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1604,7 +1630,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
@@ -1639,7 +1665,6 @@ class TestVirshSSH(MAASTestCase):
         tmpfile = NamedTemporaryFile.return_value
         tmpfile.__enter__.return_value = tmpfile
         tmpfile.name = factory.make_name("filename")
-        self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
         mock_run = self.patch(virsh.VirshSSH, "run")
         mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
         mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1663,7 +1688,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY))
         self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
index f04fb9c..629c2d8 100644
--- a/src/provisioningserver/drivers/pod/virsh.py
+++ b/src/provisioningserver/drivers/pod/virsh.py
@@ -871,13 +871,14 @@ class VirshSSH(pexpect.spawn):
 
         return networks[0]
 
-    def attach_interface(self, interface, domain, network):
+    def attach_interface(self, interface, domain):
         """Attach new network interface on `domain` to `network`."""
         mac = generate_mac_address()
         # If attachment type is not specified, default to network.
         if interface.attach_type in (None, InterfaceAttachType.NETWORK):
             # Set the network if we are explicity attaching a network
             # specified by the user.
+            network = self.get_best_network()
             if interface.attach_type is not None:
                 network = interface.attach_name
             return self.run([
@@ -977,11 +978,7 @@ class VirshSSH(pexpect.spawn):
         return "vd" + name
 
     def create_domain(self, request, default_pool=None):
-        """Create a domain based on the `request` with hostname.
-
-        For now this just uses `get_best_network` to connect the interfaces
-        of the domain to the network.
-        """
+        """Create a domain based on the `request` with hostname."""
         # Create all the block devices first. If cannot complete successfully
         # then fail early. The driver currently doesn't do any tag matching
         # for block devices, and is not really required for Virsh.
@@ -1030,17 +1027,23 @@ class VirshSSH(pexpect.spawn):
             f.flush()
             self.run(['define', f.name])
 
+        # Attach the requested interfaces.
+        for interface in request.interfaces:
+            try:
+                self.attach_interface(interface, request.hostname)
+            except PodInvalidResources as error:
+                # Delete the defined domain in virsh.
+                self.run(['destroy', request.hostname])
+                self.run(['undefine', request.hostname])
+                # Re-raise the exception.
+                raise error
+
         # Attach the created disks in order.
         for idx, (pool, volume) in enumerate(created_disks):
             block_name = self.get_block_name_from_idx(idx)
             self.attach_local_volume(
                 request.hostname, pool, volume, block_name)
 
-        # Attach new interfaces to the best possible network.
-        best_network = self.get_best_network()
-        for interface in request.interfaces:
-            self.attach_interface(interface, request.hostname, best_network)
-
         # Set machine to autostart.
         self.set_machine_autostart(request.hostname)