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

fromani at redhat.com fromani at redhat.com
Fri Nov 7 14:25:29 UTC 2014


Hello Piotr Kliczewski, Nir Soffer, Dan Kenigsberg, Michal Skrivanek,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/34938

to review the following change.

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(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/38/34938/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index aefd51a..e8613f2 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -2033,7 +2033,14 @@
         # For BC we need to save previous behaviour for old type parameters.
         # The new/old type parameter will be distinguished
         # by existence/absence of the 'devices' key
-        if self.conf.get('devices') is None:
+
+        try:
+            # while this code is running, Vm is queryable for status(),
+            # thus we must fix devices in an atomic way, hence the deepcopy
+            with self._confLock:
+                devConf = deepcopy(self.conf['devices'])
+        except KeyError:
+            # (very) old Engines do not send device configuration
             devices[DISK_DEVICES] = self.getConfDrives()
             devices[NIC_DEVICES] = self.getConfNetworkInterfaces()
             devices[SOUND_DEVICES] = self.getConfSound()
@@ -2041,7 +2048,7 @@
             devices[GRAPHICS_DEVICES] = self.getConfGraphics()
             devices[CONTROLLER_DEVICES] = self.getConfController()
         else:
-            for dev in self.conf.get('devices'):
+            for dev in devConf:
                 try:
                     devices[dev['type']].append(dev)
                 except KeyError:


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie9dee2aa01b2c231b99e02a879dcfbd7ecc7f70a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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>


More information about the vdsm-patches mailing list