Change in vdsm[master]: vmDevices: replace self.conf['devices'] by VmDeviceContainer

Martin Polednik has uploaded a new change for review.

Change subject: vmDevices: replace self.conf['devices'] by VmDeviceContainer
......................................................................

vmDevices: replace self.conf['devices'] by VmDeviceContainer

This patch servers as an implementation of VmDeviceContainer, removing
access to self.conf['devices'] and keeping migration-friendly restore
file by pickling structure similar to self.conf['devices']

Change-Id: I3bdb5c331657c1c0d1ae902eabb5d3315d45bf8b
Signed-off-by: Martin Polednik <mpoledni(a)redhat.com&gt;
---
M vdsm/vm.py
1 file changed, 42 insertions(+), 127 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/62/21162/1

diff --git a/vdsm/vm.py b/vdsm/vm.py
index d4ce708..f2e22cf 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -2919,14 +2919,7 @@
         if not 'recover' in self.conf:
             self.preparePaths(devices[DISK_DEVICES])
             self._prepareTransientDisks(devices[DISK_DEVICES])
-            # Update self.conf with updated devices
-            # For old type vmParams, new 'devices' key will be
-            # created with all devices info
-            newDevices = []
-            for dev in devices.values():
-                newDevices.extend(dev)
 
-            self.conf['devices'] = newDevices
             # 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
@@ -2939,6 +2932,7 @@
             for dev in devices[devType]:
                 self._devices[devType].append(devClass(self.conf, self.log,
                                                        **dev))
+        self.conf['devices'] = self._devices.legacy
 
         # We should set this event as a last part of drives initialization
         self._pathsPreparedEvent.set()
@@ -3072,9 +3066,8 @@
             # we sent command to libvirt and before save conf. In this case
             # we will gather almost all needed info about this NIC from
             # the libvirt during recovery process.
-            self._devices[NIC_DEVICES].append(nic)
             with self._confLock:
-                self.conf['devices'].append(nicParams)
+                self._devices[NIC_DEVICES].append(nic)
             self.saveState()
             self._getUnderlyingNetworkInterfaceInfo()
             hooks.after_nic_hotplug(nicXml, self.conf,
@@ -3109,9 +3102,8 @@
                           'not found' % alias)
 
     def _lookupConfByAlias(self, alias):
-        for devConf in self.conf['devices'][:]:
-            if devConf['type'] == NIC_DEVICES and \
-                    devConf['alias'] == alias:
+        for devConf in self._devices[NIC_DEVICES]:
+            if devConf['alias'] == alias:
                 return devConf
         raise LookupError('Configuration of device identified by alias %s not'
                           'found' % alias)
@@ -3256,15 +3248,6 @@
         # Remove found NIC from vm's NICs list
         if nic:
             self._devices[NIC_DEVICES].remove(nic)
-        # Find and remove NIC device from vm's conf
-        nicDev = None
-        for dev in self.conf['devices'][:]:
-            if (dev['type'] == NIC_DEVICES and
-                    dev['macAddr'].lower() ==
nicParams['macAddr'].lower()):
-                with self._confLock:
-                    self.conf['devices'].remove(dev)
-                nicDev = dev
-                break
 
         self.saveState()
 
@@ -3274,10 +3257,7 @@
             self.log.error("Hotunplug failed", exc_info=True)
             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
                 return errCode['noVM']
-            # Restore NIC device in vm's conf and _devices
-            if nicDev:
-                with self._confLock:
-                    self.conf['devices'].append(nicDev)
+            # Restore NIC device in vm's deviceContainer
             if nic:
                 self._devices[NIC_DEVICES].append(nic)
             self.saveState()
@@ -3365,11 +3345,10 @@
             # we sent command to libvirt and before save conf. In this case
             # we will gather almost all needed info about this drive from
             # the libvirt during recovery process.
-            self._devices[DISK_DEVICES].append(drive)
             if vdsmImg:
                 self.sdIds.append(diskParams['domainID'])
             with self._confLock:
