Change in vdsm[master]: v2v: externalVMList Xen+Kvm support

nsoffer at redhat.com nsoffer at redhat.com
Sun Nov 29 14:09:54 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: v2v: externalVMList Xen+Kvm support
......................................................................


Patch Set 7: Code-Review-1

(3 comments)

https://gerrit.ovirt.org/#/c/48672/7/vdsm/v2v.py
File vdsm/v2v.py:

Line 154:         #       under RHEL 5.x
Line 155:         for name in conn.listDefinedDomains():
Line 156:             _add_vm(conn, vms, conn.lookupByName(name))
Line 157: 
Line 158:         for vmid in conn.listDomainsID():
listDomains()? there is no such function listDomainsID() in libvirt docs:
https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectListDomains
Line 159:             _add_vm(conn, vms, conn.lookupByID(vmid))
Line 160: 
Line 161:         return {'status': doneCode, 'vmList': vms}
Line 162: 


Line 155:         for name in conn.listDefinedDomains():
Line 156:             _add_vm(conn, vms, conn.lookupByName(name))
Line 157: 
Line 158:         for vmid in conn.listDomainsID():
Line 159:             _add_vm(conn, vms, conn.lookupByID(vmid))
Why do we need  lookup both by name and by id?

It seem that we are adding the same vm twice, if it apears in both lists.

What we need is a backward compatible way to iterate on the vms, that works with both new and old libvirt on the backend.

With new libvirt, we want to use listAllDomains(), with old libvirt, listDefinedDomains() or listDomains(), but we want to return a vm object.

Assuming that we can use one of them, we can add a method returning all vm objects:

    def _list_domains(conn):
        try:
            for vm in conn.listAllDomains():
                yield vm
        except libvirt.libvirtError as e:
            # TODO: remove when...
            if e.get_error_code() != libvirt.VIR_ERR_NO_SUPPORT:
                raise
            seen = set()
            for name in conn.listDefinedDomains():
                vm = conn.lookupByName(name)
                seen.add(vm.UUIDString())
                yield vm
            for vmid in conn.listDomainsID():
                if vmid in seen:
                    continue
                vm = conn.lookupByID(vmid)
                seen.add(vmid)
                yield vm

This may be wrong, since I don't fully understand why do you need to use both listDefinedDomains and listDomains, but it shows how to avoid duplicates.

Now the verb can do on all versions, keeping the logic clear, and the backward compatibility shim hidden.

    for vm in _list_domains(conn):
        ...

Also the iteration code does not have to know about the vms dict, only about the connection object.
Line 160: 
Line 161:         return {'status': doneCode, 'vmList': vms}
Line 162: 
Line 163: 


Line 639:         raise InvalidVMConfiguration("Invalid currentMemory unit attribute:"
Line 640:                                      " %r" % unit)
Line 641: 
Line 642: 
Line 643: def _add_vm(conn, vms, vm):
Nice change, separating lookup and action taken on a vm make the code simpler and more robust.
Line 644:     params = {}
Line 645:     _add_vm_info(vm, params)
Line 646:     try:
Line 647:         xml = vm.XMLDesc(0)


-- 
To view, visit https://gerrit.ovirt.org/48672
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic7d7e211a9343a528f260da2686b34cea00c53a4
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list