New subject: Change in vdsm[master]: Move VM logic to it's own module (pt.1)

23 Aug 2012

Saggi Mizrahi has uploaded a new change for review.
Change subject: [WIP] Move vm logic to it's own module
......................................................................
[WIP] Move vm logic to it's own module
Change-Id: Icc54c1d0ab7ad535d825dcd72fd2f4583b690f44
Signed-off-by: Saggi Mizrahi smizrahi@redhat.com
---
M vdsm/API.py
M vdsm/Makefile.am
M vdsm/clientIF.py
A vdsm/vmContainer.py
4 files changed, 134 insertions(+), 79 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/23/7423/1

diff --git a/vdsm/API.py b/vdsm/API.py
index e132fb9..818e00c 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -167,34 +167,36 @@
                     self.log.error("Error restoring VM parameters",
                             exc_info=True)
-            requiredParams = ['vmId', 'memSize', 'display']
-            for param in requiredParams:
+            for param in ['vmId', 'memSize', 'display']:
                 if param not in vmParams:
-                    self.log.error('Missing required parameter %s' % (param))
-                    return {'status': {'code': errCode['MissParam']
-                                                      ['status']['code'],
-                                       'message': 'Missing required' + \
-                                       'parameter %s' % (param)}}
+                    return {'status': {
+                        'code': errCode['MissParam']['status']['code'],
+                        'message': 'Missing required parameter %s' % (param,)}
+                        }
             try:
                 storage.misc.validateUUID(vmParams['vmId'])
             except:
-                return {'status': {'code': errCode['MissParam']
-                                                  ['status']['code'],
-                                   'message': 'vmId must be a valid UUID'}}
+                return {'status': {
+                    'code': errCode['MissParam']['status']['code'],
+                    'message': 'vmId must be a valid UUID'}
+                    }
             if vmParams['memSize'] == 0:
-                return {'status': {'code': errCode['MissParam']
-                                                  ['status']['code'],
-                                   'message': 'Must specify nonzero memSize'}}
+                return {'status': {
+                    'code': errCode['MissParam']['status']['code'],
+                    'message': 'Must specify nonzero memSize'}
+                    }
if vmParams.get('boot') == 'c' and not 'hda' in vmParams \
                                            and not vmParams.get('drives'):
-                return {'status': {'code': errCode['MissParam']
-                                                  ['status']['code'],
-                                   'message': 'missing boot disk'}}
+                return {'status': {
+                    'code': errCode['MissParam']['status']['code'],
+                    'message': 'missing boot disk'}
+                    }
if 'vmType' not in vmParams:
                 vmParams['vmType'] = 'kvm'
-            elif vmParams['vmType'] == 'kvm':
+
+            if vmParams['vmType'] == 'kvm':
                 if 'kvmEnable' not in vmParams:
                     vmParams['kvmEnable'] = 'true'
@@ -205,25 +207,27 @@
                 vmParams['volatileFloppy'] = True
if caps.osversion()['name'] == caps.OSName.UNKNOWN:
-                return {'status': {'code': errCode['createErr']
-                                                  ['status']['code'],
-                                   'message': 'Unknown host operating system'}}
+                return {'status': {
+                    'code': errCode['createErr']['status']['code'],
+                    'message': 'Unknown host operating system'}
+                    }
if 'sysprepInf' in vmParams:
                 if not self._createSysprepFloppyFromInf(vmParams['sysprepInf'],
                                  vmParams['floppy']):
-                    return {'status': {'code': errCode['createErr']
-                                                      ['status']['code'],
-                                       'message': 'Failed to create '
-                                                  'sysprep floppy image. '
-                                                  'No space on /tmp?'}}
-                    return errCode['createErr']
+                    return {'status': {
+                        'code': errCode['createErr']['status']['code'],
+                        'message': 'Failed to create sysprep floppy image. '
+                                   'No space on /tmp?'}
+                        }
if vmParams.get('display') not in ('vnc', 'qxl', 'qxlnc', 'local'):
-                return {'status': {'code': errCode['createErr']
-                                                  ['status']['code'],
-                                   'message': 'Unknown display type %s'
-                                                % vmParams.get('display')}}
+                return {'status': {
+                    'code': errCode['createErr']['status']['code'],
+                    'message': 'Unknown display type %s' %
+                                    vmParams.get('display')}
+                }
+
             if 'nicModel' not in vmParams:
                 vmParams['nicModel'] = config.get('vars', 'nic_model')
             vmParams['displayIp'] = self._getNetworkIp(vmParams.get(
diff --git a/vdsm/Makefile.am b/vdsm/Makefile.am
index d4c52ed..574d762 100644
--- a/vdsm/Makefile.am
+++ b/vdsm/Makefile.am
@@ -53,6 +53,7 @@
    tc.py \
    vdsmDebugPlugin.py \
    vmChannels.py \
+	vmContainer.py \
    vm.py \
    $(NULL)
diff --git a/vdsm/clientIF.py b/vdsm/clientIF.py
index b9a4913..b2438fb 100644
--- a/vdsm/clientIF.py
+++ b/vdsm/clientIF.py
@@ -24,6 +24,7 @@
 import pickle
 from xml.dom import minidom
 import uuid
+import errno
from storage.dispatcher import Dispatcher
 from storage.hsm import HSM
@@ -40,9 +41,9 @@
 import configNetwork
 import caps
 from vmChannels import Listener
-from libvirtvm import LibvirtVm
 import blkid
 import supervdsm
+import vmContainer
 try:
     import gluster.api as gapi
     _glusterEnabled = True
@@ -66,7 +67,6 @@
         :param log: a log object to be used for this object's logging.
         :type log: :class:`logging.Logger`
         """
-        self.vmContainerLock = threading.Lock()
         self._networkSemaphore = threading.Semaphore()
         self._shutdownSemaphore = threading.Semaphore()
         self.log = log
@@ -82,7 +82,7 @@
         else:
             self.gluster = None
         try:
-            self.vmContainer = {}
+            self.vmContainer = vmContainer.VmContainer()
             ifids = netinfo.nics() + netinfo.bondings()
             ifrates = map(netinfo.speed, ifids)
             self._hostStats = utils.HostStatsThread(
@@ -328,24 +328,19 @@
         return res['status']['code']
def createVm(self, vmParams):
-        self.vmContainerLock.acquire()
-        self.log.info("vmContainerLock acquired by vm %s",
-                          vmParams['vmId'])
         try:
-            if 'recover' not in vmParams:
-                if vmParams['vmId'] in self.vmContainer:
-                    self.log.warning('vm %s already exists' %
-                                     vmParams['vmId'])
-                    return errCode['exist']
-            vm = LibvirtVm(self, vmParams)
-            self.vmContainer[vmParams['vmId']] = vm
-        finally:
-            container_len = len(self.vmContainer)
-            self.vmContainerLock.release()
-        vm.run()
-        self.log.debug("Total desktops after creation of %s is %d" %
-                       (vmParams['vmId'], container_len))
-        return {'status': doneCode, 'vmList': vm.status()}
+            vm = self.vmContainer.createVm(vmParams)
+            return {'status': doneCode, 'vmList': vm.status()}
+        except vmContainer.VmContainerError as e:
+            if e.errno == errno.EEXIST:
+                return errCode['exist']
+            return
+
+    def waitForShutdown(self, timeout=None):
+        self.shutdownEvent(timeout)
+        # Some versions of python have a bug where wait doesn't
+        # really return the correct value.
+        return self.shutdownEvent.isSet()
def _recoverExistingVms(self):
         try:
@@ -364,10 +359,11 @@
                                        'process with id: %s',
                                        vmId, exc_info=True)
-            while (self._enabled and
-                  'WaitForLaunch' in [v.lastStatus for v in
-                                      self.vmContainer.values()]):
-                time.sleep(1)
+            vmList = self.vmContainer.getVMs()
+            while ('WaitForLaunch' in (v.lastStatus for v in vmList)):
+                if self.waitForShutdown(1):
+                    break
+
             self._cleanOldFiles()
             self._recovery = False
@@ -375,20 +371,23 @@
             # and then prepare all volumes.
             # Actually, we need it just to get the resources for future
             # volumes manipulations
-            while self._enabled and self.vmContainer and \
-                  not self.irs.getConnectedStoragePoolsList()['poollist']:
-                time.sleep(5)
+            if len(vmList) > 0:
+                while not self.irs.getConnectedStoragePoolsList()['poollist']:
+                    if self.waitForShutdown(5):
+                        break
-            for vmId, vmObj in self.vmContainer.items():
-                # Let's recover as much VMs as possible
+            # Let's recover as many VMs as possible
+            for vmObj in vmList:
+                # Do not prepare volumes when system goes down
+                if self.waitForShutdown(0):
+                    break
+
                 try:
-                    # Do not prepare volumes when system goes down
-                    if self._enabled:
-                        vmObj.preparePaths(
-                                vmObj.getConfDevices()[vm.DISK_DEVICES])
+                    devices = vmObj.getConfDevices()[vm.DISK_DEVICES]
+                    vmObj.preparePaths(devices)
                 except:
-                    self.log.error("Vm %s recovery failed",
-                                   vmId, exc_info=True)
+                    self.log.error("Vm '%s' recovery failed", vmObj.id,
+                            exc_info=True)
         except:
             self.log.error("Vm's recovery failed", exc_info=True)
@@ -452,27 +451,27 @@
                 return recoveryFile
         except:
             self.log.debug("Error recovering VM", exc_info=True)
-        return None
def _cleanOldFiles(self):
         for f in os.listdir(constants.P_VDSM_RUN):
             try:
                 vmId, fileType = f.split(".", 1)
-                if fileType in ["guest.socket", "monitor.socket", "pid",
+                if fileType not in ["guest.socket", "monitor.socket", "pid",
                                     "stdio.dump", "recovery"]:
-                    if vmId in self.vmContainer:
-                        continue
-                    if f == 'vdsmd.pid':
-                        continue
-                    if f == 'respawn.pid':
-                        continue
-                    if f == 'svdsm.pid':
-                        continue
-                    if f == 'svdsm.sock':
-                        continue
-                else:
                     continue
-                self.log.debug("removing old file " + f)
+
+                if self.vmContainer.hasVM(vmId):
+                    continue
+
+                #FIXME: Move VM crud to a subdir so we don't need a
+                # whitelist. I remember spending a considerable amount of
+                # time while writing Super VDSM trying to figure out why my
+                # pid file was disappearing!
+                if f in ('vdsmd.pid', 'respawn.pid',
+                        'svdsm.pid', 'svdsm.sock'):
+                    continue
+
+                self.log.debug("Removing old file '%s'", f)
                 utils.rmFile(constants.P_VDSM_RUN + f)
             except:
                 pass
diff --git a/vdsm/vmContainer.py b/vdsm/vmContainer.py
new file mode 100644
index 0000000..f15b8eb
--- /dev/null
+++ b/vdsm/vmContainer.py
@@ -0,0 +1,51 @@
+import logging
+import threading
+from libvirtvm import LibvirtVm
+import os
+import errno
+
+
+class VmContainerError(OSError):
+    def __init__(self, errno):
+        OSError.__init__(self, errno, os.strerror(errno))
+
+
+class VmContainer(object):
+    log = logging.getLogger("VmContainer")
+
+    def __init__(self):
+        self._syncroot = threading.Lock()
+        self._vms = {}
+
+    def createVm(self, vmParams):
+        self.log.info("vmContainerLock acquired by vm %s",
+                          vmParams['vmId'])
+
+        with self._syncRoot:
+            if 'recover' not in vmParams:
+                if vmParams['vmId'] in self._vms:
+                    self.log.warning('vm %s already exists' % vmParams['vmId'])
+
+                    raise VmContainerError(errno.EEXIST)
+
+            vm = LibvirtVm(self, vmParams)
+            self._vms[vmParams['vmId']] = vm
+
+        container_len = len(self._vms)
+
+        vm.run()
+        self.log.debug("Total desktops after creation of %s is %d" %
+                       (vmParams['vmId'], container_len))
+
+        return vm
+
+    def getVMs(self):
+        return self._vms.values()
+
+    def hasVM(self, vmId):
+        with self._syncRoot:
+            return vmId in self._vms
+
+    def get(self, vmId):
+        with self._syncRoot:
+            return self._vms.get(vmId)
--
To view, visit http://gerrit.ovirt.org/7423
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icc54c1d0ab7ad535d825dcd72fd2f4583b690f44
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com

    

Change in vdsm[master]: [WIP] Move vm logic to it's own module