-                self.conf['devices'].append(diskParams)
+                self._devices[DISK_DEVICES].append(drive)
             self.saveState()
             self._getUnderlyingDriveInfo()
             hooks.after_disk_hotplug(driveXml, self.conf,
@@ -3404,16 +3383,6 @@
         if drive.isVdsmImage():
             self.sdIds.remove(drive.domainID)
         self._devices[DISK_DEVICES].remove(drive)
-        # Find and remove disk device from vm's conf
-        diskDev = None
-        for dev in self.conf['devices'][:]:
-            if (dev['type'] == DISK_DEVICES and
-                    dev['path'] == drive.path):
-                with self._confLock:
-                    self.conf['devices'].remove(dev)
-                diskDev = dev
-                break
-
         self.saveState()
 
         hooks.before_disk_hotunplug(driveXml, self.conf,
@@ -3425,10 +3394,6 @@
             if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN:
                 return errCode['noVM']
             self._devices[DISK_DEVICES].append(drive)
-            # Restore disk device in vm's conf and _devices
-            if diskDev:
-                with self._confLock:
-                    self.conf['devices'].append(diskDev)
             self.saveState()
             return {
                 'status': {'code':
errCode['hotunplugDisk']['status']['code'],
@@ -4407,12 +4372,11 @@
         return {'status': doneCode}
 
     def _getBalloonInfo(self):
-        for dev in self.conf['devices']:
-            if dev['type'] == BALLOON_DEVICES and \
-                    dev['specParams']['model'] != 'none':
+        for dev in self._devices[BALLOON_DEVICES]:
+            if dev.specParams['model'] != 'none':
                 max_mem = int(self.conf.get('memSize')) * 1024
                 min_mem = int(self.conf.get('memGuaranteedSize', '0')) *
1024
-                target_mem = dev.get('target', max_mem)
+                target_mem = getattr(dev, 'target', max_mem)
                 cur_mem = self._dom.info()[2]
                 return {'balloon_max': str(max_mem),
                         'balloon_cur': str(cur_mem),
@@ -4443,10 +4407,9 @@
                 return reportError(key='noVM')
             return reportError(msg=e.message)
         else:
-            for dev in self.conf['devices']:
-                if dev['type'] == BALLOON_DEVICES and \
-                        dev['specParams']['model'] != 'none':
-                    dev['target'] = target
+            for dev in self._devices[BALLOON_DEVICES]:
+                if dev.specParams['model'] != 'none':
+                    dev.target = target
             # persist the target value to make it consistent after recovery
             self.saveState()
             return {'status': doneCode}
@@ -4475,9 +4438,12 @@
         passed during VM creation request.
         """
         def isKnownDevice(alias):
-            for dev in self.conf['devices']:
-                if dev.get('alias') == alias:
-                    return True
+            for devClass in self._devices:
+                for dev in devClass:
+                    if (hasattr(dev, 'alias') and
+                            getattr(dev, 'alias') == alias):
+
+                        return True
             return False
 
         devsxml = _domParseStr(self._lastXMLDesc).childNodes[0]. \
@@ -4499,7 +4465,16 @@
                           'alias': alias,
                           'device': device,
                           'address': address}
-                self.conf['devices'].append(newDev)
+
+                # lets find out which device we are dealing with
+                for devType, devClass in self.DeviceMapping:
+                    if device == devType:
+                        self._devices[devType].append(
+                            devClass(self.conf, self.log, **newDev))
+                        break
+
+                # TODO: here comes the problem: the can device not have the
+                # type at this point?
 
     def _getUnderlyingControllerDeviceInfo(self):
         """
@@ -4521,6 +4496,7 @@
             # Get controller address
             address = self._getUnderlyingDeviceAddress(x)
 
+            knownDev = False
             # In case the controller has index and/or model, they
             # are compared. Currently relevant for USB controllers.
             for ctrl in self._devices[CONTROLLER_DEVICES]:
@@ -4529,24 +4505,14 @@
                         (not hasattr(ctrl, 'model') or ctrl.model == model)):
                     ctrl.alias = alias
                     ctrl.address = address
-            # Update vm's conf with address for known controller devices
-            # In case the controller has index and/or model, they
-            # are compared. Currently relevant for USB controllers.
-            knownDev = False
-            for dev in self.conf['devices']:
-                if ((dev['type'] == CONTROLLER_DEVICES) and
-                        (dev['device'] == device) and
-                        (not 'index' in dev or dev['index'] == index)
and
-                        (not 'model' in dev or dev['model'] == model)):
-                    dev['address'] = address
-                    dev['alias'] = alias
                     knownDev = True
+
             # Add unknown controller device to vm's conf
             if not knownDev:
-                self.conf['devices'].append({'type': CONTROLLER_DEVICES,
-                                             'device': device,
-                                             'address': address,
-                                             'alias': alias})
+                self._devices[CONTROLLER_DEVICES].append(
+                    ControllerDevice(self.conf, self.log,
+                                     type=CONTROLLER_DEVICES, device=device,
+                                     address=address, alias=alias))
 
     def _getUnderlyingBalloonDeviceInfo(self):
         """
@@ -4569,13 +4535,6 @@
                 if not hasattr(dev, 'alias'):
                     dev.alias = alias
 
-            for dev in self.conf['devices']:
-                if dev['type'] == BALLOON_DEVICES:
-                    if address and not dev.get('address'):
-                        dev['address'] = address
-                    if not dev.get('alias'):
-                        dev['alias'] = alias
-
     def _getUnderlyingConsoleDeviceInfo(self):
         """
         Obtain the alias for the console device from libvirt
@@ -4586,14 +4545,10 @@
         for x in consolexml:
             # All we care about is the alias
             alias =
x.getElementsByTagName('alias')[0].getAttribute('name')
+
             for dev in self._devices[CONSOLE_DEVICES]:
                 if not hasattr(dev, 'alias'):
                     dev.alias = alias
-
-            for dev in self.conf['devices']:
-                if dev['device'] == CONSOLE_DEVICES and \
-                        not dev.get('alias'):
-                    dev['alias'] = alias
 
     def _getUnderlyingSmartcardDeviceInfo(self):
         """
@@ -4614,12 +4569,6 @@
                     dev.address = address
                     dev.alias = alias
 
-            for dev in self.conf['devices']:
-                if dev['device'] == SMARTCARD_DEVICES and \
-                        not dev.get('address'):
-                    dev['address'] = address
-                    dev['alias'] = alias
-
     def _getUnderlyingWatchdogDeviceInfo(self):
         """
         Obtain watchdog device info from libvirt.
@@ -4638,12 +4587,6 @@
                     if not hasattr(wd, 'address') or not hasattr(wd,
'alias'):
                         wd.address = address
                         wd.alias = alias
-
-                for dev in self.conf['devices']:
-                    if ((dev['type'] == WATCHDOG_DEVICES) and
-                            (not dev.get('address') or not
dev.get('alias'))):
-                        dev['address'] = address
-                        dev['alias'] = alias
 
     def _getUnderlyingVideoDeviceInfo(self):
         """
@@ -4665,13 +4608,6 @@
                     vc.alias = alias
                     vc.address = address
                     break
-            # Update vm's conf with address
-            for dev in self.conf['devices']:
-                if ((dev['type'] == VIDEO_DEVICES) and
-                        (not dev.get('address') or not
dev.get('alias'))):
-                    dev['address'] = address
-                    dev['alias'] = alias
-                    break
 
     def _getUnderlyingSoundDeviceInfo(self):
         """
@@ -4692,13 +4628,6 @@
                 if not hasattr(sc, 'address') or not hasattr(sc,
'alias'):
                     sc.alias = alias
                     sc.address = address
-                    break
-            # Update vm's conf with address
-            for dev in self.conf['devices']:
-                if ((dev['type'] == SOUND_DEVICES) and
-                        (not dev.get('address') or not
dev.get('alias'))):
-                    dev['address'] = address
-                    dev['alias'] = alias
                     break
 
     def _getUnderlyingDriveInfo(self):
@@ -4735,6 +4664,7 @@
             # Get disk address
             address = self._getUnderlyingDeviceAddress(x)
 
+            knownDev = False
             for d in self._devices[DISK_DEVICES]:
                 if d.path == devPath:
                     d.name = name
@@ -4745,17 +4675,8 @@
                     d.readonly = readonly
                     if bootOrder:
                         d.bootOrder = bootOrder
-            # Update vm's conf with address for known disk devices
-            knownDev = False
-            for dev in self.conf['devices']:
-                if dev['type'] == DISK_DEVICES and dev['path'] ==
devPath:
-                    dev['name'] = name
-                    dev['address'] = address
-                    dev['alias'] = alias
-                    dev['readonly'] = str(readonly)
-                    if bootOrder:
-                        dev['bootOrder'] = bootOrder
                     knownDev = True
+
             # Add unknown disk device to vm's conf
             if not knownDev:
                 iface = 'ide' if address['type'] == 'drive' else
'pci'
@@ -4765,7 +4686,8 @@
                            'readonly': str(readonly)}
                 if bootOrder:
                     diskDev['bootOrder'] = bootOrder
-                self.conf['devices'].append(diskDev)
+                self._devices[DISK_DEVICES].append(
+                    Drive(self.conf, self.log, **diskDev))
 
     def _getUnderlyingDisplayPort(self):
         """
@@ -4815,6 +4737,7 @@
                     network = source[0].getAttribute('network')
                     network = network[len(netinfo.LIBVIRT_NET_PREFIX):]
 
+            knownDev = False
             # Get nic address
             address = self._getUnderlyingDeviceAddress(x)
             for nic in self._devices[NIC_DEVICES]:
@@ -4823,15 +4746,6 @@
                     nic.alias = alias
                     nic.address = address
                     nic.linkActive = linkActive
-            # Update vm's conf with address for known nic devices
-            knownDev = False
-            for dev in self.conf['devices']:
-                if (dev['type'] == NIC_DEVICES and
-                        dev['macAddr'].lower() == mac.lower()):
-                    dev['address'] = address
-                    dev['alias'] = alias
-                    dev['name'] = name
-                    dev['linkActive'] = linkActive
                     knownDev = True
             # Add unknown nic device to vm's conf
             if not knownDev:
@@ -4845,7 +4759,8 @@
                           'linkActive': linkActive}
                 if network:
                     nicDev['network'] = network
-                self.conf['devices'].append(nicDev)
+                self._devices[DISK_DEVICES].append(
+                    NetworkInterfaceDevice(self.conf, self.log, **nicDev))
 
     def _setWriteWatermarks(self):
         """


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3bdb5c331657c1c0d1ae902eabb5d3315d45bf8b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni(a)redhat.com&gt;

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vmDevices: replace self.conf['devices'] by VmDeviceContainer
......................................................................


Patch Set 2:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4634/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5434/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5513/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3bdb5c331657c1c0d1ae902eabb5d3315d45bf8b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vmDevices: replace self.conf['devices'] by VmDeviceContainer
......................................................................


Patch Set 1:

Build Successful 

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4555/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5355/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5434/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3bdb5c331657c1c0d1ae902eabb5d3315d45bf8b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Itamar Heim has abandoned this change.

Change subject: vmDevices: replace self.conf['devices'] by VmDeviceContainer
......................................................................


Abandoned

abandoning per no reply. please restore if still relevant.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I3bdb5c331657c1c0d1ae902eabb5d3315d45bf8b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server

Itamar Heim has posted comments on this change.

Change subject: vmDevices: replace self.conf['devices'] by VmDeviceContainer
......................................................................


Patch Set 2:

ping

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3bdb5c331657c1c0d1ae902eabb5d3315d45bf8b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni(a)redhat.com&gt;
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Itamar Heim <iheim(a)redhat.com&gt;
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com&gt;
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com&gt;
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No