testcloud/config.py
94	Shouldn't this file be part of the review?
testcloud/instance.py
290–307	This seems like a silly way to generate the xml files needed to define a vm. I'd think that using jinja (or some other form of templating if you object to another dep) to generate the xml would be a) less fragile and b) more flexible. With the current template and code, I have no say over how much memory the VM has, which network its adaptor is attached to etc.
testcloud/util.py
67	shouldn't you be checking to see if the pseudo-random mac address already exists in the libvirt network before using it?

testcloud/config.py
94	Included now.
testcloud/instance.py
290–308	Fixed in recent commit.
testcloud/util.py
67	To be honest, I don't think there's much chance of collision. Not only would we have to hit the collision, but the domains would also have to be on and connected to the same network. We could check, but then for each instance we'd have to check every other instance libvirt knows about: dump all xml for all domains, search through it and then look for matches. I just don't think it's worth the effort to be that safe - if someone hits it during general use, I'd be surprised.

testcloud/instance.py
299	is this comment still valid?
testcloud/util.py
67	These kinda sound like famous last words to me but that's something which can be added later. do we know what the failure mode is if there's a duplicate mac? my fear is that it would be a silent "network isn't working" kinda error that would be terrible to debug

conf/domain-template.jinja
12 ↗	(On Diff #1618)	Specific CPU is not probably good idea, I've got error: libvirt.libvirtError: unsupported configuration: guest and host CPU are not compatible: Host CPU does not provide required features: tbm, fma4, xop, misalignsse, sse4a, svm When I changed it to `kvm64`, it worked well.

conf/domain-template.jinja
12 ↗	(On Diff #1618)	Yeah, Tim thought that might be an issue. I'd just copied the xml from a domain testcloud had created in the past - I'll test kvm64 here and if it works then I'll make it the default.
testcloud/instance.py
299	Probably not. I'll take it out/re word it.
testcloud/util.py
67	From running the numbers through some Birthday paradox math, it looks like we have ~10% chance of collision if there are 1800 guests running on a single host connected to a single network. I'd be really really surprised if this ever happens.

conf/domain-template.jinja
8 ↗	(On Diff #1660)	the machine argument changes with different versions - my f21 box doesn't have `pc-i440fx-2.3` as a machine type, just `pc-i440fx-2.1` and older. However, the newest "version" of that machine type appears to be aliased to `pc`, so changing that part to `machine='pc'` should be more flexible and not require tweaking for every supported fedora version
testcloud/instance.py
416	The problem with this is that if there's a failure before the domain was actually created, this will TB and the only way to attempt re-creation is to remove `/var/lib/testcloud/instances/<name>` before trying again. This error handling could be improved

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	1602	4343e75		Oct 8 2015, 3:32 PM	★	★
Diff 2	1610	4343e75	- fix: remove virsh calls and replace with libvirt calls	Oct 9 2015, 9:18 PM	★	★
Diff 3	1618	4343e75	- fix: use jinja instead of xml for setting up the domain template	Oct 14 2015, 6:29 PM	★	★
Diff 4	1649	4343e75	feature: use jinja instead of munging xml directly	Nov 2 2015, 8:23 PM	★	★
Diff 5	1660	4343e75	- fix: use generic CPU in template and cleaned up comments	Nov 3 2015, 7:22 PM	★	★
Diff 6	1666	4343e75	- fix: make domain xml more generic	Nov 4 2015, 9:25 PM	★	★
Diff 7	1667	4343e75	Commit rTCLOUDb941b8f7087e7788ddfcb24c406b676f1b447048	Oct 8 2015, 3:32 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
7f7f52295652	11c2a79f1759	0d6e00b796fd	Mike Ruckman	fix: no longer crash when destroying unregistered domain	Nov 4 2015, 9:25 PM
0d6e00b796fd	857e327944db	76f8f7661b13	Mike Ruckman	fix: make domain xml more generic	Nov 4 2015, 8:40 PM
76f8f7661b13	095e108f178f	1b1a8d1958a0	Mike Ruckman	fix: use generic CPU in template and cleaned up comments	Nov 3 2015, 7:22 PM
1b1a8d1958a0	d87f15f7c907	f535c027b147	Mike Ruckman	fix: remove unused import	Oct 14 2015, 6:29 PM
f535c027b147	6667a6353ec3	852fb502ec35	Mike Ruckman	fix: use jinja instead of xml for setting up the domain template	Oct 14 2015, 6:27 PM
852fb502ec35	fdfa6b02b951	b941b8f7087e	Mike Ruckman	fix: remove virsh calls and replace with libvirt calls	Oct 9 2015, 9:18 PM
b941b8f7087e	ac6c6fe201c8	4343e75547d9	Mike Ruckman	feature: use libvirt directly for spawning instances (Show More…)	Oct 8 2015, 3:29 PM

Diff 1667

testcloud/config.py

Show First 20 Lines • Show All 84 Lines • ▼ Show 20 Line(s)		80	class ConfigData(object):
85	DOWNLOAD_PROGRESS = True	85		DOWNLOAD_PROGRESS = True
86	LOG_FILE = None	86		LOG_FILE = None
87		87
88	# Directories testcloud cares about	88		# Directories testcloud cares about
89		89
90	DATA_DIR = "/var/lib/testcloud"	90		DATA_DIR = "/var/lib/testcloud"
91	CACHE_DIR = "/var/lib/testcloud/cache"	91		CACHE_DIR = "/var/lib/testcloud/cache"
92		92
		93		# libvirt domain XML Template
		94		XML_TEMPLATE = "/var/lib/testcloud/domain-template.xml"
			tflinkUnsubmitted Not Done Shouldn't this file be part of the review?
			roshiAuthorUnsubmitted Not Done Included now.
		95
93	# Data for cloud-init	96		# Data for cloud-init
94		97
95	PASSWORD = 'passw0rd'	98		PASSWORD = 'passw0rd'
96	HOSTNAME = 'testcloud'	99		HOSTNAME = 'testcloud'
97		100
98	META_DATA = """instance-id: iid-123456	101		META_DATA = """instance-id: iid-123456
99	local-hostname: %s	102		local-hostname: %s
100	"""	103		"""
Show All 29 Lines

View Options

testcloud/instance.py

Show All 10 Lines
11	import os	11		import os
12	import sys	12		import sys
13	import subprocess	13		import subprocess
14	import glob	14		import glob
15	import logging	15		import logging
16		16
17	import libvirt	17		import libvirt
18	import shutil	18		import shutil
		19		import uuid
		20		import xml.etree.ElementTree as ET
19		21
20	from . import config	22		from . import config
		23		from . import util
21	from .exceptions import TestcloudInstanceError	24		from .exceptions import TestcloudInstanceError
22		25
23	config_data = config.get_config()	26		config_data = config.get_config()
24		27
25	log = logging.getLogger('testcloud.instance')	28		log = logging.getLogger('testcloud.instance')
26		29
27	# mapping libvirt constants to a known set of strings	30		# mapping libvirt constants to a known set of strings
28	DOMAIN_STATUS_ENUM = {libvirt.VIR_DOMAIN_NOSTATE: 'no state',	31		DOMAIN_STATUS_ENUM = {libvirt.VIR_DOMAIN_NOSTATE: 'no state',
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Line(s)		129	class Instance(object):
130		133
131	def __init__(self, name, image=None, hostname=None):	134		def __init__(self, name, image=None, hostname=None):
132	self.name = name	135		self.name = name
133	self.image = image	136		self.image = image
134	self.path = "{}/instances/{}".format(config_data.DATA_DIR, self.name)	137		self.path = "{}/instances/{}".format(config_data.DATA_DIR, self.name)
135	self.seed_path = "{}/{}-seed.img".format(self.path, self.name)	138		self.seed_path = "{}/{}-seed.img".format(self.path, self.name)
136	self.meta_path = "{}/meta".format(self.path)	139		self.meta_path = "{}/meta".format(self.path)
137	self.local_disk = "{}/{}-local.qcow2".format(self.path, self.name)	140		self.local_disk = "{}/{}-local.qcow2".format(self.path, self.name)
		141		self.xml_path = "{}/{}-domain.xml".format(self.path, self.name)
138		142
139	self.ram = 512	143		self.ram = 512
140	self.vnc = False	144		self.vnc = False
141	self.graphics = False	145		self.graphics = False
142	self.atomic = False	146		self.atomic = False
143	self.seed = None	147		self.seed = None
144	self.kernel = None	148		self.kernel = None
145	self.initrd = None	149		self.initrd = None
▲ Show 20 Lines • Show All 132 Lines • ▼ Show 20 Line(s)		281	def create_ip_file(self, ip):
278	"""Write the ip address found after instance creation to a file	282		"""Write the ip address found after instance creation to a file
279	for easier management later. This is likely going to break	283		for easier management later. This is likely going to break
280	and need a better solution."""	284		and need a better solution."""
281		285
282	with open("{}/instances/{}/ip".format(config_data.DATA_DIR,	286		with open("{}/instances/{}/ip".format(config_data.DATA_DIR,
283	self.name), 'w') as ip_file:	287		self.name), 'w') as ip_file:
284	ip_file.write(ip)	288		ip_file.write(ip)
285		289
286	def spawn_vm(self):	290		def write_domain_xml(self):
287	"""Create and boot the instance, using prepared data."""	291		"""Load the default xml template, and populate it with the following:
		292		- name
		293		- uuid
		294		- locations of disks
		295		- network mac address
		296
		297		WARNING: This is fragile. It expects exactly the xml template
		298		testcloud shipped with. In the future, we'll want this to be more
		299		extensible, but we're not there yet."""
			tflinkUnsubmitted Not Done is this comment still valid?
			roshiAuthorUnsubmitted Not Done Probably not. I'll take it out/re word it.
		300
		301		with open(config_data.XML_TEMPLATE, 'r') as template:
		302		template_xml = ''.join([x for x in template.readlines()])
		303
		304		root = ET.fromstring(template_xml)
		305
		306		root.find('./name').text = self.name
		307		root.find('./uuid').text = str(uuid.uuid4())
			tflinkUnsubmitted Not Done This seems like a silly way to generate the xml files needed to define a vm. I'd think that using jinja (or some other form of templating if you object to another dep) to generate the xml would be a) less fragile and b) more flexible. With the current template and code, I have no say over how much memory the VM has, which network its adaptor is attached to etc.
288		308
			roshiAuthorUnsubmitted Not Done Fixed in recent commit.
289	boot_args = ['/usr/bin/virt-install',	309		# Set the disks - Yes this is ugly. These can be hard coded because
290	'--connect',	310		# we're only using the template we ship. It'll have to be more robust
291	'qemu:///system',	311		# later.
292	'--import',	312		disks = root.findall('./devices/disk')
293	'-n',	313		disks[0][1].attrib['file'] = self.local_disk
294	self.name,	314		disks[1][1].attrib['file'] = self.seed_path
295	'-r',
296	str(self.ram),
297	'--os-type=linux', # This should be configurable later
298	'--disk',
299	'{},device=disk,bus=virtio,format=qcow2'.format(
300	self.local_disk),
301	'--disk',
302	'{},device=disk,bus=virtio'.format(self.seed_path),
303	]
304		315
305	# Extend with the customizations from the config_data file	316		# Set a random mac address to avoid collisions
306	boot_args.extend(config_data.CMD_LINE_ARGS)	317		net_interface = root.find('./devices/interface')[0]
		318		net_interface.attrib['address'] = util.generate_mac_address()
307		319
308	if self.graphics:	320		# Write out the final xml file for the domain
309	boot_args.extend(['--noautoconsole'])	321		with open(self.xml_path, 'w') as dom_template:
		322		dom_template.write(ET.tostring(root))
		323
		324		return
		325
		326		def spawn_vm(self):
		327		"""Create and boot the instance, using prepared data."""
310		328
311	if self.vnc:	329		self.write_domain_xml()
312	boot_args.extend(['-vnc', '0.0.0.0:1'])
313		330
314	vm = subprocess.Popen(boot_args)	331		with open(self.xml_path, 'r') as xml_file:
		332		domain_xml = ''.join([x for x in xml_file.readlines()])
315		333
316	log.info("Successfully booted your local cloud image! PID: %d" % vm.pid)	334		conn = libvirt.open('qemu:///system')
		335		domain = conn.defineXML(domain_xml)
		336		self.boot()
317		337
318	return vm	338		log.info("Successfully booted your local cloud image!")
319		339
320	def expand_qcow(self, size="+10G"):	340		def expand_qcow(self, size="+10G"):
321	"""Expand the storage for a qcow image. Currently only used for Atomic	341		"""Expand the storage for a qcow image. Currently only used for Atomic
322	Hosts."""	342		Hosts."""
323		343
324	log.info("expanding qcow2 image {}".format(self.image_path))	344		log.info("expanding qcow2 image {}".format(self.image_path))
325	subprocess.call(['qemu-img',	345		subprocess.call(['qemu-img',
326	'resize',	346		'resize',
▲ Show 20 Lines • Show All 61 Lines • ▼ Show 20 Line(s)		407	if self.name in system_domains and \
388	system_domains[self.name] == 'running':	408		system_domains[self.name] == 'running':
389		409
390	raise TestcloudInstanceError("Cannot remove running instance {}. "	410		raise TestcloudInstanceError("Cannot remove running instance {}. "
391	"Please stop the instance before "	411		"Please stop the instance before "
392	"removing.".format(self.name))	412		"removing.".format(self.name))
393		413
394	# remove from virsh, assuming that it's stopped already	414		# remove from virsh, assuming that it's stopped already
395	self._run_virsh_command('undefine')	415		self._run_virsh_command('undefine')
396		416
			tflinkUnsubmitted Not Done The problem with this is that if there's a failure before the domain was actually created, this will TB and the only way to attempt re-creation is to remove `/var/lib/testcloud/instances/<name>` before trying again. This error handling could be improved
397	log.debug("removing instance {} from disk".format(self.path))	417		log.debug("removing instance {} from disk".format(self.path))
398		418
399	# remove from disk	419		# remove from disk
400	shutil.rmtree(self.path)	420		shutil.rmtree(self.path)

View Options

testcloud/util.py

1	#!/usr/bin/env python	1		#!/usr/bin/env python
2	# -- coding: utf-8 --	2		# -- coding: utf-8 --
3	# Copyright 2015, Red Hat, Inc.	3		# Copyright 2015, Red Hat, Inc.
4	# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>	4		# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
5	# See the LICENSE file for more details on Licensing	5		# See the LICENSE file for more details on Licensing
6		6
7	"""	7		"""
8	This module contains helper functions for testcloud.	8		This module contains helper functions for testcloud.
9	"""	9		"""
10		10
11	import subprocess	11		import subprocess
12	import logging	12		import logging
13		13
		14		import random
14	import libvirt	15		import libvirt
15	import xml.etree.ElementTree as ET	16		import xml.etree.ElementTree as ET
16		17
17	from . import config	18		from . import config
18		19
19	log = logging.getLogger('testcloud.util')	20		log = logging.getLogger('testcloud.util')
20	config_data = config.get_config()	21		config_data = config.get_config()
21		22
Show All 28 Lines
50	def find_ip_from_mac(mac):	51		def find_ip_from_mac(mac):
51	"""Look through ``arp -an`` output for the IP of the provided MAC address.	52		"""Look through ``arp -an`` output for the IP of the provided MAC address.
52	"""	53		"""
53		54
54	arp_list = subprocess.check_output(["arp", "-an"]).split("\n")	55		arp_list = subprocess.check_output(["arp", "-an"]).split("\n")
55	for entry in arp_list:	56		for entry in arp_list:
56	if mac in entry:	57		if mac in entry:
57	return entry.split()[1][1:-1]	58		return entry.split()[1][1:-1]
		59
		60		def generate_mac_address():
		61		"""Create a workable mac address for our instances."""
		62
		63		hex_mac = [0x52, 0x54, 0x00] # These 3 are the prefix libvirt uses
		64		hex_mac += [random.randint(0x00, 0xff) for x in range(3)]
		65		mac = ':'.join(hex(x)[2:] for x in hex_mac)
		66
		67		return mac
			tflinkUnsubmitted Not Done shouldn't you be checking to see if the pseudo-random mac address already exists in the libvirt network before using it?
			roshiAuthorUnsubmitted Not Done To be honest, I don't think there's much chance of collision. Not only would we have to hit the collision, but the domains would also have to be on and connected to the same network. We could check, but then for each instance we'd have to check every other instance libvirt knows about: dump all xml for all domains, search through it and then look for matches. I just don't think it's worth the effort to be that safe - if someone hits it during general use, I'd be surprised.
			tflinkUnsubmitted Not Done These kinda sound like famous last words to me but that's something which can be added later. do we know what the failure mode is if there's a duplicate mac? my fear is that it would be a silent "network isn't working" kinda error that would be terrible to debug
			roshiAuthorUnsubmitted Not Done From running the numbers through some Birthday paradox math, it looks like we have ~10% chance of collision if there are 1800 guests running on a single host connected to a single network. I'd be really really surprised if this ever happens.

			Path	Packages
M			testcloud/config.py (3 lines)
M			testcloud/instance.py (72 lines)
M			testcloud/util.py (10 lines)

feature: use libvirt directly for spawning instances
ClosedPublic
Actions

Details

Diff Detail

Revision Contents

Diff 1667

testcloud/config.py

testcloud/instance.py

testcloud/util.py

feature: use libvirt directly for spawning instancesClosedPublicActions

Details

Diff Detail

Revision Contents

Diff 1667

testcloud/config.py

testcloud/instance.py

testcloud/util.py

feature: use libvirt directly for spawning instances
ClosedPublic
Actions