Change in vdsm[master]: vm: move device setup into __init__

Francesco Romani has uploaded a new change for review.

Change subject: vm: move device setup into __init__
......................................................................

vm: move device setup into __init__

Device hierarchy should never change except fot hotplug/hotunplug.
Besides avoiding RuntimeError, this is more correct and safer.

Change-Id: I5e5a8e7916bb0c0d2519949397a5c1c64085e08e
Signed-off-by: Francesco Romani 
---
M tests/vmTestsData.py
M tests/vmfakelib.py
M vdsm/clientIF.py
M vdsm/virt/vm.py
4 files changed, 38 insertions(+), 15 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/53/34753/1

diff --git a/tests/vmTestsData.py b/tests/vmTestsData.py
index 3316598..bb84412 100644
--- a/tests/vmTestsData.py
+++ b/tests/vmTestsData.py
@@ -29,7 +29,7 @@
     'smp': '1', 'cpuPinning': {}, 'numaTune': {}, 'maxVCpus': '160',
     'tabletEnable': False,
     'displayNetwork': 'mydisp', 'custom': {},
-    'guestNumaNodes': []},
+    'guestNumaNodes': [], 'devices': []},

"""<?xml version="1.0" encoding="utf-8"?>
         <domain type="kvm">
@@ -50,6 +50,11 @@
        path="/var/lib/libvirt/qemu/channels/%(vmId)s.org.qemu.guest_agent.0"/>
                 </channel>
                 <input bus="ps2" type="mouse"/>
+                <memballoon model="none"/>
+                <graphics autoport="yes" passwd="*****"
+       passwdValidTo="1970-01-01T00:00:01" port="-1" type="vnc">
+                    <listen network="vdsm-mydisp" type="network"/>
+                </graphics>
                 </devices>
                 <os>
                     <type arch="x86_64" machine="pc">hvm</type>
@@ -93,7 +98,7 @@
     'smp': '1', 'cpuPinning': {}, 'numaTune': {}, 'maxVCpus': '160',
     'tabletEnable': False,
     'displayNetwork': 'mydisp', 'custom': {},
-    'guestNumaNodes': []},
+    'guestNumaNodes': [], 'devices': []},

"""<?xml version="1.0" encoding="utf-8"?>
         <domain type="kvm"
@@ -116,6 +121,11 @@
                 </channel>
                 <input bus="usb" type="mouse"/>
                 <emulator>/usr/bin/qemu-system-ppc64</emulator>
+                <memballoon model="none"/>
+                <graphics autoport="yes" passwd="*****"
+       passwdValidTo="1970-01-01T00:00:01" port="-1" type="vnc">
+                    <listen network="vdsm-mydisp" type="network"/>
+                </graphics>
                 </devices>
                 <os>
                     <type arch="ppc64" machine="pc">hvm</type>
diff --git a/tests/vmfakelib.py b/tests/vmfakelib.py
index 83c5b7a..1baa681 100644
--- a/tests/vmfakelib.py
+++ b/tests/vmfakelib.py
@@ -169,6 +169,8 @@
                                (libvirtconnection, 'get', Connection)]):
             vmParams = {'vmId': 'TESTING'}
             vmParams.update({} if params is None else params)
+            if 'devices' not in vmParams:
+                vmParams['devices'] = tuple()
             cif = ClientIF()
             fake = vm.Vm(cif, vmParams)
             cif.vmContainer[fake.id] = fake
diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py
index 440e5e7..30a38da 100644
--- a/vdsm/clientIF.py
+++ b/vdsm/clientIF.py
@@ -457,8 +457,7 @@
                 try:
                     # Do not prepare volumes when system goes down
                     if self._enabled:
-                        vmObj.preparePaths(
-                            vmObj.buildConfDevices()[vm.DISK_DEVICES])
+                        vmObj.preparePaths(vmObj.getDiskDevicesConf())
                 except:
                     self.log.error("Vm %s recovery failed",
                                    vmId, exc_info=True)
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 05b15e9..05ee6b0 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1302,6 +1302,8 @@
         self._liveMergeCleanupThreads = {}
         self._shutdownReason = None

