Merge ~newell-jensen/maas:virsh-pod-driver-networking into maas:master

[Newell Jensen]

Update attach_interface method in virsh pod driver to deal with macvlan, bridge, and explicit network interface attachments. This will allow VMs to be composed with these different interface types.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-driver-networking lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: c1077fe6c86f788dbba9cad945f29dd0c07dd100

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-driver-networking lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 04fffe008b2e438f85f15dd1a167cce7a4ead0e2

review: Approve

[Mike Pontillo (mpontillo)]

I'm going to E2E test this before I comment further.

One thing I noticed though: I'm not sure bridge attachment is going to belong in the same code path as network attachment (the current default).

That is, if I specify a bridge to attach to, I'm going to provide a bridge name that I'd like the VM to be attached to. That might be something like 'br0'. Whereas if I'm attaching to a /network/, in libvirt terms, that's going to be something like 'default' or 'maas', which may in turn attach to a bridge at the end of the operation (such as 'virbr0').

So I think we need an XML-attachment path similar to macvlan for the bridge use case.

review: Needs Information

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-driver-networking lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 81db38278b4522f6372600f22fb26f51f4454525

review: Approve

a2ad45a... by Newell Jensen on 2018-07-10

Clean up template XMLs for macvlans and bridges.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-driver-networking lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: a2ad45ac531df66fdf64da15f866fe5c011bce79

review: Approve

a790e0a... by Newell Jensen on 2018-07-10

Add network attachment type. This is the default if nothing is specified but we should also be allowing network attachment types to be explicitly defined.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-pod-driver-networking lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: a790e0acc169c9688fdd200d77b62cb9dacf0690

review: Approve

17065cc... by Newell Jensen on 2018-07-11

Review fix.

[Mike Pontillo (mpontillo)]

Thanks for the fixes. Let's land this!

review: Approve

diff --git a/src/provisioningserver/drivers/pod/__init__.py b/src/provisioningserver/drivers/pod/__init__.py
index ff413ce..602b893 100644
--- a/src/provisioningserver/drivers/pod/__init__.py
+++ b/src/provisioningserver/drivers/pod/__init__.py
@@ -296,11 +296,11 @@ class RequestedMachineBlockDevice(AttrHelperMixin):
 @attr.s
 class RequestedMachineInterface(AttrHelperMixin):
     """Requested machine interface information."""
