Differential D600

fix: no longer crash when an instance is unregistered.
ClosedPublic
Actions

Authored by roshi on Sep 24 2015, 8:11 PM.

Details

Reviewers
tflink
Maniphest Tasks
T493: testCloud fails when listing instances if virt-manager has deleted an instance
Commits
rTCLOUDa3d1f6f7b47c: fix: no longer crash when an instance is unregistered.
Summary
No longer crashes during 'instance list' when an instance
is no longer registered with libvirt.

Cleaned up some bits. Added logging if configured in
settings.py.
Test Plan
Basic usage and py.test.

Diff Detail

Repository
rTCLOUD testcloud
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
roshi added a task: T493: testCloud fails when listing instances if virt-manager has deleted an instance.Sep 24 2015, 8:11 PM
roshi retitled this revision from to fix: no longer crash when an instance is unregistered..
roshi updated this object.
roshi edited the test plan for this revision. (Show Details)
roshi added a reviewer: tflink.
tflink accepted this revision.Sep 24 2015, 8:15 PM
tflink added inline comments.
testcloud/instance.py
112

If this ends up being used anywhere else, I don't think having a string like this is a good idea. it needs to be a const, enum or something that isn't statically listed in 50 places.

This revision is now accepted and ready to land.Sep 24 2015, 8:15 PM
roshi added a comment.Sep 24 2015, 9:15 PM

You mean the warning or the 'state?'

tflink added a comment.Sep 24 2015, 10:14 PM
In D600#11350, @roshi wrote:

You mean the warning or the 'state?'

The state

Closed by commit rTCLOUDa3d1f6f7b47c: fix: no longer crash when an instance is unregistered. (authored by roshi). · Explain WhySep 25 2015, 3:43 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mtestcloud/cli.py (4 lines)
Mtestcloud/config.py (1 line)
Mtestcloud/instance.py (24 lines)

Diff 1559

View Options

testcloud/cli.py

Show All 14 Lines
15from . import config 15​from . import config
16from . import image 16​from . import image
17from . import instance 17​from . import instance
18from . import util 18​from . import util
19from .exceptions import DomainNotFoundError, TestcloudCliError 19​from .exceptions import DomainNotFoundError, TestcloudCliError
20 20​
21config_data = config.get_config() 21​config_data = config.get_config()
22 22​
23​# Only log to a file when specifically configured to
24​if config_data.LOG_FILE is not None:
25​ logging.basicConfig(filename=config_data.LOG_FILE, level=logging.DEBUG)
26​
23log = logging.getLogger('testcloud') 27​log = logging.getLogger('testcloud')
24log.addHandler(logging.NullHandler()) # this is needed when running in library mode 28​log.addHandler(logging.NullHandler()) # this is needed when running in library mode
25 29​
26description = """Testcloud is a small wrapper program designed to quickly and 30​description = """Testcloud is a small wrapper program designed to quickly and
27simply boot images designed for cloud systems.""" 31​simply boot images designed for cloud systems."""
28 32​
29 33​
30################################################################################ 34​################################################################################
▲ Show 20 Lines • Show All 285 Lines • Show Last 20 Lines
View Options

testcloud/config.py

Show First 20 Lines • Show All 77 Lines • ▼ Show 20 Line(s)
78 78​
79 79​
80class ConfigData(object): 80​class ConfigData(object):
81 '''Holds configuration data for TestCloud. Is initialized with default 81​ '''Holds configuration data for TestCloud. Is initialized with default
82 values which can be overridden. 82​ values which can be overridden.
83 ''' 83​ '''
84 84​
85 DOWNLOAD_PROGRESS = True 85​ DOWNLOAD_PROGRESS = True
86​ LOG_FILE = None
86 87​
87 # Directories testcloud cares about 88​ # Directories testcloud cares about
88 89​
89 DATA_DIR = "/var/lib/testcloud" 90​ DATA_DIR = "/var/lib/testcloud"
90 CACHE_DIR = "/var/lib/testcloud/cache" 91​ CACHE_DIR = "/var/lib/testcloud/cache"
91 92​
92 # Data for cloud-init 93​ # Data for cloud-init
93 94​
Show All 35 Lines
View Options

testcloud/instance.py

