Change in vdsm[master]: vdsm: Add force and timeout options to VM.shutdown

mbetak at redhat.com mbetak at redhat.com
Mon May 12 12:34:06 UTC 2014


Martin Betak has posted comments on this change.

Change subject: vdsm: Add force and timeout options to VM.shutdown
......................................................................


Patch Set 4:

(7 comments)

http://gerrit.ovirt.org/#/c/27054/4/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 124: 
Line 125:         ('max_outgoing_migrations', '3',
Line 126:             'Maximum concurrent outgoing migrations'),
Line 127: 
Line 128:         ('sys_shutdown_timeout', '120',
> Why is this needed? I don't get it.
The old default was too low. 10 seconds it too short a grace period for even  medium VMs.
Line 129:             'Destroy and shutdown timeouts (in sec) before completing the '
Line 130:             'action.'),
Line 131: 
Line 132:         ('user_shutdown_timeout', '30',


http://gerrit.ovirt.org/#/c/27054/4/vdsm/API.py
File vdsm/API.py:

Line 642:         if not delay:
Line 643:             delay = config.get('vars', 'user_shutdown_timeout')
Line 644:         if not message:
Line 645:             message = USER_SHUTDOWN_MESSAGE
Line 646:         if not timeout:
> IMO better: timeout is None
Not sure about this. A quick git grep for both variants reveals a strong prevalence of the `if not` form. Maybe Danken could clarify what is the preferred form.
Line 647:             timeout = config.get('vars', 'sys_shutdown_timeout')
Line 648:         return v.shutdown(delay, message, reboot, timeout, force)
Line 649: 
Line 650:     def _createSysprepFloppyFromInf(self, infFileBinary, floppyImage):


Line 643:             delay = config.get('vars', 'user_shutdown_timeout')
Line 644:         if not message:
Line 645:             message = USER_SHUTDOWN_MESSAGE
Line 646:         if not timeout:
Line 647:             timeout = config.get('vars', 'sys_shutdown_timeout')
> getint() ?
Done
Line 648:         return v.shutdown(delay, message, reboot, timeout, force)
Line 649: 
Line 650:     def _createSysprepFloppyFromInf(self, infFileBinary, floppyImage):
Line 651:         try:


http://gerrit.ovirt.org/#/c/27054/4/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 2301:         if self.lastStatus == vmstatus.DOWN:
Line 2302:             return errCode['noVM']
Line 2303: 
Line 2304:         delay = int(delay)
Line 2305:         timeout = int(timeout)
> this will become rendundant if we use getint (and it becomes a little bit n
Done
Line 2306: 
Line 2307:         self._guestEventTime = time.time()
Line 2308:         if reboot:
Line 2309:             self._guestEvent = vmstatus.REBOOT_IN_PROGRESS


Line 4326:         self.deleteVm()
Line 4327: 
Line 4328:         return {'status': doneCode}
Line 4329: 
Line 4330:     def doDestroy(self):
> I'd like a better name... Unfortunately I'm short of suggestions. I can jus
from the top of my head I can think of perhaps: 'performDestroy' or 'destroyDomain'?
Line 4331:         for dev in self._customDevices():
Line 4332:             hooks.before_device_destroy(dev._deviceXML, self.conf,
Line 4333:                                         dev.custom)
Line 4334: 


http://gerrit.ovirt.org/#/c/27054/4/vdsm_api/vdsmapi-schema.json
File vdsm_api/vdsmapi-schema.json:

Line 6420: #
Line 6421: # @reboot:   #optional True if reboot is desired, False for shutdown
Line 6422: #
Line 6423: # @timeout:  #optional Number of seconds to wait before trying next
Line 6424: #                      shutdown/reboot method.
> Please add a (new in version whatever) line here
Done
Line 6425: #
Line 6426: # @force:    #optional True if shutdown/reboot desired by any means necessary
Line 6427: #                      (forceful reboot/shutdown if all graceful methods fail)
Line 6428: #                      Default is False (i.e. graceful only).


Line 6423: # @timeout:  #optional Number of seconds to wait before trying next
Line 6424: #                      shutdown/reboot method.
Line 6425: #
Line 6426: # @force:    #optional True if shutdown/reboot desired by any means necessary
Line 6427: #                      (forceful reboot/shutdown if all graceful methods fail)
> ditto
Done
Line 6428: #                      Default is False (i.e. graceful only).
Line 6429: # Since: 4.10.0
Line 6430: ##
Line 6431: {'command': {'class': 'VM', 'name': 'shutdown'},


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I30180f9906eb45fa485aa7d773a436e62bd3c815
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Betak <mbetak at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list