+        self._setupDevices()
+
     def _get_lastStatus(self):
         PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS,
                          vmstatus.UP)
@@ -1516,7 +1518,7 @@
             'type': GRAPHICS_DEVICES,
             'device': (
                 'spice'
-                if self.conf['display'] in ('qxl', 'qxlnc')
+                if self.conf.get('display') in ('qxl', 'qxlnc')
                 else 'vnc'),
             'specParams': makeSpecParams(self.conf)}]

@@ -2643,20 +2645,30 @@
         except Exception:
             self.log.warning('failed to set Vm niceness', exc_info=True)

+    def _setupDevices(self):
+        devices = self.buildConfDevices()
+
+        if not self.recovering:
+            self._updateDevices(devices)
+
+        for devType, devClass in self.DeviceMapping:
+            for dev in devices[devType]:
+                self._devices[devType].append(devClass(self.conf, self.log,
+                                                       **dev))
+
     def _run(self):
         self.log.info("VM wrapper has started")
-        devices = self.buildConfDevices()
-        self._updateDiskSizes(devices[DISK_DEVICES])

+        drives = list(self.getDiskDevicesConf())
+        self._updateDiskSizes(drives)
         # recovery flow note:
         # we do not start disk stats collection here since
         # in the recovery flow irs may not be ready yet.
         # Disk stats collection is started from clientIF at the end
         # of the recovery process.
         if not self.recovering:
-            self.preparePaths(devices[DISK_DEVICES])
-            self._prepareTransientDisks(devices[DISK_DEVICES])
-            self._updateDevices(devices)
+            self.preparePaths(drives)
+            self._prepareTransientDisks(drives)
             # We need to save conf here before we actually run VM.
             # It's not enough to save conf only on status changes as we did
             # before, because if vdsm will restarted between VM run and conf
@@ -2664,11 +2676,6 @@
             # So, to get proper device objects during VM recovery flow
             # we must to have updated conf before VM run
             self.saveState()
-
-        for devType, devClass in self.DeviceMapping:
-            for dev in devices[devType]:
-                self._devices[devType].append(devClass(self.conf, self.log,
-                                                       **dev))

# We should set this event as a last part of drives initialization
         self._pathsPreparedEvent.set()
@@ -5362,6 +5369,11 @@
             if dev['type'] == BALLOON_DEVICES:
                 yield dev

+    def getDiskDevicesConf(self):
+        for dev in self.conf['devices']:
+            if dev['type'] == DISK_DEVICES:
+                yield dev
+
     @property
     def sdIds(self):
         """

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13334/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13174/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12384/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1884/ : There was an infra issue, please contact [e-mail]

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 2:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13340/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13180/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12390/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1885/ : There was an infra issue, please contact [e-mail]

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13343/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13183/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12393/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1886/ : There was an infra issue, please contact [e-mail]

Francesco Romani has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 4:

just reformatted the stacktrace into the commit message to make it readable

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 4:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13357/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13197/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12407/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/1890/ : There was an infra issue, please contact [e-mail]

Francesco Romani has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 4:

Succesful partial verification with 34752:
* survived an excess of 550 VM startup without issues (stress test the same which highlighed bz1143968)
* succesfully recovered ~150 VMs without issues

Things to be verified:
- more careful recovery check
- disk hotplug scenario

Francesco Romani has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 4:

(6 comments)

Since this is a quite obscure patch changing quite obscure code, added a few comments with the intent of making reviewer's life a bit easier.

http://gerrit.ovirt.org/#/c/34753/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2662
Line 2663
Line 2664
Line 2665
Line 2666
Up to here old code was changing devices which is a hierarchy of conf data, which will flattened to Vm.conf['devices'] into _updateDevices.

Line 2667
Line 2668
Line 2669
Line 2670
Line 2671
This makes the Device instances from Conf data, so they are created correct and no need to sync

Line 2654: 
Line 2655:         for devType, devClass in self.DeviceMapping:
Line 2656:             for dev in devices[devType]:
Line 2657:                 devObj = devClass(self.conf, self.log, **dev)
Line 2658:                 self._devices[devType].append(devObj)
we need this _diskMap cache only to speedup updateDrivesFromConf below. Other than that, just code move and rearrangement of operations.
Line 2659:                 if devType == DISK_DEVICES and isVdsmImage(dev):
Line 2660:                     self._diskMap[(dev['domainID'], dev['poolID'],
Line 2661:                                    dev['imageID'], dev['volumeID'])] = devObj
Line 2662:

Line 2660:                     self._diskMap[(dev['domainID'], dev['poolID'],
Line 2661:                                    dev['imageID'], dev['volumeID'])] = devObj
Line 2662: 
Line 2663:     def _run(self):
Line 2664:         self.log.info("VM wrapper has started")
after this patch we now start the async creation with
- Vm.conf['devices'] up to date, but not saved in recovery file. This is ok because we can always rebuild Vm.conf['device'] from the conf supplied by Engine, so no real behaviour change here
- Vm._devices (*Device instance tree) created and synced with Vm.conf['devices']
Line 2665: 
Line 2666:         drives = list(self.getDiskDevicesConf())
Line 2667:         self._updateDiskSizes(drives)
Line 2668:         # recovery flow note:

Line 2679:             # saving we will fail in inconsistent state during recovery.
Line 2680:             # So, to get proper device objects during VM recovery flow
Line 2681:             # we must to have updated conf before VM run
Line 2682:             self.saveState()
Line 2683:         self._updateDrivesFromConf(drives)
But here we need to sync up
Line 2684:         self._diskMap = {}  # no longer needed, let's clean it up
Line 2685: 
Line 2686:         # We should set this event as a last part of drives initialization
Line 2687:         self._pathsPreparedEvent.set()

Line 5401:                 if dev:
Line 5402:                     dev.path = drv['path']
Line 5403:                     dev.format = drv['format']
Line 5404:                     dev.apparentsize = drv['apparentsize']
Line 5405:                     dev.truesize = drv['truesize']
these are he fields (potentially) changed into lines 2667-2675
Line 5406:                 else:
Line 5407:                     self.log.error('missing Drive from conf: %s', drv)
Line 5408: 
Line 5409:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 4:

(8 comments)

The nature of the error is very clear, and it is easy to fix by iterating over a copy of the keys or the values.

The solution you suggest is not clear.

http://gerrit.ovirt.org/#/c/34753/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2662
Line 2663
Line 2664
Line 2665
Line 2666
> Up to here old code was changing devices which is a hierarchy of conf data,

I don't understand this comment - can you explain what the old code was doing, and how the new code change it?

Line 2667
Line 2668
Line 2669
Line 2670
Line 2671
> This makes the Device instances from Conf data, so they are created correct

So why did you remove it?

Line 1301:         self._powerDownEvent = threading.Event()
Line 1302:         self._liveMergeCleanupThreads = {}
Line 1303:         self._shutdownReason = None
Line 1304: 
Line 1305:         self._diskMap = {}
When do we clear this cache? It looks like a temporary cache to speed up some operation, but its exits for the entire life of the vm object - right?
Line 1306:         self._setupDevices()
Line 1307: 
Line 1308:     def _get_lastStatus(self):
Line 1309:         PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS,

Line 1302:         self._liveMergeCleanupThreads = {}
Line 1303:         self._shutdownReason = None
Line 1304: 
Line 1305:         self._diskMap = {}
Line 1306:         self._setupDevices()
Adding more stuff to __init__ is not a good idea. Doing stuff in __init__ is always fragile and make code harder to test.

Why we must do this here?
Line 1307: 
Line 1308:     def _get_lastStatus(self):
Line 1309:         PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS,
Line 1310:                          vmstatus.UP)

Line 1518:         return [{
Line 1519:             'type': GRAPHICS_DEVICES,
Line 1520:             'device': (
Line 1521:                 'spice'
Line 1522:                 if self.conf.get('display') in ('qxl', 'qxlnc')
This seems unrelated to this patch.
Line 1523:                 else 'vnc'),
Line 1524:             'specParams': makeSpecParams(self.conf)}]
Line 1525: 
Line 1526:     def getConfSound(self):

Line 2654: 
Line 2655:         for devType, devClass in self.DeviceMapping:
Line 2656:             for dev in devices[devType]:
Line 2657:                 devObj = devClass(self.conf, self.log, **dev)
Line 2658:                 self._devices[devType].append(devObj)
> we need this _diskMap cache only to speedup updateDrivesFromConf below. Oth

We did not have this map before - why do we need it now?
Line 2659:                 if devType == DISK_DEVICES and isVdsmImage(dev):
Line 2660:                     self._diskMap[(dev['domainID'], dev['poolID'],
Line 2661:                                    dev['imageID'], dev['volumeID'])] = devObj
Line 2662:

Line 5377: 
Line 5378:     def getDiskDevicesConf(self):
Line 5379:         for dev in self.conf['devices']:
Line 5380:             if dev['type'] == DISK_DEVICES:
Line 5381:                 yield dev
This is not safe - while some code iterate over the list of devices using this generator, the  list may be modified by another thread.

It is better to avoid such code in multithreaded program.

The name also does not suggest that this is a generator. It looks like an accessor.
Line 5382: 
Line 5383:     @property
Line 5384:     def sdIds(self):
Line 5385:         """

