| roshi | |
| tflink | |
| mkrizek |
| testcloud |
testcloud was crashing when an instance was destroyed by a third-party
after it has been listed but before its state was queried. This patch
ignores such occurence instead of crashing.
There's a new function _find_domain which is more useful when stopping
or removing an instance - we don't need to query through all existing
instances, instead just access exactly the one we want.
test suite passes (but that's not worth much:)), I also tried manually creating an instance, listing all instances, stopping it, and removing it. More extensive testing is welcome. Using this from libtaskotron with --libvirt also seems fine.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
| Path | Packages | |||
|---|---|---|---|---|
| M | .arcconfig (2 lines) | |||
| M | testcloud/instance.py (53 lines) |
| Commit | Tree | Parents | Author | Summary | Date |
|---|---|---|---|---|---|
| 76ec400c4999 | 3597e31de9e6 | dc6e7859dc9a | Kamil Páral | instance: avoid race condition in _list_domains, add _find_domain (Show More…) | Feb 17 2017, 3:17 PM |
| 1 | { | 1 | { | ||
|---|---|---|---|---|---|
| 2 | "project_id" : "testCloud", | 2 | "project_id" : "testCloud", | ||
| 3 | "conduit_uri" : "https://phab.qadevel.cloud.fedoraproject.org", | 3 | "conduit_uri" : "https://phab.qa.fedoraproject.org", | ||
| 4 | "arc.land.onto.default" : "dev", | 4 | "arc.land.onto.default" : "dev", | ||
| 5 | "arc.feature.start.default" : "dev", | 5 | "arc.feature.start.default" : "dev", | ||
| 6 | "unit.engine" : "PytestTestEngine" | 6 | "unit.engine" : "PytestTestEngine" | ||
| 7 | } | 7 | } | ||
| Show All 22 Lines | |||||
| 23 | from . import config | 23 | from . import config | ||
| 24 | from . import util | 24 | from . import util | ||
| 25 | from .exceptions import TestcloudInstanceError | 25 | from .exceptions import TestcloudInstanceError | ||
| 26 | 26 | | |||
| 27 | config_data = config.get_config() | 27 | config_data = config.get_config() | ||
| 28 | 28 | | |||
| 29 | log = logging.getLogger('testcloud.instance') | 29 | log = logging.getLogger('testcloud.instance') | ||
| 30 | 30 | | |||
| 31 | # mapping libvirt constants to a known set of strings | 31 | #: mapping domain state constants from libvirt to a known set of strings | ||
| 32 | DOMAIN_STATUS_ENUM = {libvirt.VIR_DOMAIN_NOSTATE: 'no state', | 32 | DOMAIN_STATUS_ENUM = {libvirt.VIR_DOMAIN_NOSTATE: 'no state', | ||
| 33 | libvirt.VIR_DOMAIN_RUNNING: 'running', | 33 | libvirt.VIR_DOMAIN_RUNNING: 'running', | ||
| 34 | libvirt.VIR_DOMAIN_BLOCKED: 'blocked', | 34 | libvirt.VIR_DOMAIN_BLOCKED: 'blocked', | ||
| 35 | libvirt.VIR_DOMAIN_PAUSED: 'paused', | 35 | libvirt.VIR_DOMAIN_PAUSED: 'paused', | ||
| 36 | libvirt.VIR_DOMAIN_SHUTDOWN: 'shutdown', | 36 | libvirt.VIR_DOMAIN_SHUTDOWN: 'shutdown', | ||
| 37 | libvirt.VIR_DOMAIN_SHUTOFF: 'shutoff', | 37 | libvirt.VIR_DOMAIN_SHUTOFF: 'shutoff', | ||
| 38 | libvirt.VIR_DOMAIN_CRASHED: 'crashed', | 38 | libvirt.VIR_DOMAIN_CRASHED: 'crashed', | ||
| 39 | libvirt.VIR_DOMAIN_PMSUSPENDED: 'suspended' | 39 | libvirt.VIR_DOMAIN_PMSUSPENDED: 'suspended' | ||
| Show All 30 Lines | 67 | def _list_domains(connection): | |||
| 70 | :param connection: libvirt compatible hypervisor connection | 70 | :param connection: libvirt compatible hypervisor connection | ||
| 71 | :returns: dictionary mapping of name -> state | 71 | :returns: dictionary mapping of name -> state | ||
| 72 | :rtype: dict | 72 | :rtype: dict | ||
| 73 | """ | 73 | """ | ||
| 74 | 74 | | |||
| 75 | domains = {} | 75 | domains = {} | ||
| 76 | conn = libvirt.openReadOnly(connection) | 76 | conn = libvirt.openReadOnly(connection) | ||
| 77 | for domain in conn.listAllDomains(): | 77 | for domain in conn.listAllDomains(): | ||
| 78 | # the libvirt docs seem to indicate that the second int is for state | 78 | try: | ||
| 79 | # details, only used when state is ERROR, so only looking at the first | 79 | # the libvirt docs seem to indicate that the second int is for state | ||
| 80 | # int returned for domain.state() | 80 | # details, only used when state is ERROR, so only looking at the first | ||
| 81 | domains[domain.name()] = DOMAIN_STATUS_ENUM[domain.state()[0]] | 81 | # int returned for domain.state() | ||
| 82 | domains[domain.name()] = DOMAIN_STATUS_ENUM[domain.state()[0]] | ||||
| 83 | except libvirt.libvirtError as e: | ||||
| 84 | if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: | ||||
| 85 | # the domain disappeared in the meantime, just ignore | ||||
| 86 | continue | ||||
| 87 | else: | ||||
| 88 | raise e | ||||
| 82 | 89 | | |||
| 83 | return domains | 90 | return domains | ||
| 84 | 91 | | |||
| 85 | 92 | | |||
| 93 | def _find_domain(name, connection): | ||||
| 94 | '''Find whether a domain exists and get its state. | ||||
| 95 | | ||||
| 96 | :param str name: name of the domain to find | ||||
| 97 | :param str connection: name of libvirt connection uri | ||||
| 98 | :returns: domain state from ``DOMAIN_STATUS_ENUM`` if domain exists, or ``None`` if it doesn't | ||||
| 99 | :rtype: str or None | ||||
| 100 | ''' | ||||
| 101 | | ||||
| 102 | conn = libvirt.openReadOnly(connection) | ||||
| 103 | try: | ||||
| 104 | domain = conn.lookupByName(name) | ||||
| 105 | return DOMAIN_STATUS_ENUM[domain.state()[0]] | ||||
| 106 | except libvirt.libvirtError as e: | ||||
| 107 | if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: | ||||
| 108 | # no such domain | ||||
| 109 | return None | ||||
| 110 | else: | ||||
| 111 | raise e | ||||
| 112 | | ||||
| 113 | | ||||
| 86 | def find_instance(name, image=None, connection='qemu:///system'): | 114 | def find_instance(name, image=None, connection='qemu:///system'): | ||
| 87 | """Find an instance using a given name and image, if it exists. | 115 | """Find an instance using a given name and image, if it exists. | ||
| 88 | 116 | | |||
| 89 | Please note that ``connection`` is not taken into account when searching for the instance, but | 117 | Please note that ``connection`` is not taken into account when searching for the instance, but | ||
| 90 | the instance object returned has the specified connection set. It's your responsibility to | 118 | the instance object returned has the specified connection set. It's your responsibility to | ||
| 91 | make sure the provided connection is valid for this instance. | 119 | make sure the provided connection is valid for this instance. | ||
| 92 | 120 | | |||
| 93 | :param str name: name of instance to find | 121 | :param str name: name of instance to find | ||
| ▲ Show 20 Lines • Show All 326 Lines • ▼ Show 20 Line(s) | 162 | class Instance(object): | |||
| 420 | def stop(self): | 448 | def stop(self): | ||
| 421 | """Stop the instance | 449 | """Stop the instance | ||
| 422 | 450 | | |||
| 423 | :raises TestcloudInstanceError: if the instance does not exist | 451 | :raises TestcloudInstanceError: if the instance does not exist | ||
| 424 | """ | 452 | """ | ||
| 425 | 453 | | |||
| 426 | log.debug("stopping instance {}.".format(self.name)) | 454 | log.debug("stopping instance {}.".format(self.name)) | ||
| 427 | 455 | | |||
| 428 | domains = _list_domains(self.connection) | 456 | domain_state = _find_domain(self.name, self.connection) | ||
| 429 | domain_exists = self.name in domains | | |||
| 430 | 457 | | |||
| 431 | if not domain_exists: | 458 | if domain_state is None: | ||
| 432 | raise TestcloudInstanceError("Instance doesn't exist: {}".format(self.name)) | 459 | raise TestcloudInstanceError("Instance doesn't exist: {}".format(self.name)) | ||
| 433 | 460 | | |||
| 434 | if domains[self.name] == 'shutoff': | 461 | if domain_state == 'shutoff': | ||
| 435 | log.debug('Instance already shut off, not stopping: {}'.format(self.name)) | 462 | log.debug('Instance already shut off, not stopping: {}'.format(self.name)) | ||
| 436 | return | 463 | return | ||
| 437 | 464 | | |||
| 438 | # stop (destroy) the vm | 465 | # stop (destroy) the vm | ||
| 439 | self._get_domain().destroy() | 466 | self._get_domain().destroy() | ||
| 440 | 467 | | |||
| 441 | def remove(self, autostop=True): | 468 | def remove(self, autostop=True): | ||
| 442 | """Remove an already stopped instance | 469 | """Remove an already stopped instance | ||
| 443 | 470 | | |||
| 444 | :param bool autostop: if the instance is running, stop it first | 471 | :param bool autostop: if the instance is running, stop it first | ||
| 445 | :raises TestcloudInstanceError: if the instance does not exist, or is still | 472 | :raises TestcloudInstanceError: if the instance does not exist, or is still | ||
| 446 | running and ``autostop==False`` | 473 | running and ``autostop==False`` | ||
| 447 | """ | 474 | """ | ||
| 448 | 475 | | |||
| 449 | log.debug("removing instance {} from libvirt.".format(self.name)) | 476 | log.debug("removing instance {} from libvirt.".format(self.name)) | ||
| 450 | 477 | | |||
| 451 | # this should be changed if/when we start supporting configurable | 478 | # this should be changed if/when we start supporting configurable | ||
| 452 | # libvirt connections | 479 | # libvirt connections | ||
| 453 | domains = _list_domains(self.connection) | 480 | domain_state = _find_domain(self.name, self.connection) | ||
| 454 | # Check that the domain is registered with libvirt | | |||
| 455 | domain_exists = self.name in domains | | |||
| 456 | if domain_exists and domains[self.name] == 'running': | | |||
| 457 | 481 | | |||
| 482 | if domain_state == 'running': | ||||
| 458 | if autostop: | 483 | if autostop: | ||
| 459 | self.stop() | 484 | self.stop() | ||
| 460 | else: | 485 | else: | ||
| 461 | raise TestcloudInstanceError( | 486 | raise TestcloudInstanceError( | ||
| 462 | "Cannot remove running instance {}. Please stop the " | 487 | "Cannot remove running instance {}. Please stop the " | ||
| 463 | "instance before removing.".format(self.name)) | 488 | "instance before removing.".format(self.name)) | ||
| 464 | 489 | | |||
| 465 | # remove from libvirt, assuming that it's stopped already | 490 | # remove from libvirt, assuming that it's stopped already | ||
| 466 | if domain_exists: | 491 | if domain_state is not None: | ||
| 467 | self._get_domain().undefine() | 492 | self._get_domain().undefine() | ||
| 468 | log.debug("Unregistering instance from libvirt.") | 493 | log.debug("Unregistering instance from libvirt.") | ||
| 469 | else: | 494 | else: | ||
| 470 | log.warn('Instance "{}" not found in libvirt "{}". Was it removed already? Should ' | 495 | log.warn('Instance "{}" not found in libvirt "{}". Was it removed already? Should ' | ||
| 471 | 'you have used a different connection?'.format(self.name, self.connection)) | 496 | 'you have used a different connection?'.format(self.name, self.connection)) | ||
| 472 | 497 | | |||
| 473 | log.debug("removing instance {} from disk".format(self.path)) | 498 | log.debug("removing instance {} from disk".format(self.path)) | ||
| 474 | 499 | | |||
| 475 | # remove from disk | 500 | # remove from disk | ||
| 476 | shutil.rmtree(self.path) | 501 | shutil.rmtree(self.path) | ||
| 477 | 502 | | |||
| 478 | def destroy(self): | 503 | def destroy(self): | ||
| 479 | '''A deprecated method. Please call :meth:`remove` instead.''' | 504 | '''A deprecated method. Please call :meth:`remove` instead.''' | ||
| 480 | 505 | | |||
| 481 | log.debug('DEPRECATED: destroy() method was deprecated. Please use remove()') | 506 | log.debug('DEPRECATED: destroy() method was deprecated. Please use remove()') | ||
| 482 | self.remove() | 507 | self.remove() | ||