Change in vdsm[master]: vm: safe(r) Vm.conf update in creation thread

danken at redhat.com danken at redhat.com
Fri Nov 7 14:11:57 UTC 2014


Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: safe(r) Vm.conf update in creation thread
......................................................................


vm: safe(r) Vm.conf update in creation thread

A VM is created asynchronously for performance's sake.
It is registered in the internal vmContainer inside VDSM
as early as possible, while real initialization is carried in
background in the creation thread.
While creation thread runs, the Vm status is queryable by Engine.

In the VM creation thread, the code will change the device
configuration data received from Engine, by adding new devices
(example: graphic device is added) or by fixing the fields in existing
device configuration
(example: for disks configuration, truesize/apparentsize is added).

The creation thread incorrectly does not take any lock while it is
changing Vm.conf['devices'], so concurrent access can fail because
it can find data mutating while iterating.
This is just made evident by the recent switch to JSON-RPC, but
it is a quite old flaw in VDSM.

This is known to break in at least two cases: JSON encoding
and deepcopy() itself, because both modules in the standard
library use iteritems() internally when dealing with dicts().

Since VM creation thread always need to prepare conf data and to
update it, the best fix is to prepare it without modifying Vm.conf
unsafely and to update it atomically.

This patch does that for buildConfDevices().

Bug-Url: https://bugzilla.redhat.com/1143968
Change-Id: Ie9dee2aa01b2c231b99e02a879dcfbd7ecc7f70a
Signed-off-by: Francesco Romani <fromani at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/34813
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-by: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Reviewed-by: Michal Skrivanek <michal.skrivanek at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M vdsm/virt/vm.py
1 file changed, 9 insertions(+), 2 deletions(-)

Approvals:
  Piotr Kliczewski: Looks good to me, but someone else must approve
  Nir Soffer: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified
  Michal Skrivanek: Looks good to me, but someone else must approve

Objections:
  Martin Polednik: I would prefer that you didn't submit this



-- 
To view, visit http://gerrit.ovirt.org/34813
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie9dee2aa01b2c231b99e02a879dcfbd7ecc7f70a
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list