Francesco Romani has posted comments on this change.
Change subject: vdsm: add support for S3/S4 suspend calls
......................................................................
Patch Set 8: Code-Review-1
(10 comments)
Mostly minor things and a few questions
http://gerrit.ovirt.org/#/c/19389/8/client/vdsClient.py
File client/vdsClient.py:
Line 2461: 'channel'
Line 2462: )),
Line 2463: 'wakeup': (serv.wakeup,
Line 2464: ('<vmId>',
Line 2465: 'Wakes up the desktop'
I guess 'desktop' here is for consistency with the hibernate verb below, right?
Unrelated to this patch: I think we should change the naming.
Line 2466: )),
Line 2467: 'hibernate': (serv.hibernate,
Line 2468: ('<vmId> <target>',
Line 2469: 'Hibernates the desktop'
http://gerrit.ovirt.org/#/c/19389/8/tests/functional/utils.py
File tests/functional/utils.py:
Line 226:
Line 227: def getVmList(self, vmId):
Line 228: result = self.vdscli.list('true', [vmId])
Line 229: return result['status']['code'],
result['status']['message'],\
Line 230: result['vmList'][0]
Why do we need this? looks like a clone of the method above...
http://gerrit.ovirt.org/#/c/19389/8/vdsm/API.py
File vdsm/API.py:
Line 387: v = self._cif.vmContainer[self._UUID]
Line 388: response = v.pmSuspend(target)
Line 389: except KeyError:
Line 390: return errCode['noVM']
Line 391: else:
I'd like to see an explicit check for 'hiberVolHandle' here, just to be safe.
Line 392: params = {'vmId': self._UUID, 'mode':
'file',
Line 393: 'hiberVolHandle': target}
Line 394:
Line 395: response = self.migrate(params)
http://gerrit.ovirt.org/#/c/19389/8/vdsm/virt/vm.py
File vdsm/virt/vm.py:
Line 2302: return errCode['noVM']
Line 2303:
Line 2304: def pmWakeup(self):
Line 2305: self.log.debug('pmWakeup Called')
Line 2306: hooks.after_vm_dehibernate(self._dom.XMLDesc(0), self.conf)
Is this right? after_vm_dehibernate *before* wakeup?
If so please add one-line comment to reduce/avoid future confusion.
Line 2307: try:
Line 2308: self._dom.pMWakeup()
Line 2309: except:
Line 2310: self.log.error("pmWakeup failed", exc_info=True)
Line 2308: pMWakeup
This looks suspicious. AFAIK is PMWakeup(); however below you use pmWakeup().
Line 2311: return {'status': {'code':
errCode['exist']['status']['code'],
Line 2312: 'message': 'Failed to wake up VM.'}}
Line 2313: else:
Line 2314: self.log.debug('pmWakeup successful')
Line 2315: self.lastStatus = 'Up'
this needs rebase.
self.lastStatus = vmstatus.UP
Line 2316: hooks.before_vm_dehibernate(self._dom.XMLDesc(0), self.conf)
Line 2317:
Line 2318: return {'status': {'code': doneCode['code'],
Line 2319: 'message': 'Machine woken up'}}
Line 2339: self.log.error("pmSuspend failed", exc_info=True)
Line 2340: return {'status': {'code':
errCode['exist']['status']['code'],
Line 2341: 'message': 'Failed to suspend VM.'}}
Line 2342: else:
Line 2343: # We have to treat hybrid as standard S3
So it is impossible we get here if target is S4?
Line 2344: self.log.debug('pmSuspend succeeded')
Line 2345: if target in ('mem', 'hybrid'):
Line 2346: self.lastStatus = 'Paused'
Line 2347: self.conf['pauseCode'] =
targets[target]['reason']
Line 2342: else:
Line 2343: # We have to treat hybrid as standard S3
Line 2344: self.log.debug('pmSuspend succeeded')
Line 2345: if target in ('mem', 'hybrid'):
Line 2346: self.lastStatus = 'Paused'
same here for vmstatus
Line 2347: self.conf['pauseCode'] =
targets[target]['reason']
Line 2348: else:
Line 2349: self.setDownStatus(NORMAL, targets[target]['reason'])
Line 2350: hooks.after_vm_hibernate(self._dom.XMLDesc(0), self.conf,
http://gerrit.ovirt.org/#/c/19389/8/vdsm_api/vdsmapi-schema.json
File vdsm_api/vdsmapi-schema.json:
Line 5924: # Wake up suspended VM
Line 5925: #
Line 5926: # @vmID: The UUID of the VM
Line 5927: #
Line 5928: # Since: 4.10.0
4.15.0 I guess
Line 5929: ##
Line 5930: {'command': {'class': 'VM', 'name':
'wakeup'},
Line 5931: 'data': {'vmID': 'UUID'}}
Line 5932:
Line 5967: # @vmID: The UUID of the VM
Line 5968: #
Line 5969: # @target: Target suspension mode
Line 5970: #
Line 5971: # Since: 4.12.0
same here
Line 5972: ##
Line 5973: {'command': {'class': 'VM', 'name':
'hibernate'},
Line 5974: 'data': {'vmID': 'UUID', 'target':
'hibernationTarget'}}
Line 5975:
--
To view, visit
http://gerrit.ovirt.org/19389
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic30016c5cd555f5771dde8db3f1340e1c11b3da7
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpoledni(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Better Saggi <bettersaggi(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Martin Polednik <mpoledni(a)redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes