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_...
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/externa...
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/opensta...
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_...
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 <mmirecki(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes