Change in vdsm[master]: draft: migration plugin proposal - draft 102
Edward Haas has posted comments on this change. Change subject: draft: migration plugin proposal - draft 102 .. Patch Set 1: Marcin, please add an explanation in the commit message on why we had to use plugins for this, and not just embed it in the libvirt hook. -- 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 MireckiGerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: draft: migration plugin proposal - draft 102
Francesco Romani has posted comments on this change. Change subject: draft: migration plugin proposal - draft 102 .. Patch Set 1: just looking at the names, I have a very bad feeling about the idea of adding plugins to an hook. Seems a pretty bad smell to me. But this is just my very fist feeling, let me read the actual code before to give a real score. -- 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 MireckiGerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: draft: migration plugin proposal - draft 102
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 MireckiGerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: draft: migration plugin proposal - draft 102
Dan Kenigsberg has posted comments on this change. Change subject: draft: migration plugin proposal - draft 102 .. Patch Set 1: (3 comments) https://gerrit.ovirt.org/#/c/64186/1/vdsm/virt/vm_migrate_hook.py File vdsm/virt/vm_migrate_hook.py: Line 27: from vdsm import jsonrpcvdscli Line 28: from vdsm.config import config Line 29: from vdsm.network import api as net_api Line 30: Line 31: import vm_migrate_plugins I like this simplistic approach. but now the name vnic_migrate_plugins may better fit. Also, each plugin should be shipped as part of its relevant hook. Line 32: Line 33: Line 34: _DEBUG_MODE = False Line 35: LOG_FILE = '/tmp/libvirthook_ovs_migrate.log' https://gerrit.ovirt.org/#/c/64186/1/vdsm/virt/vm_migrate_plugins/__init__.py File vdsm/virt/vm_migrate_plugins/__init__.py: Line 18: # Line 19: Line 20: import abc Line 21: from importlib import import_module Line 22: from pkgutil import iter_modules please place new code under lib/vdsm Line 23: Line 24: import six Line 25: Line 26: 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: Line 25: Line 26: EXTERNAL_NETWORK = 'EXTERNAL_NETWORK' Line 27: Line 28: def skip_processing(self, domxml, conf): Line 29: custom = conf.get('custom') custom = conf.get('custom', {}) is much safer Line 30: return custom.get('provider_type') == self.EXTERNAL_NETWORK \ Line 31: if custom else False Line 32: Line 33: -- 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 MireckiGerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Edward Haas Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
Change in vdsm[master]: draft: migration plugin proposal - draft 102
Marcin Mirecki has uploaded a new change for review. Change subject: draft: migration plugin proposal - draft 102 .. draft: migration plugin proposal - draft 102 Just a draft to show the idea, not verified Change-Id: Ia0fb056c4b4732505d567385546e78048c37d140 Signed-off-by: mirecki--- M vdsm/virt/vm_migrate_hook.py A vdsm/virt/vm_migrate_plugins/__init__.py A vdsm/virt/vm_migrate_plugins/external_network_plugin.py A vdsm/virt/vm_migrate_plugins/openstack_network_plugin.py A vdsm/virt/vm_migrate_plugins/vm_fex_plugin.py 5 files changed, 161 insertions(+), 11 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/86/64186/1 diff --git a/vdsm/virt/vm_migrate_hook.py b/vdsm/virt/vm_migrate_hook.py index 633e4f0..dd276f5 100755 --- a/vdsm/virt/vm_migrate_hook.py +++ b/vdsm/virt/vm_migrate_hook.py @@ -28,6 +28,8 @@ from vdsm.config import config from vdsm.network import api as net_api +import vm_migrate_plugins + _DEBUG_MODE = False LOG_FILE = '/tmp/libvirthook_ovs_migrate.log' @@ -82,24 +84,28 @@ def _set_bridge_interfaces(devices, target_vm_conf): -target_vm_nets_by_vnic_mac = {dev['macAddr']: dev['network'] - for dev in target_vm_conf['devices'] - if dev.get('type') == 'interface'} +target_vm_conf_by_mac = {dev['macAddr']: dev + for dev in target_vm_conf['devices'] + if dev.get('type') == 'interface'} + for interface in devices.findall('interface'): if interface.get('type') == 'bridge': -_bind_iface_to_bridge(interface, target_vm_nets_by_vnic_mac) +_bind_iface_to_bridge(interface, target_vm_conf_by_mac) -def _bind_iface_to_bridge(interface, target_vm_nets_by_vnic_mac): +def _bind_iface_to_bridge(interface, target_vm_conf_by_mac): elem_macaddr = interface.find('mac') mac_addr = elem_macaddr.get('address') -target_vm_net = target_vm_nets_by_vnic_mac[mac_addr] -target_ovs_bridge = net_api.ovs_bridge(target_vm_net) -if target_ovs_bridge: -_bind_iface_to_ovs_bridge(interface, target_ovs_bridge, target_vm_net) -else: -_bind_iface_to_linux_bridge(interface, target_vm_net) +interface_conf = target_vm_conf_by_mac[mac_addr] +target_vm_net = interface_conf['network'] +if not vm_migrate_plugins.skip_processing(interface, interface_conf): +target_ovs_bridge = net_api.ovs_bridge(target_vm_net) +if target_ovs_bridge: +_bind_iface_to_ovs_bridge(interface, target_ovs_bridge, + target_vm_net) +else: +_bind_iface_to_linux_bridge(interface, target_vm_net) def _bind_iface_to_ovs_bridge(interface, target_ovs_bridge, target_vm_net): diff --git a/vdsm/virt/vm_migrate_plugins/__init__.py b/vdsm/virt/vm_migrate_plugins/__init__.py new file mode 100644 index 000..6c81eee --- /dev/null +++ b/vdsm/virt/vm_migrate_plugins/__init__.py @@ -0,0 +1,43 @@ +# Copyright 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + +import abc +from importlib import import_module +from pkgutil import iter_modules + +import six + + +_DRIVERS = [] + + +def skip_processing(domxml, device_conf): +for driver in _DRIVERS: +driver.create().skip_processing(domxml, device_conf) + + +class VmMigratePlugin(): + +@abc.abstractmethod +def skip_processing(self, domxml, device_conf): +return False + + +for _, module, _ in iter_modules([__path__[0]]): +_DRIVERS.append(import_module('{}.{}'.format(__name__, module))) diff --git a/vdsm/virt/vm_migrate_plugins/external_network_plugin.py b/vdsm/virt/vm_migrate_plugins/external_network_plugin.py new file mode 100644 index 000..6beb0b2 --- /dev/null +++ b/vdsm/virt/vm_migrate_plugins/external_network_plugin.py @@ -0,0 +1,35 @@ +#!/usr/bin/env python +# Copyright 2016 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +#
Change in vdsm[master]: draft: migration plugin proposal - draft 102
gerrit-hooks has posted comments on this change. Change subject: draft: migration plugin proposal - draft 102 .. Patch Set 1: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0']) -- 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 MireckiGerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org