Change in vdsm[master]: clientIF: automatically unpause vms in EIO when SD becomes a...

abaron at redhat.com abaron at redhat.com
Sun Jun 30 15:01:26 UTC 2013


Ayal Baron has posted comments on this change.

Change subject: clientIF: automatically unpause vms in EIO when SD becomes active
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(8 inline comments)

....................................................
File vdsm/clientIF.py
Line 122:                     pass  # drive is not a dictionary
Line 123:                 except KeyError:
Line 124:                     self.domainVmIds[drive['domainID']] = set([vmId])
Line 125: 
Line 126:     def contEIOVms(self, sdUUID, isValid):
There is no 'connected' domain.  but can definitely be renamed to isDomainStateValid
Line 127:         # This method is called everytime the onDomainConnectivityStateChange
Line 128:         # event is emitted, this event is emitted even when a domain goes
Line 129:         # INVALID if this happens there is nothing to do
Line 130:         if not isValid:


Line 134:         libvirtVms = libvirtCon.listAllDomains(
Line 135:             libvirt.VIR_CONNECT_LIST_DOMAINS_PAUSED)
Line 136: 
Line 137:         if sdUUID in self.domainVmIds:
Line 138:             self.vmContainerLock.acquire()
s/.*/with self.vmContainerLock:/
Line 139:             self.log.info("vmContainerLock acquired")
Line 140:             for libvirtVm in libvirtVms:
Line 141:                 state = libvirtVm.state(0)
Line 142:                 if state[1] == libvirt.VIR_DOMAIN_PAUSED_IOERROR:


Line 144:                     if vmId in self.domainVmIds[sdUUID]:
Line 145:                         vmObj = self.vmContainer[vmId]
Line 146:                         self.log.info("Cont vm %s in EIO", vmId)
Line 147:                         vmObj.cont()
Line 148:             self.vmContainerLock.release()
lock release must be done within scope under finally, but in any event redundant once you change above to 'with'
Line 149: 
Line 150:     @classmethod
Line 151:     def getInstance(cls, log=None):
Line 152:         with cls._instanceLock:


....................................................
File vdsm/storage/hsm.py
Line 396:         storageRefreshThread.daemon = True
Line 397:         storageRefreshThread.start()
Line 398: 
Line 399:     @public
Line 400:     def registerContEIOVmsCallback(self, callbackFunc):
this is a mechanism for registering for domain state change events, no reason to limit it to this use case.
it should be something like (but possibly shorter and clear ;) : registerToAllDomainStateChangeEvents.
Line 401:         self.contEIOVmsCallback = callbackFunc
Line 402: 
Line 403:     def _hsmSchedule(self, name, func, *args):
Line 404:         self.taskMng.scheduleJob("hsm", self.tasksDir, vars.task,


Line 397:         storageRefreshThread.start()
Line 398: 
Line 399:     @public
Line 400:     def registerContEIOVmsCallback(self, callbackFunc):
Line 401:         self.contEIOVmsCallback = callbackFunc
this should probably be a list of callback functions (single occurrence per callback).
Line 402: 
Line 403:     def _hsmSchedule(self, name, func, *args):
Line 404:         self.taskMng.scheduleJob("hsm", self.tasksDir, vars.task,
Line 405:                                  name, func, *args)


....................................................
File vdsm/vm.py
Line 2041:     def isDisksStatsCollectionEnabled(self):
Line 2042:         return self._volumesPrepared
Line 2043: 
Line 2044:     def preparePaths(self, drives):
Line 2045:         self.cif.addVmToContOnEIO(self.id, drives)
This looks wrong to me.
First, if you move this after "drive['path'] =" and you pass only these drives then you ensure that you only ever pass dictionaries and make some checks in the method redundant.
Second, you can limit this even further to 'disk' devices only here and avoid the need to validate that and while you're at it you can already check if it's a PDIV and get just those devices you care about.
In addition, you should store the list you get in a temporary variable and pass it back only in the 'else' clause in case the VM has been destroyed.
So in essence, I'd move most of the logic to a method here (that validates a single device).
Line 2046:         for drive in drives:
Line 2047:             with self._volPrepareLock:
Line 2048:                 if self.destroyed:
Line 2049:                     # A destroy request has been issued, exit early


Line 3121:         if self.isMigrating():
Line 3122:             return errCode['migInProgress']
Line 3123: 
Line 3124:         diskParams = params.get('drive', {})
Line 3125:         diskParams['path'] = self.cif.prepareVolumePath(diskParams)
shouldn't you be adding the VM from this flow as well?
Line 3126: 
Line 3127:         if isVdsmImage(diskParams):
Line 3128:             self._normalizeVdsmImg(diskParams)
Line 3129: 


Line 3160: 
Line 3161:         return {'status': doneCode, 'vmList': self.status()}
Line 3162: 
Line 3163:     def hotunplugDisk(self, params):
Line 3164:         if self.isMigrating():
I believe that here you should be removing the VM if the removed disk is the last one in the domain (for this VM)
Line 3165:             return errCode['migInProgress']
Line 3166: 
Line 3167:         diskParams = params.get('drive', {})
Line 3168:         diskParams['path'] = self.cif.prepareVolumePath(diskParams)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f88ebcab07dea20a0288f6b75c5888a74d9e440
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list