Show First 20 Lines • Show All 99 Lines • ▼ Show 20 Line(s)98def list_instances(connection='qemu:///system'):
100 100​
101 :param connection: libvirt compatible connection to use when listing domains 101​ :param connection: libvirt compatible connection to use when listing domains
102 :returns: dictionary of instance_name to domain_state mapping 102​ :returns: dictionary of instance_name to domain_state mapping
103 """ 103​ """
104 system_domains = _list_system_domains(connection) 104​ system_domains = _list_system_domains(connection)
105 all_instances = _list_instances() 105​ all_instances = _list_instances()
106 106​
107 instances = [] 107​ instances = []
108​
108 for instance in all_instances: 109​ for instance in all_instances:
109 if instance['name'] not in system_domains.keys(): 110​ if instance['name'] not in system_domains.keys():
110 raise TestcloudInstanceError("instance {} exists in instances/ " 111​ log.warn('{} is not registered, might want to delete it.'.format(instance['name']))
111 "but is not a libvirt domain on " 112​ instance['state'] = 'de-sync'
tflinkUnsubmitted
Not Done

If this ends up being used anywhere else, I don't think having a string like this is a good idea. it needs to be a const, enum or something that isn't statically listed in 50 places.

112 "{}".format(instance['name'], connection)) 113​
114​ instances.append(instance)
113 115​
114 # Add the state of the instance 116​ else:
115 instance['state'] = system_domains[instance['name']] ​
116 117​
117 instances.append(instance) 118​ # Add the state of the instance
119​ instance['state'] = system_domains[instance['name']]
120​
121​ instances.append(instance)
118 122​
119 return instances 123​ return instances
120 124​
121 125​
122class Instance(object): 126​class Instance(object):
123 """Handles creating, starting, stopping and destroying virtual machines 127​ """Handles creating, starting, stopping and destroying virtual machines
124 defined on the local system, using an existing :py:class:`Image`. 128​ defined on the local system, using an existing :py:class:`Image`.
125 """ 129​ """
▲ Show 20 Lines • Show All 201 Lines • ▼ Show 20 Line(s)
327 331​
328 def set_seed(self, path): 332​ def set_seed(self, path):
329 """Set the seed image for the instance.""" 333​ """Set the seed image for the instance."""
330 self.seed = path 334​ self.seed = path
331 335​
332 def boot(self): 336​ def boot(self):
333 """Boot an already spawned instance.""" 337​ """Boot an already spawned instance."""
334 338​
335 subprocess.Popen(['virsh', 339​ self._run_virsh_command('start')
336 'start', ​
337 self.name ​
338 ]) ​
339 340​
340 def _destroy_virsh_instance(self): 341​ def _destroy_virsh_instance(self):
341 """Remove an instance from virsh.""" 342​ """Remove an instance from virsh."""
342 343​
343 for cmd in ['destroy', 'undefine']: 344​ for cmd in ['destroy', 'undefine']:
344 subprocess.Popen(['virsh', 345​ subprocess.Popen(['virsh',
345 cmd, 346​ cmd,
346 self.name 347​ self.name
347 ]) 348​ ])
348 349​
349 def _run_virsh_command(self, command): 350​ def _run_virsh_command(self, command):
350 subprocess.Popen(['virsh', 351​ subprocess.Popen(['virsh',
351 '-c', 352​ '-c',
352 'qemu:///system', 353​ 'qemu:///system',
353 command, 354​ command,
354 self.name 355​ self.name
355 ]) 356​ ])
356 357​
357 def start(self): 358​ def start(self):
358 """Start the instance""" 359​ """Start the instance"""
359 360​
360 log.debug("starting instance {} with virsh".format(self.name)) 361​ log.debug("attempting to start instance {} with virsh".format(self.name))
361 362​
362 # stop (destroy) the vm using virsh ​
363 self._run_virsh_command('start') 363​ self._run_virsh_command('start')
364 364​
365 def stop(self): 365​ def stop(self):
366 """Stop the instance""" 366​ """Stop the instance"""
367 367​
368 log.debug("stopping instance {} with virsh".format(self.name)) 368​ log.debug("stopping instance {} with virsh".format(self.name))
369 369​
370 # stop (destroy) the vm using virsh 370​ # stop (destroy) the vm using virsh
Show All 29 Lines