Line 5401:                 if dev:
Line 5402:                     dev.path = drv['path']
Line 5403:                     dev.format = drv['format']
Line 5404:                     dev.apparentsize = drv['apparentsize']
Line 5405:                     dev.truesize = drv['truesize']
> these are he fields (potentially) changed into lines 2667-2675

This code should be in the Drive class. If a drive need to be updated using a dict, the drive should have an update(dict) method.

This will change this to:

for drive in drives:
        drive.update(conf)
Line 5406:                 else:
Line 5407:                     self.log.error('missing Drive from conf: %s', drv)
Line 5408: 
Line 5409:

Francesco Romani has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 4: Code-Review-1

(8 comments)

Yes, I just took a too big step. It is not time yet to clean up the device/conf data confusion, and not in this convoluted way.

The review of parent patch just highlighted that the main assumption behind this change doesn't hold,
so no point to keep going in this direction.

So, what's left to do is fix the access of Vm.conf the way you suggested, by properly copying data with proper locking.
See much simpler and much less ambitious http://gerrit.ovirt.org/#/c/34813/

http://gerrit.ovirt.org/#/c/34753/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2662
Line 2663
Line 2664
Line 2665
Line 2666
> I don't understand this comment - can you explain what the old code was doi

Vm object is created with conf given from outside (Engine usually), but once its __init__() has run, it is not fully initialized yet.

Full initialization happens here, because here we're fixing up the important Vm.conf['devices'] field, and building the Device object tree based on up-to-date Vm.conf['devices'] data. It all happens in buildConfDevices().

Now, the problem is that modern Engines already supply VDSM with some devices. Almost all of them.
VDSM however needs to make sure that all of them are present (e.g. GraphicsDevices) and each of them have all the fields and the fields have the correct data (normalizeVdsmImages).

This makes the dictionaries change.
The case I observed was about Drive.
Code was failing with RuntimeError because the two attributes
'truesize' and 'apparentsize' sprung up midway.

This change was supposed to fix that by
- fixing the device hierarchy before _run() is invoked and Vm is queryable (added to vmContainer). There is no good reason for this to change, Vm configuration is fixed and this flow can't change that.
- updating the fields which needs to be updated, with potentially slow storage access (getVolumeSize) asyncrhonously on _run()

Line 2667
Line 2668
Line 2669
Line 2670
Line 2671
> So why did you remove it?

Not removed, but moved in _setupDevices(), which is invoked in Vm.__init__()

