Change in vdsm[master]: PATCH: external VMs integration

fromani at redhat.com fromani at redhat.com
Mon Oct 6 14:01:41 UTC 2014


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 <shavivi at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi 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