-    attach_ifname = attr.ib(
+    attach_name = attr.ib(
         converter=converter_obj(str, optional=True), default=None)
     attach_type = attr.ib(
         converter=converter_obj(str, optional=True), default=None)
-    attach_type_mode = attr.ib(
+    attach_options = attr.ib(
         converter=converter_obj(str, optional=True), default=None)
 
 
diff --git a/src/provisioningserver/drivers/pod/tests/test_base.py b/src/provisioningserver/drivers/pod/tests/test_base.py
index b38b42b..866b7eb 100644
--- a/src/provisioningserver/drivers/pod/tests/test_base.py
+++ b/src/provisioningserver/drivers/pod/tests/test_base.py
@@ -619,12 +619,12 @@ class TestRequestClasses(MAASTestCase):
             "memory": Equals(memory),
             "interfaces": MatchesListwise([
                 MatchesDict({
-                    "attach_ifname": Equals(
-                        interface.attach_ifname),
+                    "attach_name": Equals(
+                        interface.attach_name),
                     "attach_type": Equals(
                         interface.attach_type),
-                    "attach_type_mode": Equals(
-                        interface.attach_type_mode),
+                    "attach_options": Equals(
+                        interface.attach_options),
                 })
                 for interface in interfaces
             ]),
diff --git a/src/provisioningserver/drivers/pod/tests/test_virsh.py b/src/provisioningserver/drivers/pod/tests/test_virsh.py
index a46f0a0..fe4a766 100644
--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
@@ -39,6 +39,8 @@ from provisioningserver.drivers.pod.virsh import (
     ARCH_FIX_REVERSE,
     DOM_TEMPLATE_AMD64,
     DOM_TEMPLATE_ARM64,
+    DOM_TEMPLATE_BRIDGE_INTERFACE,
+    DOM_TEMPLATE_MACVLAN_INTERFACE,
     DOM_TEMPLATE_PPC64,
     DOM_TEMPLATE_S390X,
     VirshPodDriver,
@@ -1236,19 +1238,102 @@ class TestVirshSSH(MAASTestCase):
         self.patch(virsh.VirshSSH, "get_network_list").return_value = []
         self.assertRaises(PodInvalidResources, conn.get_best_network)
 
-    def test_attach_interface(self):
+    def test_attach_interface_defaults_to_network_attachment(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()
         self.patch(virsh, 'generate_mac_address').return_value = fake_mac
-        conn.attach_interface(domain, network)
+        conn.attach_interface(interface, domain, network)
         self.assertThat(mock_run, MockCalledOnceWith([
             'attach-interface', domain, 'network', network,
             '--mac', fake_mac,
             '--model', 'virtio', '--config']))
 
+    def test_attach_interface_calls_attaches_network(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(
+            attach_name=network, attach_type='network')
+        self.patch(virsh, 'generate_mac_address').return_value = fake_mac
+        conn.attach_interface(interface, domain, network)
+        self.assertThat(mock_run, MockCalledOnceWith([
+            'attach-interface', domain, 'network', network,
+            '--mac', fake_mac,
+            '--model', 'virtio', '--config']))
+
+    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(
+            attach_name=factory.make_name('name'),
+            attach_type='macvlan', attach_options=random.choice([
+                'private', 'vepa', 'bridge', 'passthru']))
+        self.patch(virsh, 'generate_mac_address').return_value = fake_mac
+        NamedTemporaryFile = self.patch(virsh_module, "NamedTemporaryFile")
+        tmpfile = NamedTemporaryFile.return_value
+        tmpfile.__enter__.return_value = tmpfile
+        tmpfile.name = factory.make_name("filename")
+        conn.attach_interface(interface, domain, network)
+
+        device_params = {
+            'mac_address': fake_mac,
+            'attach_name': interface.attach_name,
+            'attach_options': interface.attach_options,
+        }
+        self.assertThat(mock_run, MockCalledOnceWith([
+            'attach-device', domain, ANY, '--config']))
+        self.assertThat(
+            NamedTemporaryFile, MockCalledOnceWith())
+        self.assertThat(tmpfile.__enter__, MockCalledOnceWith())
+        self.assertThat(tmpfile.write, MockCallsMatch(
+            call(DOM_TEMPLATE_MACVLAN_INTERFACE.format(
+                **device_params).encode('utf-8')),
+            call(b'\n')))
+        self.assertThat(tmpfile.flush, MockCalledOnceWith())
+        self.assertThat(tmpfile.__exit__, MockCalledOnceWith(None, None, None))
+
+    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(
+            attach_name=factory.make_name('ifname'),
+            attach_type='bridge', attach_options=random.choice([
+                'private', 'vepa', 'bridge', 'passthru']))
+        self.patch(virsh, 'generate_mac_address').return_value = fake_mac
+        NamedTemporaryFile = self.patch(virsh_module, "NamedTemporaryFile")
+        tmpfile = NamedTemporaryFile.return_value
+        tmpfile.__enter__.return_value = tmpfile
+        tmpfile.name = factory.make_name("filename")
+        conn.attach_interface(interface, domain, network)
+
+        device_params = {
+            'mac_address': fake_mac,
+            'attach_name': interface.attach_name,
+        }
+        self.assertThat(mock_run, MockCalledOnceWith([
+            'attach-device', domain, ANY, '--config']))
+        self.assertThat(
+            NamedTemporaryFile, MockCalledOnceWith())
+        self.assertThat(tmpfile.__enter__, MockCalledOnceWith())
+        self.assertThat(tmpfile.write, MockCallsMatch(
+            call(DOM_TEMPLATE_BRIDGE_INTERFACE.format(
+                **device_params).encode('utf-8')),
+            call(b'\n')))
+        self.assertThat(tmpfile.flush, MockCalledOnceWith())
+        self.assertThat(tmpfile.__exit__, MockCalledOnceWith(None, None, None))
+
     def test_get_domain_capabilities_for_kvm(self):
         conn = self.configure_virshssh(SAMPLE_CAPABILITY_KVM)
         self.assertEqual({
@@ -1387,7 +1472,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
         self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
@@ -1446,7 +1531,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
         self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
@@ -1505,7 +1590,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
         self.assertThat(
             mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
         self.assertThat(
@@ -1564,7 +1649,7 @@ class TestVirshSSH(MAASTestCase):
             mock_attach_disk,
             MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
         self.assertThat(
-            mock_attach_nic, MockCalledOnceWith(ANY, 'maas'))
+            mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))
         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 a19e0aa..42914c0 100644
--- a/src/provisioningserver/drivers/pod/virsh.py
+++ b/src/provisioningserver/drivers/pod/virsh.py
@@ -64,6 +64,21 @@ XPATH_POOL_CAPACITY = "/pool/capacity"
 XPATH_POOL_PATH = "/pool/target/path"
 XPATH_POOL_UUID = "/pool/uuid"
 
+DOM_TEMPLATE_MACVLAN_INTERFACE = dedent("""\
+    <interface type='direct'>
+      <source dev='{attach_name}' mode='{attach_options}'/>
+      <mac address='{mac_address}'/>
+      <model type='virtio'/>
+    </interface>
+    """)
+
+DOM_TEMPLATE_BRIDGE_INTERFACE = dedent("""\
+    <interface type='bridge'>
+      <source bridge='{attach_name}'/>
+      <mac address='{mac_address}'/>
+      <model type='virtio'/>
+    </interface>
+    """)
 
 DOM_TEMPLATE_AMD64 = dedent("""\
     <domain type='{type}'>
@@ -834,12 +849,48 @@ class VirshSSH(pexpect.spawn):
 
         return networks[0]
 
-    def attach_interface(self, domain, network):
+    def attach_interface(self, interface, domain, network):
         """Attach new network interface on `domain` to `network`."""
         mac = generate_mac_address()
-        self.run([
-            'attach-interface', domain, 'network', network,
-            '--mac', mac, '--model', 'virtio', '--config'])
+        # If attachment type is not specified, default to network.
+        if interface.attach_type in (None, 'network'):
+            # Set the network if we are explicity attaching a network
+            # specified by the user.
+            if interface.attach_type is not None:
+                network = interface.attach_name
+            return self.run([
+                'attach-interface', domain, 'network', network,
+                '--mac', mac, '--model', 'virtio', '--config'])
+
+        # For macvlans and bridges, we need to pass an XML template to
+        # virsh's attach-device command since the attach-interface
+        # command doesn't have a flag for setting the macvlan's mode.
+        device_params = {
+            'mac_address': mac,
+            'attach_name': interface.attach_name,
+        }
+        if interface.attach_type == 'macvlan':
+            device_params['attach_options'] = interface.attach_options
+            device_xml = DOM_TEMPLATE_MACVLAN_INTERFACE.format(**device_params)
+        if interface.attach_type == 'bridge':
+            device_xml = DOM_TEMPLATE_BRIDGE_INTERFACE.format(**device_params)
+
+        # Rewrite the XML in a temporary file to use with 'virsh define'.
+        with NamedTemporaryFile() as f:
+            f.write(device_xml.encode('utf-8'))
+            f.write(b'\n')
+            f.flush()
+            output = self.run([
+                'attach-device', domain, f.name, '--config'])
+            if output.startswith('error:'):
+                maaslog.error(
+                    "%s: Failed to attach network device %s" % (
+                        domain, interface.attach_name))
+                return False
+            maaslog.info(
+                "%s: Successfully attached network device %s" % (
+                    domain, interface.attach_name))
+            return True
 
     def get_domain_capabilities(self):
         """Return the domain capabilities.
@@ -965,8 +1016,8 @@ class VirshSSH(pexpect.spawn):
 
         # Attach new interfaces to the best possible network.
         best_network = self.get_best_network()
-        for _ in request.interfaces:
-            self.attach_interface(request.hostname, 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)