Dan Kenigsberg has posted comments on this change. Change subject: set the # of vcpus for a VM (hot plug/unplug) ......................................................................
Patch Set 15: Code-Review-1 (14 comments) .................................................... Commit Message Line 4: Commit: Roy Golan <[email protected]> Line 5: CommitDate: 2014-01-02 13:51:18 +0200 Line 6: Line 7: set the # of vcpus for a VM (hot plug/unplug) Line 8: We have bug-url. This line is redundant. This is a new important feature. It deserves a better explanation and reasoning. Line 9: RFE: https://bugzilla.redhat.com/show_bug.cgi?id=1036492 Line 10: Line 11: Change-Id: Ief35e1d335737cd98d21a5413ac9f8ab9d824c3e Line 12: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1036492 .................................................... File debian/vdsm.dirs Line 51: usr/libexec/vdsm/hooks/before_vm_migrate_source Line 52: usr/libexec/vdsm/hooks/before_vm_pause Line 53: usr/libexec/vdsm/hooks/before_vm_set_ticket Line 54: usr/libexec/vdsm/hooks/before_vm_start Line 55: usr/libexec/vdsm/hooks/before_set_num_of_cpus Keep sorted Line 56: usr/libexec/vdsm/hooks/after_set_num_of_cpus Line 57: var/lib/libvirt/qemu/channels Line 58: var/lib/vdsm Line 59: var/lib/vdsm/netconfback .................................................... File lib/vdsm/define.py Line 131: 'message': 'Wrong resize disk parameter'}}, Line 132: 'transientErr': {'status': { Line 133: 'code': 59, Line 134: 'message': 'Action not permitted on a VM with transient disks'}}, Line 135: 'setNumberOfCpus': {'status': { All the error names end with "err". Please conform. Line 136: 'code': 60, Line 137: 'message': 'Failed to set the number of cpus'}}, Line 138: 'recovery': {'status': { Line 139: 'code': 99, .................................................... File tests/vmTests.py Line 55: converted = element.toprettyxml() Line 56: else: Line 57: elem = ET.fromstring(element.toprettyxml()) Line 58: converted = re.sub(' />', '/>', Line 59: ET.tostring(elem.find("./%s" % path))) Unrelated, it seems. Line 60: self.assertEqual( Line 61: re.sub('\n\s*', ' ', converted).strip(' '), Line 62: re.sub('\n\s*', ' ', expectedXML).strip(' ')) Line 63: .................................................... File tests/vmTestsData.py Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: CONF_TO_DOMXML = ({ Why was this changed? I'd like to see further elements added to this list. Line 22: 'vmId': '9ffe28b6-6134-4b1e-8804-1185f49c436f', Line 23: 'smp': '8', 'memSize': '1024', 'memGuaranteedSize': '512', Line 24: 'displayPort': '-1', 'vmName': 'testVm', Line 25: 'display': 'vnc', 'emulatedMachine': 'pc', .................................................... File vdsm/API.py Line 457: return curVm.hotunplugDisk(params) Line 458: Line 459: def setNumberOfCpus(self, vmId, numberOfCpus): Line 460: Line 461: if vmId is None or numberOfCpus is None: How can any of them be None? None cannot be transported over xmlrpc. Line 462: self.log.error( Line 463: 'Missing one of required parameters: vmId, numberOfCpus') Line 464: return {'status': {'code': errCode['MissParam']['status']['code'], Line 465: 'message': 'Missing one of required ' .................................................... File vdsm/hooks.py Line 297: Line 298: Line 299: def before_set_num_of_cpus(vmconf={}, params={}): Line 300: return _runHooksDir(None, 'before_set_num_of_cpus', vmconf=vmconf, Line 301: params=params, raiseError=False) "Before" Hooke *should* raise errors. Line 302: Line 303: Line 304: def after_set_num_of_cpus(vmconf={}, params={}): Line 305: return _runHooksDir(None, 'after_set_num_of_cpus', vmconf=vmconf, .................................................... File vdsm/vm.py Line 77: RNG_DEVICES = 'rng' Line 78: WATCHDOG_DEVICES = 'watchdog' Line 79: CONSOLE_DEVICES = 'console' Line 80: SMARTCARD_DEVICES = 'smartcard' Line 81: CPU_DEVICES = 'cpu' Is this ever used? Line 82: Line 83: Line 84: def isVdsmImage(drive): Line 85: return all(k in drive.keys() for k in ('volumeID', 'domainID', 'imageID', Line 897: memSizeKB = str(int(self.conf.get('memSize', '256')) * 1024) Line 898: self.dom.appendChildWithArgs('memory', text=memSizeKB) Line 899: self.dom.appendChildWithArgs('currentMemory', text=memSizeKB) Line 900: vcpu = self.dom.appendChildWithArgs( Line 901: 'vcpu', text=self.conf.get('maxVCpus', '160')) Please define 160 as a CONSTANT. Is there any side effect of always using the greatest maximum? Line 902: # hotplug cpu requires a maximum number which is higher than the static Line 903: # assignment. 160 is the max number of cpu's for a guest Line 904: # see http://www.ovirt.org/Hot_plug_cpu Line 905: vcpu.setAttrs(**{'current': self.conf['smp'], 'placement': 'static'}) Line 901: 'vcpu', text=self.conf.get('maxVCpus', '160')) Line 902: # hotplug cpu requires a maximum number which is higher than the static Line 903: # assignment. 160 is the max number of cpu's for a guest Line 904: # see http://www.ovirt.org/Hot_plug_cpu Line 905: vcpu.setAttrs(**{'current': self.conf['smp'], 'placement': 'static'}) Currently we protect for a case were 'smp' is not specified at all and defaults to 1. Line 906: Line 907: memSizeGuaranteedKB = str(1024 * int( Line 908: self.conf.get('memGuaranteedSize', '0') Line 909: )) Line 1060: maxVCpus = int(self.conf.get('maxVCpus', '160')) Line 1061: cores = int(self.conf.get('smpCoresPerSocket', '1')) Line 1062: threads = int(self.conf.get('smpThreadsPerCore', '1')) Line 1063: cpu.appendChildWithArgs('topology', Line 1064: sockets=str(maxVCpus / cores / threads), this mean that with old Engine, this vdsm would suddenly create many sockets. Have you considered averse effects on guest apps with license that counts sockets? Line 1065: cores=str(cores), threads=str(threads)) Line 1066: Line 1067: #CPU-pinning support Line 1068: # see http://www.ovirt.org/wiki/Features/Design/cpu-pinning Line 3372: hooks.after_nic_hotunplug(nicXml, self.conf, Line 3373: params=customProps) Line 3374: return {'status': doneCode, 'vmList': self.status()} Line 3375: Line 3376: def setNumberOfCpus(self, numberOfCpus): It would be much nicer if in this level, int is used, not str. Line 3377: Line 3378: if self.isMigrating(): Line 3379: return errCode['migInProgress'] Line 3380: Line 3380: Line 3381: self.log.debug("Setting number of cpus to : %s", numberOfCpus) Line 3382: hooks.before_set_num_of_cpus() Line 3383: try: Line 3384: self._dom.setVcpus(int(numberOfCpus)) This is going to fail for VMS started with no maxvcpu (before vdsm upgrade, on another host, etc). This is fine by me, but should be acknowledged in the commit message. Line 3385: except libvirt.libvirtError as e: Line 3386: self.log.error("setNumberOfCpus failed", exc_info=True) Line 3387: if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: Line 3388: return errCode['noVM'] .................................................... File vdsm_api/vdsmapi-schema.json Line 6723: # Line 6724: # Returns: Line 6725: # The VM definition, as updated Line 6726: # Line 6727: # Since: 4.10.0 Most probably new in 4.14. Line 6728: ## Line 6729: {'command': {'class': 'VM', 'name': 'setNumberOfCpus'}, Line 6730: 'data': {'vmID': 'UUID', 'numberOfCpus': 'int'}, -- To view, visit http://gerrit.ovirt.org/21789 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ief35e1d335737cd98d21a5413ac9f8ab9d824c3e Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Roy Golan <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
