Saggi Mizrahi has uploaded a new change for review.
Change subject: Fix PEP8 violations in vm.py ......................................................................
Fix PEP8 violations in vm.py
Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Signed-off-by: Saggi Mizrahi smizrahi@redhat.com --- M Makefile.am M vdsm/vm.py 2 files changed, 118 insertions(+), 69 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/49/7449/1
diff --git a/Makefile.am b/Makefile.am index 611fec4..7a07608c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -90,6 +90,7 @@ vdsm/storage/volume.py \ vdsm/supervdsm.py \ vdsm/supervdsmServer.py \ + vdsm/vm.py \ vdsm_cli \ vdsm_hooks/fileinject/before_vm_start.py \ vdsm_hooks/promisc/after_vm_start.py \ diff --git a/vdsm/vm.py b/vdsm/vm.py index 949a3ee..549eb8f 100644 --- a/vdsm/vm.py +++ b/vdsm/vm.py @@ -20,7 +20,9 @@
import os import time -import threading, logging +import threading +import logging +import math from vdsm import constants import tempfile import pickle @@ -50,6 +52,7 @@ A module containing classes needed for VM communication. """
+ def isVdsmImage(drive): return all(k in drive.keys() for k in ('volumeID', 'domainID', 'imageID', 'poolID')) @@ -67,13 +70,17 @@ if not a.startswith('__')] return " ".join(attrs)
-class _MigrationError(RuntimeError): pass + +class _MigrationError(RuntimeError): + pass +
class MigrationSourceThread(threading.Thread): """ A thread that takes care of migration on the source vdsm. """ _ongoingMigrations = threading.BoundedSemaphore(1) + @classmethod def setMaxOutgoingMigrations(cls, n): """Set the initial value of the _ongoingMigrations semaphore. @@ -81,8 +88,8 @@ must not be called after any vm has been run.""" cls._ongoingMigrations = threading.BoundedSemaphore(n)
- def __init__ (self, vm, dst='', dstparams='', - mode='remote', method='online', **kwargs): + def __init__(self, vm, dst='', dstparams='', mode='remote', + method='online', **kwargs): self.log = vm.log self._vm = vm self._dst = dst @@ -92,19 +99,23 @@ self._machineParams = {} self._downtime = kwargs.get('downtime') or \ config.get('vars', 'migration_downtime') - self.status = {'status': {'code': 0, 'message': 'Migration in process'}, 'progress': 0} + self.status = {'status': { + 'code': 0, + 'message': 'Migration in process'}, 'progress': 0 + } threading.Thread.__init__(self) self._preparingMigrationEvt = False self._migrationCanceledEvt = False
- def getStat (self): + def getStat(self): """ Get the status of the migration. """ return self.status
def _setupVdsConnection(self): - if self._mode == 'file': return + if self._mode == 'file': + return self.remoteHost = self._dst.split(':')[0] # FIXME: The port will depend on the binding being used. # This assumes xmlrpc @@ -188,7 +199,11 @@ def _finishSuccessfully(self): if self._mode != 'file': self._vm.setDownStatus(NORMAL, "Migration succeeded") - self.status = {'status': {'code': 0, 'message': 'Migration done'}, 'progress': 100} + self.status = { + 'status': { + 'code': 0, + 'message': 'Migration done'}, + 'progress': 100} else: # don't pickle transient params for ignoreParam in ('displayIp', 'display', 'pid'): @@ -203,7 +218,11 @@ self._vm.cif.teardownVolumePath(self._dstparams)
self._vm.setDownStatus(NORMAL, "SaveState succeeded") - self.status = {'status': {'code': 0, 'message': 'SaveState done'}, 'progress': 100} + self.status = { + 'status': { + 'code': 0, + 'message': 'SaveState done'}, + 'progress': 100}
def run(self): try: @@ -240,12 +259,15 @@ def __str__(self): return "Bad volume specification " + RuntimeError.__str__(self)
-class DoubleDownError(RuntimeError): pass + +class DoubleDownError(RuntimeError): + pass
VALID_STATES = ('Down', 'Migration Destination', 'Migration Source', 'Paused', 'Powering down', 'RebootInProgress', 'Restoring state', 'Saving State', 'Up', 'WaitForLaunch') +
class Vm(object): """ @@ -271,7 +293,7 @@ self.conf = {'pid': '0'} self.conf.update(params) self.cif = cif - self.log = SimpleLogAdapter(self.log, {"vmId" : self.conf['vmId']}) + self.log = SimpleLogAdapter(self.log, {"vmId": self.conf['vmId']}) self.destroyed = False self._recoveryFile = constants.P_VDSM_RUN + str( self.conf['vmId']) + '.recovery' @@ -304,7 +326,7 @@ self._startTime = time.time() - float( self.conf.pop('elapsedTimeOffset', 0))
- self._usedIndices = {} #{'ide': [], 'virtio' = []} + self._usedIndices = {} # {'ide': [], 'virtio' = []} self._volumesPrepared = False self._pathsPreparedEvent = threading.Event() self._devices = {DISK_DEVICES: [], NIC_DEVICES: [], @@ -312,9 +334,15 @@ CONTROLLER_DEVICES: [], GENERAL_DEVICES: [], BALLOON_DEVICES: [], REDIR_DEVICES: []}
+ self.volumeUtilizationChunk = \ + config.getint('irs', 'volume_utilization_chunk_mb') + self.systemShutdownTimeout = \ + config.getint('vars', 'sys_shutdown_timeout') + def _get_lastStatus(self): SHOW_PAUSED_STATES = ('Powering down', 'RebootInProgress', 'Up') - if not self._guestCpuRunning and self._lastStatus in SHOW_PAUSED_STATES: + if (not self._guestCpuRunning and + self._lastStatus in SHOW_PAUSED_STATES): return 'Paused' return self._lastStatus
@@ -344,7 +372,7 @@ def _normalizeVdsmImg(self, drv): drv['needExtend'] = False drv['reqsize'] = drv.get('reqsize', '0') - if not drv.has_key('device'): + if not 'device' in drv: drv['device'] = 'disk'
if drv['device'] == 'disk': @@ -358,22 +386,26 @@ drv['apparentsize'] = 0
def __legacyDrives(self): - """ - Backward compatibility for qa scripts that specify direct paths. - """ - legacies = [] - for index, linuxName in ((0, 'hda'), (1, 'hdb'), (2, 'hdc'), (3, 'hdd')): - path = self.conf.get(linuxName) - if path: - legacies.append({'type': DISK_DEVICES, 'device': 'disk', - 'path': path, 'iface': 'ide', 'index': index, - 'truesize': 0}) - return legacies + """ + Backward compatibility for qa scripts that specify direct paths. + """ + legacies = [] + for index, linuxName in enumerate(('hda', 'hdb', 'hdc', 'hdd')): + path = self.conf.get(linuxName) + if path: + legacies.append({'type': DISK_DEVICES, 'device': 'disk', + 'path': path, 'iface': 'ide', 'index': index, + 'truesize': 0}) + return legacies
def __removableDrives(self): - removables = [{'type': DISK_DEVICES, 'device': 'cdrom', 'iface': 'ide', - 'path': self.conf.get('cdrom', ''), 'index': 2, - 'truesize': 0}] + removables = [{ + 'type': DISK_DEVICES, + 'device': 'cdrom', + 'iface': 'ide', + 'path': self.conf.get('cdrom', ''), + 'index': 2, + 'truesize': 0}] floppyPath = self.conf.get('floppy') if floppyPath: removables.append({'type': DISK_DEVICES, 'device': 'floppy', @@ -440,14 +472,16 @@ """ controllers = [] # For now we create by default only 'virtio-serial' controller - controllers.append({'type': CONTROLLER_DEVICES, 'device': 'virtio-serial'}) + controllers.append({ + 'type': CONTROLLER_DEVICES, + 'device': 'virtio-serial'}) return controllers
def getConfVideo(self): """ Normalize video device provided by conf. """ - vcards =[] + vcards = [] if self.conf.get('display') == 'vnc': devType = 'cirrus' elif self.conf.get('display') == 'qxl': @@ -480,9 +514,12 @@ macs = self.conf.get('macAddr', '').split(',') models = self.conf.get('nicModel', '').split(',') bridges = self.conf.get('bridge', DEFAULT_BRIDGE).split(',') - if macs == ['']: macs = [] - if models == ['']: models = [] - if bridges == ['']: bridges = [] + if macs == ['']: + macs = [] + if models == ['']: + models = [] + if bridges == ['']: + bridges = [] if len(models) < len(macs) or len(models) < len(bridges): raise ValueError('Bad nic specification') if models and not (macs or bridges): @@ -495,17 +532,20 @@ for mac, model, bridge in zip(macs, models, bridges): if model == 'pv': model = 'virtio' - nics.append({'type': NIC_DEVICES, 'macAddr': mac, 'nicModel': model, - 'network': bridge, 'device': 'bridge'}) + nics.append({ + 'type': NIC_DEVICES, + 'macAddr': mac, + 'nicModel': model, + 'network': bridge, + 'device': 'bridge'}) return nics
def getConfDrives(self): """ Normalize drives provided by conf. """ - # FIXME - # Will be better to change the self.conf but this implies an API change. - # Remove this when the API parameters will be consistent. + # FIXME: Will be better to change the self.conf but this implies an API + # change. Remove this when the API parameters will be consistent. confDrives = self.conf['drives'] if self.conf.get('drives') else [] if not confDrives: confDrives.extend(self.__legacyDrives()) @@ -536,7 +576,7 @@ drives = [(order, drv) for order, drv in enumerate(confDrives)] indexed = [] for order, drv in drives: - if not self._usedIndices.has_key(drv['iface']): + if drv['iface'] not in self._usedIndices: self._usedIndices[drv['iface']] = [] idx = drv.get('index') if idx is not None: @@ -556,8 +596,8 @@ """ Reserve the required memory for this VM. """ - self.memCommitted = 2**20 * (int(self.conf['memSize']) + - config.getint('vars', 'guest_ram_overhead')) + self.memCommitted = ((2 ** 20) * (int(self.conf['memSize'])) + + config.getint('vars', 'guest_ram_overhead'))
def _startUnderlyingVm(self): self.log.debug("Start") @@ -653,7 +693,7 @@ load = len(self.cif.vmContainer) return base * (doubler + load) / doubler
- def saveState (self): + def saveState(self): if self.destroyed: return toSave = deepcopy(self.status()) @@ -666,7 +706,8 @@ toSave['guestIPs'] = "" if 'sysprepInf' in toSave: del toSave['sysprepInf'] - if 'floppy' in toSave: del toSave['floppy'] + if 'floppy' in toSave: + del toSave['floppy'] for drive in toSave.get('drives', []): for d in self._devices[DISK_DEVICES]: if d.isVdsmImage() and drive.get('volumeID') == d.volumeID: @@ -675,11 +716,11 @@
with tempfile.NamedTemporaryFile(dir=constants.P_VDSM_RUN, delete=False) as f: - pickle.dump(toSave, f) + pickle.dump(toSave, f)
os.rename(f.name, self._recoveryFile)
- def onReboot (self, withRelaunch): + def onReboot(self, withRelaunch): try: self.log.debug('reboot event') self._startTime = time.time() @@ -695,7 +736,7 @@ except: self.log.error("Reboot event failed", exc_info=True)
- def onShutdown (self): + def onShutdown(self): self.log.debug('onShutdown() event') self.user_destroy = True
@@ -719,12 +760,13 @@ def _lvExtend(self, block_dev, newsize=None): volID = None for d in self._devices[DISK_DEVICES]: - if not d.blockDev: continue - if d.name != block_dev: continue + if not d.blockDev: + continue + if d.name != block_dev: + continue if newsize is None: - newsize = config.getint('irs', - 'volume_utilization_chunk_mb') + (d.apparentsize + 2**20 - - 1) / 2**20 + apperantSizeMB = int(math.ceil(d.apparentsize / (2.0 ** 20.0))) + newsize = self.volumeUtilizationChunk + apperantSizeMB # TODO cap newsize by max volume size volDict = {'poolID': d.poolID, 'domainID': d.domainID, 'imageID': d.imageID, 'volumeID': d.volumeID, @@ -732,11 +774,11 @@ d.needExtend = True d.reqsize = newsize # sendExtendMsg expects size in bytes - self.cif.irs.sendExtendMsg(d.poolID, volDict, newsize * 2**20, + self.cif.irs.sendExtendMsg(d.poolID, volDict, newsize * (2 ** 20), self._afterLvExtend) self.log.debug('%s/%s (%s): apparentsize %s req %s', d.domainID, - d.volumeID, d.name, d.apparentsize / constants.MEGAB, - newsize) #in MiB + d.volumeID, d.name, d.apparentsize / constants.MEGAB, + newsize) # in MiB
volID = d.volumeID break @@ -778,8 +820,8 @@ apparentsize = int(res['apparentsize']) truesize = int(res['truesize']) self.log.debug('_afterLvExtend apparentsize %s req size %s', - apparentsize / constants.MEGAB, d.reqsize) # MiB - if apparentsize >= d.reqsize * constants.MEGAB: #in Bytes + apparentsize / constants.MEGAB, d.reqsize) # MiB + if apparentsize >= d.reqsize * constants.MEGAB: # in Bytes d.needExtend = False try: self.cont() @@ -821,8 +863,8 @@ self._acquireCpuLockWithTimeout() try: if self.lastStatus in ('Migration Source', 'Saving State', 'Down'): - self.log.error('cannot cont while %s', self.lastStatus) - return errCode['unexpected'] + self.log.error('cannot cont while %s', self.lastStatus) + return errCode['unexpected'] self._underlyingCont() if hasattr(self, 'updateGuestCpuRunning'): self.updateGuestCpuRunning() @@ -860,7 +902,7 @@ self._guestEvent = 'Powering down' self.log.debug('guestAgent shutdown called') self.guestAgent.desktopShutdown(timeout, message) - agent_timeout = int(timeout) + config.getint('vars', 'sys_shutdown_timeout') + agent_timeout = int(timeout) + self.systemShutdownTimeout timer = threading.Timer(agent_timeout, self._timedShutdown) timer.start() elif utils.tobool(self.conf.get('acpiEnable', 'true')): @@ -869,8 +911,10 @@ self._acpiShutdown() # No tools, no ACPI else: - return {'status': {'code': errCode['exist']['status']['code'], - 'message': 'VM without ACPI or active SolidICE tools. Try Forced Shutdown.'}} + return {'status': { + 'code': errCode['exist']['status']['code'], + 'message': 'VM without ACPI or active SolidICE tools. ' + 'Try Forced Shutdown.'}} except: self.log.error("Shutdown failed", exc_info=True) return {'status': {'code': doneCode['code'], @@ -912,7 +956,7 @@ utils.rmFile(self._guestSocketFile) utils.rmFile(self._recoveryFile)
- def setDownStatus (self, code, reason): + def setDownStatus(self, code, reason): try: self.lastStatus = 'Down' self.conf['exitCode'] = code @@ -972,7 +1016,7 @@ 'network': {}, 'disks': {}, 'monitorResponse': str(self._monitorResponse), 'nice': self._nice, - 'elapsedTime' : str(int(time.time() - self._startTime)), + 'elapsedTime': str(int(time.time() - self._startTime)), } if 'cdrom' in self.conf: stats['cdrom'] = self.conf['cdrom'] @@ -998,12 +1042,15 @@ try: stats['disks'][var] = {} for value in decStats[var]: - stats['disks'][var][value] = utils.convertToStr(decStats[var][value]) + stats['disks'][var][value] = \ + utils.convertToStr(decStats[var][value]) except: - self.log.error("Error setting vm disk stats", exc_info=True) + self.log.error("Error setting vm disk stats", + exc_info=True)
- - if self.lastStatus in ('Saving State', 'Restoring state', 'Migration Source', 'Migration Destination', 'Paused'): + if self.lastStatus in ('Saving State', + 'Restoring state', 'Migration Source', + 'Migration Destination', 'Paused'): stats['status'] = self.lastStatus elif self.isMigrating(): if self._migrationSourceThread._mode == 'file': @@ -1026,8 +1073,9 @@ memUsage = 0 realMemUsage = int(stats['memUsage']) if realMemUsage != 0: - memUsage = 100 - float(realMemUsage) / int(self.conf['memSize']) * 100 - stats['memUsage'] = utils.convertToStr(int(memUsage)) + memUsage = 1 - (float(realMemUsage) / int(self.conf['memSize'])) + + stats['memUsage'] = utils.convertToStr(int(memUsage * 100)) return stats
def isMigrating(self):
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Fix PEP8 violations in vm.py ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/641/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: Fix PEP8 violations in vm.py ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(10 inline comments)
.................................................... File vdsm/vm.py Line 101: config.get('vars', 'migration_downtime') Line 102: self.status = {'status': { Line 103: 'code': 0, Line 104: 'message': 'Migration in process'}, 'progress': 0 Line 105: } I think it can be self.status = {'status': {'code': 0, 'message': 'Migration in process'}, 'progress': 0}
or
self.status = {'status': {'code': 0, 'message': 'Migration in process'}, 'progress': 0} Line 106: threading.Thread.__init__(self) Line 107: self._preparingMigrationEvt = False Line 108: self._migrationCanceledEvt = False Line 109:
Line 125: except: Line 126: pass Line 127: serverAddress = self.remoteHost + ':' + self.remotePort Line 128: if config.getboolean('vars', 'ssl'): Line 129: self.destServer = vdscli.connect(serverAddress, useSSL=True, How about
self.destServer = vdscli.connect( serverAddress, useSSL=True, TransportClass=kaxmlrpclib.TcpkeepSafeTransport) Line 130: TransportClass=kaxmlrpclib.TcpkeepSafeTransport) Line 131: else: Line 132: self.destServer = kaxmlrpclib.Server('http://' + serverAddress) Line 133: self.log.debug('Destination server is: ' + serverAddress)
Line 172: break Line 173: time.sleep(1) Line 174: lockTimeout -= 1 Line 175: if lockTimeout == 0: Line 176: self.log.warning('Agent ' + self._vm.id + Same as line 130. Line 177: ' unresponsive. Hiberanting without desktopLock.') Line 178: break Line 179: self._vm.pause('Saving State') Line 180: else:
Line 233: self.status['progress'] = 10 Line 234: MigrationSourceThread._ongoingMigrations.acquire() Line 235: try: Line 236: self.log.debug("migration semaphore acquired") Line 237: if not mstate: Same as line 130. Line 238: self._vm.conf['_migrationParams'] = {'dst': self._dst, Line 239: 'mode': self._mode, 'method': self._method, Line 240: 'dstparams': self._dstparams} Line 241: self._vm.saveState()
Line 241: self._vm.saveState() Line 242: self._startUnderlyingMigration() Line 243: self._finishSuccessfully() Line 244: except libvirt.libvirtError, e: Line 245: if e.get_error_code() == libvirt.VIR_ERR_OPERATION_ABORTED: Same as line 130.... I will skip similar cases. Line 246: self.status = {'status': {'code': errCode['migCancelErr'], Line 247: 'message': 'Migration canceled'}} Line 248: raise Line 249: finally:
Line 341: Line 342: def _get_lastStatus(self): Line 343: SHOW_PAUSED_STATES = ('Powering down', 'RebootInProgress', 'Up') Line 344: if (not self._guestCpuRunning and Line 345: self._lastStatus in SHOW_PAUSED_STATES): Can "self" in the second line align to "not" in the upper line? Line 346: return 'Paused' Line 347: return self._lastStatus Line 348: Line 349: def _set_lastStatus(self, value):
Line 406 Line 407 Line 408 Line 409 Line 410 Is it better if we write "if self.conf.get('devices') is None" ?
Line 606: self._ongoingCreations.acquire() Line 607: self.log.debug("_ongoingCreations acquired") Line 608: try: Line 609: self._run() Line 610: if self.lastStatus != 'Down' and 'recover' not in self.conf \ I think it could be
if (self.lastStatus != 'Down' and 'recover' not in self.conf and not self.cif.mom): Line 611: and not self.cif.mom: Line 612: # If MOM is available, we needn't tell it to adjust KSM Line 613: # behaviors on VM start/destroy, because the tuning can be Line 614: # done automatically acccording its statistical data.
Line 621: finally: Line 622: self._ongoingCreations.release() Line 623: self.log.debug("_ongoingCreations released") Line 624: Line 625: if ('migrationDest' in self.conf or 'restoreState' in self.conf The upper 2 lines are not very nice... Line 626: ) and self.lastStatus != 'Down': Line 627: self._waitForIncomingMigrationFinish() Line 628: Line 629: self.lastStatus = 'Up'
Line 1090: # while we were blocking, another migrationSourceThread could have Line 1091: # taken self Down Line 1092: if self._lastStatus == 'Down': Line 1093: return errCode['noVM'] Line 1094: self._migrationSourceThread = self.MigrationSourceThreadClass(self, I think it's OK to use back slash here
self._migrationSourceThread = \ self.MigrationSourceThreadClass(self, **params) Line 1095: **params) Line 1096: self._migrationSourceThread.start() Line 1097: check = self._migrationSourceThread.getStat() Line 1098: if check['status']['code']:
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Saggi Mizrahi has posted comments on this change.
Change subject: Fix PEP8 violations in vm.py ......................................................................
Patch Set 1:
Non of these are officially PEP8 issues. Maybe the "if alignment" one.
I'm not saying it's not better, I'm just saying it's not mandatory for having the patch pushed in before I need to rebase.
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: Fix PEP8 violations in vm.py ......................................................................
Patch Set 1: No score
(3 inline comments)
I just located the cases that caught my eye. The current pep8 tool can not cover every case in PEP 8.
.................................................... File vdsm/vm.py Line 101: config.get('vars', 'migration_downtime') Line 102: self.status = {'status': { Line 103: 'code': 0, Line 104: 'message': 'Migration in process'}, 'progress': 0 Line 105: } http://www.python.org/dev/peps/pep-0008/#indentation "Continuation lines should align wrapped elements either vertically using Python's implicit line joining inside parentheses, brackets and braces, or using a hanging indent."
No: # Arguments on first line forbidden when not using vertical alignment foo = long_function_name(var_one, var_two, var_three, var_four) Line 106: threading.Thread.__init__(self) Line 107: self._preparingMigrationEvt = False Line 108: self._migrationCanceledEvt = False Line 109:
Line 125: except: Line 126: pass Line 127: serverAddress = self.remoteHost + ':' + self.remotePort Line 128: if config.getboolean('vars', 'ssl'): Line 129: self.destServer = vdscli.connect(serverAddress, useSSL=True, Same as line 105. Line 130: TransportClass=kaxmlrpclib.TcpkeepSafeTransport) Line 131: else: Line 132: self.destServer = kaxmlrpclib.Server('http://' + serverAddress) Line 133: self.log.debug('Destination server is: ' + serverAddress)
Line 606: self._ongoingCreations.acquire() Line 607: self.log.debug("_ongoingCreations acquired") Line 608: try: Line 609: self._run() Line 610: if self.lastStatus != 'Down' and 'recover' not in self.conf \ http://www.python.org/dev/peps/pep-0008/#maximum-line-length "The preferred place to break around a binary operator is after the operator, not before it." Line 611: and not self.cif.mom: Line 612: # If MOM is available, we needn't tell it to adjust KSM Line 613: # behaviors on VM start/destroy, because the tuning can be Line 614: # done automatically acccording its statistical data.
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Fix some PEP8 violations in vm.py so the pep8 tool would stop complaining ......................................................................
Patch Set 2:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/690/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Adam Litke has posted comments on this change.
Change subject: Fix some PEP8 violations in vm.py so the pep8 tool would stop complaining ......................................................................
Patch Set 2: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Fix some PEP8 violations in vm.py so the pep8 tool would stop complaining ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/vm.py Line 333: SOUND_DEVICES: [], VIDEO_DEVICES: [], Line 334: CONTROLLER_DEVICES: [], GENERAL_DEVICES: [], Line 335: BALLOON_DEVICES: [], REDIR_DEVICES: []} Line 336: Line 337: self.volumeUtilizationChunk = \ thes self.CONSTANTS are better placed in a patch of their own. Line 338: config.getint('irs', 'volume_utilization_chunk_mb') Line 339: self.systemShutdownTimeout = \ Line 340: config.getint('vars', 'sys_shutdown_timeout') Line 341:
Line 764: continue Line 765: if d.name != block_dev: Line 766: continue Line 767: if newsize is None: Line 768: apperantSizeMB = int(math.ceil(d.apparentsize / (2.0 ** 20.0))) I'm not crazy for floating point arithmetic, certainly not under the pep8 title.
I would appreciate if you define and use
def ceildiv(m, n): return (m + n - 1) // n
instead. Line 769: newsize = self.volumeUtilizationChunk + apperantSizeMB Line 770: # TODO cap newsize by max volume size Line 771: volDict = {'poolID': d.poolID, 'domainID': d.domainID, Line 772: 'imageID': d.imageID, 'volumeID': d.volumeID,
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: Fix some PEP8 violations in vm.py so the pep8 tool would stop complaining ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(4 inline comments)
.................................................... File vdsm/vm.py Line 439: # For BC we need to save previous behaviour for old type parameters. Line 440: # The new/old type parameter will be distinguished Line 441: # by existence/absence of the 'devices' key Line 442: devices = {} Line 443: # Build devices structure Could it be better if we use "is" instead of "==" here? Line 444: if self.conf.get('devices') == None: Line 445: self.conf['devices'] = [] Line 446: devices[DISK_DEVICES] = self.getConfDrives() Line 447: devices[NIC_DEVICES] = self.getConfNetworkInterfaces()
Line 597: """ Line 598: Reserve the required memory for this VM. Line 599: """ Line 600: self.memCommitted = ((2 ** 20) * (int(self.conf['memSize'])) + Line 601: config.getint('vars', 'guest_ram_overhead')) The original code means (2**20) * (... + ...)
The modified code means ((2**20) * (...) + (...))
Seems different, is this intentional? Line 602: Line 603: def _startUnderlyingVm(self): Line 604: self.log.debug("Start") Line 605: try:
Line 767: if d.name != block_dev: Line 768: continue Line 769: if newsize is None: Line 770: apperantSizeMB = int(math.ceil(d.apparentsize / (2.0 ** 20.0))) Line 771: newsize = self.volumeUtilizationChunk + apperantSizeMB As Dan says, this constant is better be put in another patch. Line 772: # TODO cap newsize by max volume size Line 773: volDict = {'poolID': d.poolID, 'domainID': d.domainID, Line 774: 'imageID': d.imageID, 'volumeID': d.volumeID, Line 775: 'name': d.name}
Line 903: self._guestEventTime = now Line 904: self._guestEvent = 'Powering down' Line 905: self.log.debug('guestAgent shutdown called') Line 906: self.guestAgent.desktopShutdown(timeout, message) Line 907: agent_timeout = int(timeout) + self.systemShutdownTimeout Same as line 770. Line 908: timer = threading.Timer(agent_timeout, self._timedShutdown) Line 909: timer.start() Line 910: elif utils.tobool(self.conf.get('acpiEnable', 'true')): Line 911: self._guestEventTime = now
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: Fix some PEP8 violations in vm.py so the pep8 tool would stop complaining ......................................................................
Patch Set 3: Do not submit
I believe vm.py is now good to go, as far as pep8 can tell.
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Saggi Mizrahi has abandoned this change.
Change subject: Fix some PEP8 violations in vm.py so the pep8 tool would stop complaining ......................................................................
Patch Set 3: Abandoned
-- To view, visit http://gerrit.ovirt.org/7449 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon Gerrit-Change-Id: I369a1dbfcf49cf1a8b1ac61fe88dd2fe3ea93b8e Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org
-
agl＠us.ibm.com -
Dan Kenigsberg -
Jenkins CI RO -
smizrahi＠redhat.com -
zhshzhou＠linux.vnet.ibm.com