Shahar Havivi has posted comments on this change. Change subject: External hypervisor VMs integration ......................................................................
Patch Set 12: (12 comments) Thanks for reviewing Nir. http://gerrit.ovirt.org/#/c/33309/12/vdsm/API.py File vdsm/API.py: Line 1384: def getExternalVMList(self, uri, username, password): Line 1385: "return information about the not-KVM virtual machines" Line 1386: vms = v2v.getExternalVMList(uri, username, password) Line 1387: if vms is None: Line 1388: return errCode['externalErr'] > Returning None for error is little ugly - why not raise an exception with t Done Line 1389: return {'status': doneCode, 'vmList': vms} Line 1390: Line 1391: # Networking-related functions Line 1392: def setupNetworks(self, networks, bondings, options): http://gerrit.ovirt.org/#/c/33309/12/vdsm/rpc/Bridge.py File vdsm/rpc/Bridge.py: Line 329: """ Line 330: Return list of VMs from non-KVM source Line 331: """ Line 332: uri = args.get('uri', None) Line 333: username = args.get('username', None) > Noe is the default value, there is no need to repeat that here. Done Line 334: password = args.get('password', None) Line 335: return API.Global().getExternalVMList(uri, username, password) Line 336: Line 337: http://gerrit.ovirt.org/#/c/33309/12/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3630: # @ExternalVmInfo: Line 3631: # Line 3632: # map of VM properties Line 3633: # Line 3634: # @vmName: The name of the VM > Why not "name"? (ExternalVmInfo.vmName - do we have another type of name he we are using the identical names as @VMFullInfo Line 3635: # Line 3636: # @status: The VM status (up, down, etc) Line 3637: # Line 3638: # @vmId: The VM uuid Line 3632: # map of VM properties Line 3633: # Line 3634: # @vmName: The name of the VM Line 3635: # Line 3636: # @status: The VM status (up, down, etc) > Here you agree with me that there is no need to repeat the "vm" prefix :-) same as above :) Line 3637: # Line 3638: # @vmId: The VM uuid Line 3639: # Line 3640: # @smp: number of cpu Line 3634: # @vmName: The name of the VM Line 3635: # Line 3636: # @status: The VM status (up, down, etc) Line 3637: # Line 3638: # @vmId: The VM uuid > Why not "id"? same as above :) Line 3639: # Line 3640: # @smp: number of cpu Line 3641: # Line 3642: # @memSize: size of actual memory in MB http://gerrit.ovirt.org/#/c/33309/12/vdsm/v2v.py File vdsm/v2v.py: Line 32: libvirt.VIR_DOMAIN_CRASHED: 'Crushed', Line 33: libvirt.VIR_DOMAIN_PMSUSPENDED: 'PMSuspended'} Line 34: Line 35: Line 36: def _hypervisorConnect(uri, username, password): > This connects to another hypervisor using libvirt? Yes, we do want to connect to VMWare esx for start as noted in this mail: http://lists.ovirt.org/pipermail/devel/2014-September/008773.html I will inline this method. Line 37: conn = libvirtconnection.open_connection(uri=uri, username=username, Line 38: passwd=password) Line 39: return conn Line 40: Line 35: Line 36: def _hypervisorConnect(uri, username, password): Line 37: conn = libvirtconnection.open_connection(uri=uri, username=username, Line 38: passwd=password) Line 39: return conn > Why not: Done Line 40: Line 41: Line 42: def _memToMb(size, unit): Line 43: if unit == 'bytes' or unit == 'b': Line 51: elif unit == 'Tib' or unit == 'T': Line 52: return size * 1024 * 1024 Line 53: Line 54: Line 55: def _parseVmElements(xml): > This does not get elements - right? it accepts an xml document. Done Line 56: ret = {} Line 57: root = ET.fromstring(xml) Line 58: Line 59: ''' VM ID ''' Line 88: if source is not None and 'file' in source.attrib: Line 89: d['alias'] = source.attrib['file'] Line 90: ret['disks'].append(d) Line 91: Line 92: ''' Network ''' > The need to add these (non-standard) comments (should use # Networks) show Done Line 93: ret['networks'] = [] Line 94: interfaces = root.findall('.//interface') Line 95: for iface in interfaces: Line 96: i = {} Line 108: ret['networks'].append(i) Line 109: return ret Line 110: Line 111: Line 112: def getExternalVMList(uri, username, password): > This is the only public API here - why not put this on the top of the file? right, I am going to change that to class. Line 113: try: Line 114: conn = _hypervisorConnect(uri, username, password) Line 115: if conn is None: Line 116: return None Line 110: Line 111: Line 112: def getExternalVMList(uri, username, password): Line 113: try: Line 114: conn = _hypervisorConnect(uri, username, password) > If _hypervisorConnect fails, conn is not defined, and conn.close() in final This is a different connection - not the local qemu connection ("qemu:///system") but other such as esx://myhypervisor. this is a short connection for probing VMs in remove hytpervisior, the connection will not use a long connection such as monitoring a VM. Line 115: if conn is None: Line 116: return None Line 117: Line 118: ret = [] Line 118: ret = [] Line 119: for vm in conn.listAllDomains(): Line 120: params = _parseVmElements(vm.XMLDesc(0)) Line 121: params['vmName'] = vm.name() Line 122: params['status'] = _VM_STATUS[vm.state()[0]] > There are 2 functions here building the params dict - this function, and _p it does make sense, I didn't find where there is duplication, VDSM usually build xml for libvirt and here we parse it back. Line 123: ret.append(params) Line 124: return ret Line 125: finally: -- To view, visit http://gerrit.ovirt.org/33309 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7dcfb860626a844d1d08590274b508519a33f4a3 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [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
