Francesco Romani has posted comments on this change. Change subject: PATCH: external VMs integration ......................................................................
Patch Set 4: (11 comments) good start. We can improve some things. One of the biggest problems is the lack of unit tests, please add. http://gerrit.ovirt.org/#/c/33309/4//COMMIT_MSG Commit Message: Line 7: PATCH: external VMs integration Line 8: Line 9: Added v2v module - added ability to list VMs from external source Line 10: (non KVM) via libvirt. Line 11: this is a bit terse. OK for a draft, but you'll need to expand it for review, (shortly) documenting the use cases and why this code is needed. Line 12: Change-Id: I7dcfb860626a844d1d08590274b508519a33f4a3 http://gerrit.ovirt.org/#/c/33309/4/client/vdsClient.py File client/vdsClient.py: Line 1890: return status['status']['code'], status['status']['message'] Line 1891: Line 1892: def getExternalVMList(self, args): Line 1893: if len(args) != 3: Line 1894: raise ValueError('Wrong number of arguments') not so common but won't hurt Line 1895: uri, username, password = args Line 1896: status = self.s.getExternalVMList(uri, username, password) Line 1897: if status['status']['code'] == 0: Line 1898: vmList = status['vmList'] Line 1896: status = self.s.getExternalVMList(uri, username, password) Line 1897: if status['status']['code'] == 0: Line 1898: vmList = status['vmList'] Line 1899: for vm in vmList: Line 1900: for key in vm.keys(): if vm is a a dict(): for key, val in vm.iteritems(): # or items() print key + ': ' + str(val) Line 1901: print key + ': ' + str(vm[key]) Line 1902: Line 1903: return status['status']['code'], status['status']['message'] Line 1904: http://gerrit.ovirt.org/#/c/33309/4/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 3588: # Line 3589: # Since: 4.17.0 Line 3590: ## Line 3591: {'map': 'VmProperties', Line 3592: 'key': 'str', 'value': 'str'} If this is a fixed set, so if we expect a fixed number of keys, better to document that Line 3593: Line 3594: ## Line 3595: # @VMFullInfo: Line 3596: # http://gerrit.ovirt.org/#/c/33309/4/vdsm/v2v.py File vdsm/v2v.py: Line 1: import libvirt missing copyright notice Line 2: import logging Line 3: import xml.etree.ElementTree as ET Line 4: Line 5: from virt import vmstatus Line 1: import libvirt Line 2: import logging Line 3: import xml.etree.ElementTree as ET Line 4: Line 5: from virt import vmstatus please add another space Line 6: Line 7: vmStatus = {libvirt.VIR_DOMAIN_NOSTATE: 'Unknown', Line 8: libvirt.VIR_DOMAIN_RUNNING: vmstatus.UP, Line 9: libvirt.VIR_DOMAIN_BLOCKED: 'Blocked', Line 3: import xml.etree.ElementTree as ET Line 4: Line 5: from virt import vmstatus Line 6: Line 7: vmStatus = {libvirt.VIR_DOMAIN_NOSTATE: 'Unknown', constants SHOULD_BE_UPPERCASE if they are private to module, _PREFIXED_WITH_UNDERSCORE Line 8: libvirt.VIR_DOMAIN_RUNNING: vmstatus.UP, Line 9: libvirt.VIR_DOMAIN_BLOCKED: 'Blocked', Line 10: libvirt.VIR_DOMAIN_PAUSED: vmstatus.PAUSED, Line 11: libvirt.VIR_DOMAIN_SHUTDOWN: vmstatus.POWERING_DOWN, Line 9: libvirt.VIR_DOMAIN_BLOCKED: 'Blocked', Line 10: libvirt.VIR_DOMAIN_PAUSED: vmstatus.PAUSED, Line 11: libvirt.VIR_DOMAIN_SHUTDOWN: vmstatus.POWERING_DOWN, Line 12: libvirt.VIR_DOMAIN_SHUTOFF: vmstatus.DOWN, Line 13: libvirt.VIR_DOMAIN_CRASHED: 'Crushed', typo :) Line 14: libvirt.VIR_DOMAIN_PMSUSPENDED: 'PMSuspend'} Line 15: Line 16: Line 17: def memToMb(size, unit): Line 26: elif unit == 'Tib' or unit == 'T': Line 27: return size * 1024 * 1024 Line 28: Line 29: Line 30: def parseVmElements(xml): same. If private, prefix with '_' _parseVmElements Line 31: ret = {} Line 32: ''' Memory ''' Line 33: root = ET.fromstring(xml) Line 34: e = root.find('./currentMemory') Line 79: Line 80: Line 81: def getExternalVMList(uri, username, password): Line 82: """ return external VMs with its state (up/down...) given uri """ Line 83: def req(credentials, user_data): can we share some code with libvirtconnection? We can probably reuse some of its facilities, or make them (more) general. Line 84: for cred in credentials: Line 85: if cred[0] == libvirt.VIR_CRED_AUTHNAME: Line 86: cred[4] = username Line 87: elif cred[0] == libvirt.VIR_CRED_PASSPHRASE: Line 89: return 0 Line 90: Line 91: auth = [[libvirt.VIR_CRED_AUTHNAME, libvirt.VIR_CRED_PASSPHRASE], Line 92: req, None] Line 93: try: why not openReadOnly? Line 94: conn = libvirt.openAuth(uri, auth, 0) Line 95: except libvirt.libvirtError as e: Line 96: logging.error( Line 97: 'v2v.getExternalVMList: error connection to remove server: %s' -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Francesco Romani <[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
