Change in vdsm[master]: draft: migration plugin proposal - draft 102

2016-09-20 Thread edwardh 
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 Mirecki 
Gerrit-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

2016-09-20 Thread fromani 
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 Mirecki 
Gerrit-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

2016-09-20 Thread edwardh 
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 
Gerrit-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

2016-09-20 Thread danken 
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 Mirecki 
Gerrit-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

2016-09-20 Thread mmirecki 
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

2016-09-20 Thread automation 
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 Mirecki 
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