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

Proposed by Newell Jensen on 2018-08-24
Status: Merged
Approved by: Newell Jensen on 2018-08-25
Approved revision: 19d0b693523255c2702859ba62c24269acfdd933
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~newell-jensen/maas:lp1788756
Merge into: maas:master
Diff against target: 236 lines (+54/-26)
2 files modified
src/provisioningserver/drivers/pod/tests/test_virsh.py (+40/-15)
src/provisioningserver/drivers/pod/virsh.py (+14/-11)
Reviewer Review Type Date Requested Status
Mike Pontillo (community) 2018-08-24 Approve on 2018-08-25
MAAS Lander Approve on 2018-08-24
Review via email: mp+353741@code.launchpad.net

Commit message

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

To post a comment you must log in.
MAAS Lander (maas-lander) wrote :

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) wrote :

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) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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):
1256 conn = self.configure_virshssh('')1256 conn = self.configure_virshssh('')
1257 domain = factory.make_name('domain')1257 domain = factory.make_name('domain')
1258 network = factory.make_name('network')1258 network = factory.make_name('network')
1259 self.patch(virsh.VirshSSH, "get_best_network").return_value = network
1259 mock_run = self.patch(virsh.VirshSSH, "run")1260 mock_run = self.patch(virsh.VirshSSH, "run")
1260 fake_mac = factory.make_mac_address()1261 fake_mac = factory.make_mac_address()
1261 interface = RequestedMachineInterface()1262 interface = RequestedMachineInterface()
1262 self.patch(virsh, 'generate_mac_address').return_value = fake_mac1263 self.patch(virsh, 'generate_mac_address').return_value = fake_mac
1263 conn.attach_interface(interface, domain, network)1264 conn.attach_interface(interface, domain)
1264 self.assertThat(mock_run, MockCalledOnceWith([1265 self.assertThat(mock_run, MockCalledOnceWith([
1265 'attach-interface', domain, 'network', network,1266 'attach-interface', domain, 'network', network,
1266 '--mac', fake_mac,1267 '--mac', fake_mac,
@@ -1270,12 +1271,13 @@ class TestVirshSSH(MAASTestCase):
1270 conn = self.configure_virshssh('')1271 conn = self.configure_virshssh('')
1271 domain = factory.make_name('domain')1272 domain = factory.make_name('domain')
1272 network = factory.make_name('network')1273 network = factory.make_name('network')
1274 self.patch(virsh.VirshSSH, "get_best_network").return_value = network
1273 mock_run = self.patch(virsh.VirshSSH, "run")1275 mock_run = self.patch(virsh.VirshSSH, "run")
1274 fake_mac = factory.make_mac_address()1276 fake_mac = factory.make_mac_address()
1275 interface = RequestedMachineInterface(1277 interface = RequestedMachineInterface(
1276 attach_name=network, attach_type='network')1278 attach_name=network, attach_type='network')
1277 self.patch(virsh, 'generate_mac_address').return_value = fake_mac1279 self.patch(virsh, 'generate_mac_address').return_value = fake_mac
1278 conn.attach_interface(interface, domain, network)1280 conn.attach_interface(interface, domain)
1279 self.assertThat(mock_run, MockCalledOnceWith([1281 self.assertThat(mock_run, MockCalledOnceWith([
1280 'attach-interface', domain, 'network', network,1282 'attach-interface', domain, 'network', network,
1281 '--mac', fake_mac,1283 '--mac', fake_mac,
@@ -1284,7 +1286,6 @@ class TestVirshSSH(MAASTestCase):
1284 def test_attach_interface_attaches_macvlan(self):1286 def test_attach_interface_attaches_macvlan(self):
1285 conn = self.configure_virshssh('')1287 conn = self.configure_virshssh('')
1286 domain = factory.make_name('domain')1288 domain = factory.make_name('domain')
1287 network = factory.make_name('network')
1288 mock_run = self.patch(virsh.VirshSSH, "run")1289 mock_run = self.patch(virsh.VirshSSH, "run")
1289 fake_mac = factory.make_mac_address()1290 fake_mac = factory.make_mac_address()
1290 interface = RequestedMachineInterface(1291 interface = RequestedMachineInterface(
@@ -1296,7 +1297,7 @@ class TestVirshSSH(MAASTestCase):
1296 tmpfile = NamedTemporaryFile.return_value1297 tmpfile = NamedTemporaryFile.return_value
1297 tmpfile.__enter__.return_value = tmpfile1298 tmpfile.__enter__.return_value = tmpfile
1298 tmpfile.name = factory.make_name("filename")1299 tmpfile.name = factory.make_name("filename")
1299 conn.attach_interface(interface, domain, network)1300 conn.attach_interface(interface, domain)
13001301
1301 device_params = {1302 device_params = {
1302 'mac_address': fake_mac,1303 'mac_address': fake_mac,
@@ -1318,7 +1319,6 @@ class TestVirshSSH(MAASTestCase):
1318 def test_attach_interface_attaches_bridge(self):1319 def test_attach_interface_attaches_bridge(self):
1319 conn = self.configure_virshssh('')1320 conn = self.configure_virshssh('')
1320 domain = factory.make_name('domain')1321 domain = factory.make_name('domain')
1321 network = factory.make_name('network')
1322 mock_run = self.patch(virsh.VirshSSH, "run")1322 mock_run = self.patch(virsh.VirshSSH, "run")
1323 fake_mac = factory.make_mac_address()1323 fake_mac = factory.make_mac_address()
1324 interface = RequestedMachineInterface(1324 interface = RequestedMachineInterface(
@@ -1330,7 +1330,7 @@ class TestVirshSSH(MAASTestCase):
1330 tmpfile = NamedTemporaryFile.return_value1330 tmpfile = NamedTemporaryFile.return_value
1331 tmpfile.__enter__.return_value = tmpfile1331 tmpfile.__enter__.return_value = tmpfile
1332 tmpfile.name = factory.make_name("filename")1332 tmpfile.name = factory.make_name("filename")
1333 conn.attach_interface(interface, domain, network)1333 conn.attach_interface(interface, domain)
13341334
1335 device_params = {1335 device_params = {
1336 'mac_address': fake_mac,1336 'mac_address': fake_mac,
@@ -1434,9 +1434,38 @@ class TestVirshSSH(MAASTestCase):
1434 request = make_requested_machine()1434 request = make_requested_machine()
1435 self.patch(virsh.VirshSSH, "create_local_volume").return_value = None1435 self.patch(virsh.VirshSSH, "create_local_volume").return_value = None
1436 error = self.assertRaises(1436 error = self.assertRaises(
1437 PodInvalidResources, conn.create_domain, request, )1437 PodInvalidResources, conn.create_domain, request)
1438 self.assertEqual("not enough space for disk 0.", str(error))1438 self.assertEqual("not enough space for disk 0.", str(error))
14391439
1440 def test_create_domain_handles_no_network_for_requested_interface(self):
1441 conn = self.configure_virshssh('')
1442 request = make_requested_machine()
1443 request.block_devices = request.block_devices[:1]
1444 request.interfaces = request.interfaces[:1]
1445 disk_info = (factory.make_name('pool'), factory.make_name('vol'))
1446 domain_params = {
1447 "type": "kvm",
1448 "emulator": "/usr/bin/qemu-system-x86_64",
1449 }
1450 self.patch(
1451 virsh.VirshSSH, "create_local_volume").return_value = disk_info
1452 self.patch(virsh.VirshSSH, "get_domain_capabilities").return_value = (
1453 domain_params)
1454 mock_uuid = self.patch(virsh_module, "uuid4")
1455 mock_uuid.return_value = str(uuid4())
1456 domain_params['name'] = request.hostname
1457 domain_params['uuid'] = mock_uuid.return_value
1458 domain_params['arch'] = ARCH_FIX_REVERSE[request.architecture]
1459 domain_params['cores'] = str(request.cores)
1460 domain_params['memory'] = str(request.memory)
1461 NamedTemporaryFile = self.patch(virsh_module, "NamedTemporaryFile")
1462 tmpfile = NamedTemporaryFile.return_value
1463 tmpfile.__enter__.return_value = tmpfile
1464 tmpfile.name = factory.make_name("filename")
1465 self.patch(virsh.VirshSSH, "run")
1466 self.patch(virsh.VirshSSH, "get_network_list").return_value = []
1467 self.assertRaises(PodInvalidResources, conn.create_domain, request)
1468
1440 def test_create_domain_calls_correct_methods_with_amd64_arch(self):1469 def test_create_domain_calls_correct_methods_with_amd64_arch(self):
1441 conn = self.configure_virshssh('')1470 conn = self.configure_virshssh('')
1442 request = make_requested_machine()1471 request = make_requested_machine()
@@ -1462,7 +1491,6 @@ class TestVirshSSH(MAASTestCase):
1462 tmpfile = NamedTemporaryFile.return_value1491 tmpfile = NamedTemporaryFile.return_value
1463 tmpfile.__enter__.return_value = tmpfile1492 tmpfile.__enter__.return_value = tmpfile
1464 tmpfile.name = factory.make_name("filename")1493 tmpfile.name = factory.make_name("filename")
1465 self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
1466 mock_run = self.patch(virsh.VirshSSH, "run")1494 mock_run = self.patch(virsh.VirshSSH, "run")
1467 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")1495 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
1468 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")1496 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1486,7 +1514,7 @@ class TestVirshSSH(MAASTestCase):
1486 mock_attach_disk,1514 mock_attach_disk,
1487 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))1515 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
1488 self.assertThat(1516 self.assertThat(
1489 mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))1517 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
1490 self.assertThat(1518 self.assertThat(
1491 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))1519 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
1492 self.assertThat(1520 self.assertThat(
@@ -1521,7 +1549,6 @@ class TestVirshSSH(MAASTestCase):
1521 tmpfile = NamedTemporaryFile.return_value1549 tmpfile = NamedTemporaryFile.return_value
1522 tmpfile.__enter__.return_value = tmpfile1550 tmpfile.__enter__.return_value = tmpfile
1523 tmpfile.name = factory.make_name("filename")1551 tmpfile.name = factory.make_name("filename")
1524 self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
1525 mock_run = self.patch(virsh.VirshSSH, "run")1552 mock_run = self.patch(virsh.VirshSSH, "run")
1526 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")1553 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
1527 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")1554 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1545,7 +1572,7 @@ class TestVirshSSH(MAASTestCase):
1545 mock_attach_disk,1572 mock_attach_disk,
1546 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))1573 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
1547 self.assertThat(1574 self.assertThat(
1548 mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))1575 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
1549 self.assertThat(1576 self.assertThat(
1550 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))1577 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
1551 self.assertThat(1578 self.assertThat(
@@ -1580,7 +1607,6 @@ class TestVirshSSH(MAASTestCase):
1580 tmpfile = NamedTemporaryFile.return_value1607 tmpfile = NamedTemporaryFile.return_value
1581 tmpfile.__enter__.return_value = tmpfile1608 tmpfile.__enter__.return_value = tmpfile
1582 tmpfile.name = factory.make_name("filename")1609 tmpfile.name = factory.make_name("filename")
1583 self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
1584 mock_run = self.patch(virsh.VirshSSH, "run")1610 mock_run = self.patch(virsh.VirshSSH, "run")
1585 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")1611 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
1586 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")1612 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1604,7 +1630,7 @@ class TestVirshSSH(MAASTestCase):
1604 mock_attach_disk,1630 mock_attach_disk,
1605 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))1631 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
1606 self.assertThat(1632 self.assertThat(
1607 mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))1633 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
1608 self.assertThat(1634 self.assertThat(
1609 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))1635 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
1610 self.assertThat(1636 self.assertThat(
@@ -1639,7 +1665,6 @@ class TestVirshSSH(MAASTestCase):
1639 tmpfile = NamedTemporaryFile.return_value1665 tmpfile = NamedTemporaryFile.return_value
1640 tmpfile.__enter__.return_value = tmpfile1666 tmpfile.__enter__.return_value = tmpfile
1641 tmpfile.name = factory.make_name("filename")1667 tmpfile.name = factory.make_name("filename")
1642 self.patch(virsh.VirshSSH, "get_best_network").return_value = "maas"
1643 mock_run = self.patch(virsh.VirshSSH, "run")1668 mock_run = self.patch(virsh.VirshSSH, "run")
1644 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")1669 mock_attach_disk = self.patch(virsh.VirshSSH, "attach_local_volume")
1645 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")1670 mock_attach_nic = self.patch(virsh.VirshSSH, "attach_interface")
@@ -1663,7 +1688,7 @@ class TestVirshSSH(MAASTestCase):
1663 mock_attach_disk,1688 mock_attach_disk,
1664 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))1689 MockCalledOnceWith(ANY, disk_info[0], disk_info[1], 'vda'))
1665 self.assertThat(1690 self.assertThat(
1666 mock_attach_nic, MockCalledOnceWith(ANY, ANY, 'maas'))1691 mock_attach_nic, MockCalledOnceWith(ANY, ANY))
1667 self.assertThat(1692 self.assertThat(
1668 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))1693 mock_set_machine_autostart, MockCalledOnceWith(request.hostname))
1669 self.assertThat(1694 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):
871871
872 return networks[0]872 return networks[0]
873873
874 def attach_interface(self, interface, domain, network):874 def attach_interface(self, interface, domain):
875 """Attach new network interface on `domain` to `network`."""875 """Attach new network interface on `domain` to `network`."""
876 mac = generate_mac_address()876 mac = generate_mac_address()
877 # If attachment type is not specified, default to network.877 # If attachment type is not specified, default to network.
878 if interface.attach_type in (None, InterfaceAttachType.NETWORK):878 if interface.attach_type in (None, InterfaceAttachType.NETWORK):
879 # Set the network if we are explicity attaching a network879 # Set the network if we are explicity attaching a network
880 # specified by the user.880 # specified by the user.
881 network = self.get_best_network()
881 if interface.attach_type is not None:882 if interface.attach_type is not None:
882 network = interface.attach_name883 network = interface.attach_name
883 return self.run([884 return self.run([
@@ -977,11 +978,7 @@ class VirshSSH(pexpect.spawn):
977 return "vd" + name978 return "vd" + name
978979
979 def create_domain(self, request, default_pool=None):980 def create_domain(self, request, default_pool=None):
980 """Create a domain based on the `request` with hostname.981 """Create a domain based on the `request` with hostname."""
981
982 For now this just uses `get_best_network` to connect the interfaces
983 of the domain to the network.
984 """
985 # Create all the block devices first. If cannot complete successfully982 # Create all the block devices first. If cannot complete successfully
986 # then fail early. The driver currently doesn't do any tag matching983 # then fail early. The driver currently doesn't do any tag matching
987 # for block devices, and is not really required for Virsh.984 # for block devices, and is not really required for Virsh.
@@ -1030,17 +1027,23 @@ class VirshSSH(pexpect.spawn):
1030 f.flush()1027 f.flush()
1031 self.run(['define', f.name])1028 self.run(['define', f.name])
10321029
1030 # Attach the requested interfaces.
1031 for interface in request.interfaces:
1032 try:
1033 self.attach_interface(interface, request.hostname)
1034 except PodInvalidResources as error:
1035 # Delete the defined domain in virsh.
1036 self.run(['destroy', request.hostname])
1037 self.run(['undefine', request.hostname])
1038 # Re-raise the exception.
1039 raise error
1040
1033 # Attach the created disks in order.1041 # Attach the created disks in order.
1034 for idx, (pool, volume) in enumerate(created_disks):1042 for idx, (pool, volume) in enumerate(created_disks):
1035 block_name = self.get_block_name_from_idx(idx)1043 block_name = self.get_block_name_from_idx(idx)
1036 self.attach_local_volume(1044 self.attach_local_volume(
1037 request.hostname, pool, volume, block_name)1045 request.hostname, pool, volume, block_name)
10381046
1039 # Attach new interfaces to the best possible network.
1040 best_network = self.get_best_network()
1041 for interface in request.interfaces:
1042 self.attach_interface(interface, request.hostname, best_network)
1043
1044 # Set machine to autostart.1047 # Set machine to autostart.
1045 self.set_machine_autostart(request.hostname)1048 self.set_machine_autostart(request.hostname)
10461049

Subscribers

People subscribed via source and target branches