Nir Soffer has posted comments on this change. Change subject: External hypervisor VMs integration ......................................................................
Patch Set 14: (12 comments) I think this version is a regression. http://gerrit.ovirt.org/#/c/33309/14/vdsm/v2v.py File vdsm/v2v.py: Line 34: libvirt.VIR_DOMAIN_CRASHED: 'Crushed', Line 35: libvirt.VIR_DOMAIN_PMSUSPENDED: 'PMSuspended'} Line 36: Line 37: Line 38: class V2V(object): This is a bad name. Not only it repeats the module name "v2v.V2V", it also does not mean anything. The simplest fix would be to eliminate this unneeded class - the previous version using module and some functions was better. Line 39: _instance = None Line 40: _instanceLock = threading.Lock() Line 41: Line 42: @classmethod Line 43: def getInstance(cls): Line 44: with cls._instanceLock: Line 45: if cls._instance is None: Line 46: cls._instance = V2V() Line 47: return cls._instance If you want a singleton, the best way to do it is to use a module, like you had before. Even if you use classes, there is no need to expose this java-like mechanism to the outside world. Simply do: # v2v.py class _V2V: ... _instance = _V2V() def getExternalVMList(...): _instance.getExternalVMList() Client code is much nicer now: # user.py import v2v v2v.getExternalVMList() And we don't need any locks. But in this case, this class is has no state, and it does not help anyone. Line 48: Line 49: def getExternalVMList(self, uri, username, password): Line 50: try: Line 51: conn = libvirtconnection.open_connection(uri=uri, Line 57: root = ET.fromstring(vm.XMLDesc(0)) Line 58: params = {} Line 59: params['vmName'] = vm.name() Line 60: params['status'] = _VM_STATUS[vm.state()[0]] Line 61: params.update(self._findGeneralInfo(root)) This would be better if you pass params to the functions and let them add what they want into the dict. Then we can rename them to _addGeneralInfo(root, params) This way, there is single place where a key and its contents are generated. Line 62: params.update(self._findDisks(root)) Line 63: params.update(self._findNetworks(root)) Line 64: ret.append(params) Line 65: return ret Line 59: params['vmName'] = vm.name() Line 60: params['status'] = _VM_STATUS[vm.state()[0]] Line 61: params.update(self._findGeneralInfo(root)) Line 62: params.update(self._findDisks(root)) Line 63: params.update(self._findNetworks(root)) We did not solve yet the fact that you add keys to this dict in two places, here and in the various findXXX methods. It will work if the functions return single value. Line 64: ret.append(params) Line 65: return ret Line 66: except libvirt.libvirtError as e: Line 67: raise V2VException(e.code, e.message) Line 63: params.update(self._findNetworks(root)) Line 64: ret.append(params) Line 65: return ret Line 66: except libvirt.libvirtError as e: Line 67: raise V2VException(e.code, e.message) This looses the original stacktrace - not sure if it is useful, but bad idea in general. If you cannot handle this error, let the caller handle it. Line 68: Line 69: def _memToMb(self, size, unit): Line 70: if unit == 'bytes' or unit == 'b': Line 71: return size / 1024 / 1024 Line 66: except libvirt.libvirtError as e: Line 67: raise V2VException(e.code, e.message) Line 68: Line 69: def _memToMb(self, size, unit): Line 70: if unit == 'bytes' or unit == 'b': Can be little bit nicer as: if unit in ('bytes', 'b'): ... Line 71: return size / 1024 / 1024 Line 72: if unit == 'KiB' or unit == 'k': Line 73: return size / 1024 Line 74: elif unit == 'MiB' or unit == 'M': Line 75: return size Line 76: elif unit == 'GiB' or unit == 'G': Line 77: return size * 1024 Line 78: elif unit == 'Tib' or unit == 'T': Line 79: return size * 1024 * 1024 else ? If not valid input, raise. If valid, this must return a size - right? Line 80: Line 81: def _findGeneralInfo(self, root): Line 82: ret = {} Line 83: e = root.find('./uuid') Line 89: size = int(e.text) Line 90: if 'unit' in e.attrib: Line 91: unit = e.attrib['unit'] Line 92: else: Line 93: unit = 'KiB' This is cleaner and more efficient: unit = e.get('unit', 'KiB') Line 94: ret['memSize'] = self._memToMb(size, unit) Line 95: Line 96: e = root.find('./vcpu') Line 97: if e is not None: Line 94: ret['memSize'] = self._memToMb(size, unit) Line 95: Line 96: e = root.find('./vcpu') Line 97: if e is not None: Line 98: ret['smp'] = int(e.text) Do you like to fail the entire request if the value is not valid integer? If you do, do you really want to fail with ValueError? This error means usually that there is a bug in the program, but this is obviously bug in the client that send this value. When you parse invalid data, you want to fail with an error from the application domain, such as InvalidVMConfiguration("bad vcpu value: 'bad value'") When you raise such error, it is easy to handle it in the upper layer by logging an error and returning error response. There is not need for logging exceptions in this case since we know exactly what is the problem. When you use ValueError, the upper layer must log an exception because this error may be bug in the code. This cause people to open bugs for vdsm because they found a scary exception in vdsm log. Line 99: return ret Line 100: Line 101: def _findDisks(self, root): Line 102: ret = {} Line 99: return ret Line 100: Line 101: def _findDisks(self, root): Line 102: ret = {} Line 103: ret['disks'] = [] Would be nicer to collect the data and create the dict at the end: disks = [] for disk in disks: ... disks.append(...) return {'disks': disks} Same for other methods here. Line 104: disks = root.findall('.//disk[@type="file"]') Line 105: for disk in disks: Line 106: d = {} Line 107: target = disk.find('./target') Line 104: disks = root.findall('.//disk[@type="file"]') Line 105: for disk in disks: Line 106: d = {} Line 107: target = disk.find('./target') Line 108: if target is not None and 'dev' in target.attrib: This can be replaced with: target = disk.find('./target[@dev]') And for getting values, it is more efficient to use get(). Using attrib the library must create a new Python dictionary, but we want only one of the values - so: d['dev'] = target.get('dev') Same for the code bellow. Line 109: d['dev'] = target.attrib['dev'] Line 110: source = disk.find('./source') Line 111: if source is not None and 'file' in source.attrib: Line 112: d['alias'] = source.attrib['file'] Line 133: ret['networks'].append(i) Line 134: return ret Line 135: Line 136: Line 137: class V2VException(Exception): Can be removed since you do not have any module specific error. Line 138: code = 0 Line 139: message = "V2V Exception" Line 140: Line 141: def __init__(self, code=0, message='V2V Exception'): -- 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: 14 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