Line 1301:         self._powerDownEvent = threading.Event()
Line 1302:         self._liveMergeCleanupThreads = {}
Line 1303:         self._shutdownReason = None
Line 1304: 
Line 1305:         self._diskMap = {}
> When do we clear this cache? It looks like a temporary cache to speed up so

We clear this in _run(), once Devices are synced with the updated conf.
Line 1306:         self._setupDevices()
Line 1307: 
Line 1308:     def _get_lastStatus(self):
Line 1309:         PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS,

Line 1302:         self._liveMergeCleanupThreads = {}
Line 1303:         self._shutdownReason = None
Line 1304: 
Line 1305:         self._diskMap = {}
Line 1306:         self._setupDevices()
> Adding more stuff to __init__ is not a good idea. Doing stuff in __init__ i

In general, I fully agree with you.

In this case, we are 'just' transforming the data we're given to build the internal representation.
Basically, this is buildConfDevices()

Until this transformation is done, the Vm class is not really initialized, because the internal representation is not yet the expected one.

I was not sure, but everything considered the above seemed enough to have this method called in __init__.
Line 1307: 
Line 1308:     def _get_lastStatus(self):
Line 1309:         PAUSED_STATES = (vmstatus.POWERING_DOWN, vmstatus.REBOOT_IN_PROGRESS,
Line 1310:                          vmstatus.UP)

Line 1518:         return [{
Line 1519:             'type': GRAPHICS_DEVICES,
Line 1520:             'device': (
Line 1521:                 'spice'
Line 1522:                 if self.conf.get('display') in ('qxl', 'qxlnc')
> This seems unrelated to this patch.

Yes, it is a misplaced fix.
Line 1523:                 else 'vnc'),
Line 1524:             'specParams': makeSpecParams(self.conf)}]
Line 1525: 
Line 1526:     def getConfSound(self):

Line 2654: 
Line 2655:         for devType, devClass in self.DeviceMapping:
Line 2656:             for dev in devices[devType]:
Line 2657:                 devObj = devClass(self.conf, self.log, **dev)
Line 2658:                 self._devices[devType].append(devObj)
> We did not have this map before - why do we need it now?

Because this patch was trying to split the device conf normalization (the act of fixing the fields and making sure they all exist) from the configuration update (the act of filling them with real data, e.g. Drive apparent/truesize).

The intent was to avoid alltogether mutating data, so no RuntimeError possible ever, but altough the goal is right, the implementation is heading in the wrong direction.
Line 2659:                 if devType == DISK_DEVICES and isVdsmImage(dev):
Line 2660:                     self._diskMap[(dev['domainID'], dev['poolID'],
Line 2661:                                    dev['imageID'], dev['volumeID'])] = devObj
Line 2662:

Line 5377: 
Line 5378:     def getDiskDevicesConf(self):
Line 5379:         for dev in self.conf['devices']:
Line 5380:             if dev['type'] == DISK_DEVICES:
Line 5381:                 yield dev
> This is not safe - while some code iterate over the list of devices using t

Right, this was wrong.
Line 5382: 
Line 5383:     @property
Line 5384:     def sdIds(self):
Line 5385:         """

Line 5401:                 if dev:
Line 5402:                     dev.path = drv['path']
Line 5403:                     dev.format = drv['format']
Line 5404:                     dev.apparentsize = drv['apparentsize']
Line 5405:                     dev.truesize = drv['truesize']
> This code should be in the Drive class. If a drive need to be updated using

Right
Line 5406:                 else:
Line 5407:                     self.log.error('missing Drive from conf: %s', drv)
Line 5408: 
Line 5409:

Francesco Romani has posted comments on this change.

Change subject: vm: move device setup into __init__
......................................................................

Patch Set 4: Verified-1

This may be a good idea buried into a messy implementation, but the codebase isn't ready yet to this change.
http://gerrit.ovirt.org/#/c/34813/ is just a better direction for a fix.

Francesco Romani has abandoned this change.

Change subject: vm: move device setup into __init__
......................................................................

Abandoned