Edward Haas has posted comments on this change.

Change subject: draft: migration plugin proposal - draft 102
......................................................................


Patch Set 1: Code-Review-1

(11 comments)

https://gerrit.ovirt.org/#/c/64186/1/vdsm/virt/vm_migrate_hook.py
File vdsm/virt/vm_migrate_hook.py:

PS1, Line 87: target_vm_conf_by_mac
Can you add the 'vnic' part? ..by_vnic_mac
If we do not use 'vnic', it is hard to understand which mac is it all about.


PS1, Line 100: interface_conf
How about: 'target_vm_iface_conf' ?
Just a nit to keep the convention.


Line 98:     mac_addr = elem_macaddr.get('address')
Line 99: 
Line 100:     interface_conf = target_vm_conf_by_mac[mac_addr]
Line 101:     target_vm_net = interface_conf['network']
Line 102:     if not vm_migrate_plugins.skip_processing(interface, 
interface_conf):
It's a bit nicer to go with the positive one and just bail out earlier with a 
return.
It saves a nasty indentation. Influenced by Dan :)

And please separate this one by newline above and below, so we will better 
notice it. It does something unexpected.
Line 103:         target_ovs_bridge = net_api.ovs_bridge(target_vm_net)
Line 104:         if target_ovs_bridge:
Line 105:             _bind_iface_to_ovs_bridge(interface, target_ovs_bridge,
Line 106:                                       target_vm_net)


https://gerrit.ovirt.org/#/c/64186/1/vdsm/virt/vm_migrate_plugins/__init__.py
File vdsm/virt/vm_migrate_plugins/__init__.py:

PS1, Line 27: _DRIVERS
_PLUGINS


Line 26: 
Line 27: _DRIVERS = []
Line 28: 
Line 29: 
Line 30: def skip_processing(domxml, device_conf):
domxml_iface or something similar.
vm_iface_conf

And a docstring is in order here, explaining what is inputted and what it does.
Line 31:     for driver in _DRIVERS:
Line 32:         driver.create().skip_processing(domxml, device_conf)
Line 33: 
Line 34: 


Line 31:     for driver in _DRIVERS:
Line 32:         driver.create().skip_processing(domxml, device_conf)
Line 33: 
Line 34: 
Line 35: class VmMigratePlugin():
You will need a decorator:
@six.add_metaclass(abc.ABCMeta)

BTW: You do not really need this class at all, you could use a plugin_init() or 
plugin_run() func that is an entry point, replacing create().
Line 36: 
Line 37:     @abc.abstractmethod
Line 38:     def skip_processing(self, domxml, device_conf):
Line 39:         return False


https://gerrit.ovirt.org/#/c/64186/1/vdsm/virt/vm_migrate_plugins/external_network_plugin.py
File vdsm/virt/vm_migrate_plugins/external_network_plugin.py:

PS1, Line 1: #!/usr/bin/env python
drop this please


PS1, Line 28: domxml, conf
I think you should specify what part of domxml and conf we are talking about, 
it is not clear when looking here.


Line 26:     EXTERNAL_NETWORK = 'EXTERNAL_NETWORK'
Line 27: 
Line 28:     def skip_processing(self, domxml, conf):
Line 29:         custom = conf.get('custom')
Line 30:         return custom.get('provider_type') == self.EXTERNAL_NETWORK \
Maybe extract the first one to a variable so you will not need '\'.
But with Dan comment, you will probably not need the if part at all.
Line 31:             if custom else False
Line 32: 
Line 33: 
Line 34: def create():


https://gerrit.ovirt.org/#/c/64186/1/vdsm/virt/vm_migrate_plugins/openstack_network_plugin.py
File vdsm/virt/vm_migrate_plugins/openstack_network_plugin.py:

PS1, Line 1: #!/usr/bin/env python
drop this please


https://gerrit.ovirt.org/#/c/64186/1/vdsm/virt/vm_migrate_plugins/vm_fex_plugin.py
File vdsm/virt/vm_migrate_plugins/vm_fex_plugin.py:

PS1, Line 1: #!/usr/bin/env python
drop this please


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0fb056c4b4732505d567385546e78048c37d140
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki <mmire...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Edward Haas <edwa...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to