[PATCH 1/3] Generate libvirt's interface XML definition for vlanned bridge
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id. Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+) diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import ipaddr +import lxml.etree as ET +from lxml.builder import E # FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m) diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest from contextlib import closing +from lxml import etree import kimchi.server @@ -159,3 +160,8 @@ def patch_auth(): import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True))) -- 1.8.4.2
It creates a vlan interface on top of the given nic or bond interface, and bridge the vlan interface to a new created bridge, which is used to forward VM's traffic. So all packets transmitted from VM will be tagged before it departs from the physical network interface. Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/API.json | 6 ++++++ src/kimchi/model.py | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..07bf41e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -56,6 +56,12 @@ "interface": { "description": "The name of a network interface on the host", "type": "string" + }, + "vlan_id": { + "description": "Network's VLAN ID", + "type": "integer", + "maximum": 4094, + "minimum": 1 } } }, diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..b3589e9 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -58,6 +58,7 @@ from kimchi import config from kimchi import isoinfo from kimchi import netinfo from kimchi import network as knetwork +from kimchi import networkxml from kimchi import vnc from kimchi import xmlutils from kimchi.asynctask import AsyncTask @@ -65,7 +66,6 @@ from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter, MissingParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests -from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner from kimchi.screenshot import VMScreenshot @@ -806,7 +806,12 @@ class Model(object): if netinfo.is_bridge(iface): params['bridge'] = iface elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): - params['forward']['dev'] = iface + if params.get('vlan_id') is None: + params['forward']['dev'] = iface + else: + params['bridge'] = \ + self._create_vlanned_bridge(str(iface), + str(params['vlan_id'])) else: raise InvalidParameter("the interface should be bare nic, " "bonding or bridge device.") @@ -830,7 +835,7 @@ class Model(object): if connection == 'bridge': self._set_network_bridge(params) - xml = to_network_xml(**params) + xml = networkxml.to_network_xml(**params) try: network = conn.networkDefineXML(xml) @@ -888,8 +893,44 @@ class Model(object): if network.isActive(): raise InvalidOperation( "Unable to delete the active network %s" % name) + self._remove_vlanned_bridge(network) network.undefine() + def _get_vlanned_bridge_name(self, interface, vlan_id): + return '-'.join(('kimchi', interface, vlan_id)) + + def _is_vlanned_bridge(self, bridge): + return bridge.startswith('kimchi-') + + def _create_vlanned_bridge(self, interface, vlan_id): + bridge = self._get_vlanned_bridge_name(interface, vlan_id) + bridge_xml = networkxml.create_vlanned_bridge_xml(bridge, interface, + vlan_id) + conn = self.conn.get() + conn.changeBegin() + try: + vlanned_bridge = conn.interfaceDefineXML(bridge_xml) + vlanned_bridge.create() + except: + conn.changeRollback() + raise OperationFailed('Failed to create vlanned bridge') + else: + conn.changeCommit() + return bridge + + def _remove_vlanned_bridge(self, network): + try: + bridge = network.bridgeName() + except libvirt.libvirtError: + pass + else: + if self._is_vlanned_bridge(bridge): + conn = self.conn.get() + iface = conn.interfaceLookupByName(bridge) + if iface.isActive(): + iface.destroy() + iface.undefine() + def add_task(self, target_uri, fn, opaque=None): id = self.next_taskid self.next_taskid = self.next_taskid + 1 -- 1.8.4.2
On 01/02/2014 07:50 AM, Mark Wu wrote:
It creates a vlan interface on top of the given nic or bond interface, and bridge the vlan interface to a new created bridge, which is used to forward VM's traffic. So all packets transmitted from VM will be tagged before it departs from the physical network interface.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/API.json | 6 ++++++ src/kimchi/model.py | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..07bf41e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -56,6 +56,12 @@ "interface": { "description": "The name of a network interface on the host", "type": "string" + }, + "vlan_id": { + "description": "Network's VLAN ID", + "type": "integer", + "maximum": 4094, + "minimum": 1 } } }, diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..b3589e9 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -58,6 +58,7 @@ from kimchi import config from kimchi import isoinfo from kimchi import netinfo from kimchi import network as knetwork +from kimchi import networkxml from kimchi import vnc from kimchi import xmlutils from kimchi.asynctask import AsyncTask @@ -65,7 +66,6 @@ from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter, MissingParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests -from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner from kimchi.screenshot import VMScreenshot @@ -806,7 +806,12 @@ class Model(object): if netinfo.is_bridge(iface): params['bridge'] = iface elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): - params['forward']['dev'] = iface + if params.get('vlan_id') is None: + params['forward']['dev'] = iface + else: + params['bridge'] = \ + self._create_vlanned_bridge(str(iface), + str(params['vlan_id'])) else: raise InvalidParameter("the interface should be bare nic, " "bonding or bridge device.") @@ -830,7 +835,7 @@ class Model(object): if connection == 'bridge': self._set_network_bridge(params)
- xml = to_network_xml(**params) + xml = networkxml.to_network_xml(**params)
try: network = conn.networkDefineXML(xml) @@ -888,8 +893,44 @@ class Model(object): if network.isActive(): raise InvalidOperation( "Unable to delete the active network %s" % name) + self._remove_vlanned_bridge(network) network.undefine()
+ def _get_vlanned_bridge_name(self, interface, vlan_id): + return '-'.join(('kimchi', interface, vlan_id)) + + def _is_vlanned_bridge(self, bridge): + return bridge.startswith('kimchi-') + + def _create_vlanned_bridge(self, interface, vlan_id): + bridge = self._get_vlanned_bridge_name(interface, vlan_id) + bridge_xml = networkxml.create_vlanned_bridge_xml(bridge, interface, + vlan_id) + conn = self.conn.get() + conn.changeBegin() + try: + vlanned_bridge = conn.interfaceDefineXML(bridge_xml) + vlanned_bridge.create() + except: + conn.changeRollback() + raise OperationFailed('Failed to create vlanned bridge')
+ else: + conn.changeCommit() + return bridge +
You can avoid multiple indents by removing 'else' statement
+ def _remove_vlanned_bridge(self, network): + try: + bridge = network.bridgeName() + except libvirt.libvirtError: + pass
Log the information. kimchi_log.info() or kimchi_log.debug()
+ else: + if self._is_vlanned_bridge(bridge): + conn = self.conn.get() + iface = conn.interfaceLookupByName(bridge) + if iface.isActive(): + iface.destroy() + iface.undefine()
You can avoid multiple indents by removing 'else' statement
+ def add_task(self, target_uri, fn, opaque=None): id = self.next_taskid self.next_taskid = self.next_taskid + 1
On 01/03/2014 02:04 AM, Aline Manera wrote:
On 01/02/2014 07:50 AM, Mark Wu wrote:
It creates a vlan interface on top of the given nic or bond interface, and bridge the vlan interface to a new created bridge, which is used to forward VM's traffic. So all packets transmitted from VM will be tagged before it departs from the physical network interface.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/API.json | 6 ++++++ src/kimchi/model.py | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..07bf41e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -56,6 +56,12 @@ "interface": { "description": "The name of a network interface on the host", "type": "string" + }, + "vlan_id": { + "description": "Network's VLAN ID", + "type": "integer", + "maximum": 4094, + "minimum": 1 } } }, diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..b3589e9 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -58,6 +58,7 @@ from kimchi import config from kimchi import isoinfo from kimchi import netinfo from kimchi import network as knetwork +from kimchi import networkxml from kimchi import vnc from kimchi import xmlutils from kimchi.asynctask import AsyncTask @@ -65,7 +66,6 @@ from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter, MissingParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests -from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner from kimchi.screenshot import VMScreenshot @@ -806,7 +806,12 @@ class Model(object): if netinfo.is_bridge(iface): params['bridge'] = iface elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): - params['forward']['dev'] = iface + if params.get('vlan_id') is None: + params['forward']['dev'] = iface + else: + params['bridge'] = \ + self._create_vlanned_bridge(str(iface), + str(params['vlan_id'])) else: raise InvalidParameter("the interface should be bare nic, " "bonding or bridge device.") @@ -830,7 +835,7 @@ class Model(object): if connection == 'bridge': self._set_network_bridge(params)
- xml = to_network_xml(**params) + xml = networkxml.to_network_xml(**params)
try: network = conn.networkDefineXML(xml) @@ -888,8 +893,44 @@ class Model(object): if network.isActive(): raise InvalidOperation( "Unable to delete the active network %s" % name) + self._remove_vlanned_bridge(network) network.undefine()
+ def _get_vlanned_bridge_name(self, interface, vlan_id): + return '-'.join(('kimchi', interface, vlan_id)) + + def _is_vlanned_bridge(self, bridge): + return bridge.startswith('kimchi-') + + def _create_vlanned_bridge(self, interface, vlan_id): + bridge = self._get_vlanned_bridge_name(interface, vlan_id) + bridge_xml = networkxml.create_vlanned_bridge_xml(bridge, interface, + vlan_id) + conn = self.conn.get() + conn.changeBegin() + try: + vlanned_bridge = conn.interfaceDefineXML(bridge_xml) + vlanned_bridge.create() + except: + conn.changeRollback() + raise OperationFailed('Failed to create vlanned bridge') + else: + conn.changeCommit() + return bridge +
You can avoid multiple indents by removing 'else' statement I prefer to use 'try...except...else' to because it matches 'do...rollback...commit' clearly. If the 'else' statement is removed, we need to check the code in 'except' to see if unindent code only runs on success. Moreover, the statement under 'else' is very short. We don't need care about the indents. Does it make sense?
+ def _remove_vlanned_bridge(self, network): + try: + bridge = network.bridgeName() + except libvirt.libvirtError: + pass
Log the information.
kimchi_log.info() or kimchi_log.debug() It doesn't mean any actual error. It only happen when the network doesn't have a bridge, like the networks forwarded by macvtap. So it's just like a normal 'if' statement to check the forward mode of the network. I don't think we need a special logging here.
+ else: + if self._is_vlanned_bridge(bridge): + conn = self.conn.get() + iface = conn.interfaceLookupByName(bridge) + if iface.isActive(): + iface.destroy() + iface.undefine()
You can avoid multiple indents by removing 'else' statement Nope, we can't remove 'else' here. It change the semantic. Because the 'pass' statement in 'except' block doesn't stop execution, it will run into the code no matter it succeeds or not. We need only call it on success.
+ def add_task(self, target_uri, fn, opaque=None): id = self.next_taskid self.next_taskid = self.next_taskid + 1
On 01/03/2014 10:34 AM, Mark Wu wrote:
On 01/03/2014 02:04 AM, Aline Manera wrote:
On 01/02/2014 07:50 AM, Mark Wu wrote:
It creates a vlan interface on top of the given nic or bond interface, and bridge the vlan interface to a new created bridge, which is used to forward VM's traffic. So all packets transmitted from VM will be tagged before it departs from the physical network interface.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/API.json | 6 ++++++ src/kimchi/model.py | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..07bf41e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -56,6 +56,12 @@ "interface": { "description": "The name of a network interface on the host", "type": "string" + }, + "vlan_id": { + "description": "Network's VLAN ID", + "type": "integer", + "maximum": 4094, + "minimum": 1 } } }, diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..b3589e9 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -58,6 +58,7 @@ from kimchi import config from kimchi import isoinfo from kimchi import netinfo from kimchi import network as knetwork +from kimchi import networkxml from kimchi import vnc from kimchi import xmlutils from kimchi.asynctask import AsyncTask @@ -65,7 +66,6 @@ from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter, MissingParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests -from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner from kimchi.screenshot import VMScreenshot @@ -806,7 +806,12 @@ class Model(object): if netinfo.is_bridge(iface): params['bridge'] = iface elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): - params['forward']['dev'] = iface + if params.get('vlan_id') is None: + params['forward']['dev'] = iface + else: + params['bridge'] = \ + self._create_vlanned_bridge(str(iface), + str(params['vlan_id'])) else: raise InvalidParameter("the interface should be bare nic, " "bonding or bridge device.") @@ -830,7 +835,7 @@ class Model(object): if connection == 'bridge': self._set_network_bridge(params)
- xml = to_network_xml(**params) + xml = networkxml.to_network_xml(**params)
try: network = conn.networkDefineXML(xml) @@ -888,8 +893,44 @@ class Model(object): if network.isActive(): raise InvalidOperation( "Unable to delete the active network %s" % name) + self._remove_vlanned_bridge(network) network.undefine()
+ def _get_vlanned_bridge_name(self, interface, vlan_id): + return '-'.join(('kimchi', interface, vlan_id)) + + def _is_vlanned_bridge(self, bridge): + return bridge.startswith('kimchi-') + + def _create_vlanned_bridge(self, interface, vlan_id): + bridge = self._get_vlanned_bridge_name(interface, vlan_id) + bridge_xml = networkxml.create_vlanned_bridge_xml(bridge, interface, + vlan_id) + conn = self.conn.get() + conn.changeBegin() + try: + vlanned_bridge = conn.interfaceDefineXML(bridge_xml) + vlanned_bridge.create() + except: + conn.changeRollback() + raise OperationFailed('Failed to create vlanned bridge') + else: + conn.changeCommit() + return bridge +
You can avoid multiple indents by removing 'else' statement I prefer to use 'try...except...else' to because it matches 'do...rollback...commit' clearly. If the 'else' statement is removed, we need to check the code in 'except' to see if unindent code only runs on success. Moreover, the statement under 'else' is very short. We don't need care about the indents. Does it make sense?
+ def _remove_vlanned_bridge(self, network): + try: + bridge = network.bridgeName() + except libvirt.libvirtError: + pass
Log the information.
kimchi_log.info() or kimchi_log.debug() It doesn't mean any actual error. It only happen when the network doesn't have a bridge, like the networks forwarded by macvtap. So it's just like a normal 'if' statement to check the forward mode of the network. I don't think we need a special logging here. sure, some type networks do not has bridge, and libvirt will throw a exception.
+ else: + if self._is_vlanned_bridge(bridge): + conn = self.conn.get() + iface = conn.interfaceLookupByName(bridge) + if iface.isActive(): + iface.destroy() + iface.undefine()
You can avoid multiple indents by removing 'else' statement Nope, we can't remove 'else' here. It change the semantic. Because the 'pass' statement in 'except' block doesn't stop execution, it will run into the code no matter it succeeds or not. We need only call it on success.
+ def add_task(self, target_uri, fn, opaque=None): id = self.next_taskid self.next_taskid = self.next_taskid + 1
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 01/02/2014 07:50 AM, Mark Wu wrote:
It creates a vlan interface on top of the given nic or bond interface, and bridge the vlan interface to a new created bridge, which is used to forward VM's traffic. So all packets transmitted from VM will be tagged before it departs from the physical network interface.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/API.json | 6 ++++++ src/kimchi/model.py | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..07bf41e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -56,6 +56,12 @@ "interface": { "description": "The name of a network interface on the host", "type": "string" + }, + "vlan_id": { + "description": "Network's VLAN ID", + "type": "integer", + "maximum": 4094, + "minimum": 1 }
Please, use 4 spaces to indentation
} }, diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..b3589e9 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -58,6 +58,7 @@ from kimchi import config from kimchi import isoinfo from kimchi import netinfo from kimchi import network as knetwork +from kimchi import networkxml from kimchi import vnc from kimchi import xmlutils from kimchi.asynctask import AsyncTask @@ -65,7 +66,6 @@ from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter, MissingParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests -from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner from kimchi.screenshot import VMScreenshot @@ -806,7 +806,12 @@ class Model(object): if netinfo.is_bridge(iface): params['bridge'] = iface elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): - params['forward']['dev'] = iface + if params.get('vlan_id') is None: + params['forward']['dev'] = iface + else: + params['bridge'] = \ + self._create_vlanned_bridge(str(iface), + str(params['vlan_id'])) else: raise InvalidParameter("the interface should be bare nic, " "bonding or bridge device.") @@ -830,7 +835,7 @@ class Model(object): if connection == 'bridge': self._set_network_bridge(params)
- xml = to_network_xml(**params) + xml = networkxml.to_network_xml(**params)
try: network = conn.networkDefineXML(xml) @@ -888,8 +893,44 @@ class Model(object): if network.isActive(): raise InvalidOperation( "Unable to delete the active network %s" % name) + self._remove_vlanned_bridge(network) network.undefine()
+ def _get_vlanned_bridge_name(self, interface, vlan_id): + return '-'.join(('kimchi', interface, vlan_id)) + + def _is_vlanned_bridge(self, bridge): + return bridge.startswith('kimchi-') + + def _create_vlanned_bridge(self, interface, vlan_id): + bridge = self._get_vlanned_bridge_name(interface, vlan_id) + bridge_xml = networkxml.create_vlanned_bridge_xml(bridge, interface, + vlan_id) + conn = self.conn.get() + conn.changeBegin() + try: + vlanned_bridge = conn.interfaceDefineXML(bridge_xml) + vlanned_bridge.create() + except: + conn.changeRollback() + raise OperationFailed('Failed to create vlanned bridge') + else: + conn.changeCommit() + return bridge + + def _remove_vlanned_bridge(self, network): + try: + bridge = network.bridgeName() + except libvirt.libvirtError: + pass + else: + if self._is_vlanned_bridge(bridge): + conn = self.conn.get() + iface = conn.interfaceLookupByName(bridge) + if iface.isActive(): + iface.destroy() + iface.undefine() + def add_task(self, target_uri, fn, opaque=None): id = self.next_taskid self.next_taskid = self.next_taskid + 1
On 01/03/2014 02:11 AM, Aline Manera wrote:
On 01/02/2014 07:50 AM, Mark Wu wrote:
It creates a vlan interface on top of the given nic or bond interface, and bridge the vlan interface to a new created bridge, which is used to forward VM's traffic. So all packets transmitted from VM will be tagged before it departs from the physical network interface.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/API.json | 6 ++++++ src/kimchi/model.py | 47 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/src/kimchi/API.json b/src/kimchi/API.json index 7b90826..07bf41e 100644 --- a/src/kimchi/API.json +++ b/src/kimchi/API.json @@ -56,6 +56,12 @@ "interface": { "description": "The name of a network interface on the host", "type": "string" + }, + "vlan_id": { + "description": "Network's VLAN ID", + "type": "integer", + "maximum": 4094, + "minimum": 1 }
Please, use 4 spaces to indentation ACK. thanks!
} }, diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..b3589e9 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -58,6 +58,7 @@ from kimchi import config from kimchi import isoinfo from kimchi import netinfo from kimchi import network as knetwork +from kimchi import networkxml from kimchi import vnc from kimchi import xmlutils from kimchi.asynctask import AsyncTask @@ -65,7 +66,6 @@ from kimchi.distroloader import DistroLoader from kimchi.exception import InvalidOperation, InvalidParameter, MissingParameter from kimchi.exception import NotFoundError, OperationFailed from kimchi.featuretests import FeatureTests -from kimchi.networkxml import to_network_xml from kimchi.objectstore import ObjectStore from kimchi.scan import Scanner from kimchi.screenshot import VMScreenshot @@ -806,7 +806,12 @@ class Model(object): if netinfo.is_bridge(iface): params['bridge'] = iface elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): - params['forward']['dev'] = iface + if params.get('vlan_id') is None: + params['forward']['dev'] = iface + else: + params['bridge'] = \ + self._create_vlanned_bridge(str(iface), + str(params['vlan_id'])) else: raise InvalidParameter("the interface should be bare nic, " "bonding or bridge device.") @@ -830,7 +835,7 @@ class Model(object): if connection == 'bridge': self._set_network_bridge(params)
- xml = to_network_xml(**params) + xml = networkxml.to_network_xml(**params)
try: network = conn.networkDefineXML(xml) @@ -888,8 +893,44 @@ class Model(object): if network.isActive(): raise InvalidOperation( "Unable to delete the active network %s" % name) + self._remove_vlanned_bridge(network) network.undefine()
+ def _get_vlanned_bridge_name(self, interface, vlan_id): + return '-'.join(('kimchi', interface, vlan_id)) + + def _is_vlanned_bridge(self, bridge): + return bridge.startswith('kimchi-') + + def _create_vlanned_bridge(self, interface, vlan_id): + bridge = self._get_vlanned_bridge_name(interface, vlan_id) + bridge_xml = networkxml.create_vlanned_bridge_xml(bridge, interface, + vlan_id) + conn = self.conn.get() + conn.changeBegin() + try: + vlanned_bridge = conn.interfaceDefineXML(bridge_xml) + vlanned_bridge.create() + except: + conn.changeRollback() + raise OperationFailed('Failed to create vlanned bridge') + else: + conn.changeCommit() + return bridge + + def _remove_vlanned_bridge(self, network): + try: + bridge = network.bridgeName() + except libvirt.libvirtError: + pass + else: + if self._is_vlanned_bridge(bridge): + conn = self.conn.get() + iface = conn.interfaceLookupByName(bridge) + if iface.isActive(): + iface.destroy() + iface.undefine() + def add_task(self, target_uri, fn, opaque=None): id = self.next_taskid self.next_taskid = self.next_taskid + 1
It adds a checkbox to provide choice of enabling vlan. If it's checked, it all user to type the vlan ID. Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- po/en_US.po | 3 +++ po/kimchi.pot | 3 +++ po/pt_BR.po | 3 +++ po/zh_CN.po | 3 +++ ui/css/theme-default/network.css | 9 +++++++++ ui/js/src/kimchi.network.js | 9 ++++++++- ui/pages/tabs/network.html.tmpl | 6 ++++++ 7 files changed, 35 insertions(+), 1 deletion(-) diff --git a/po/en_US.po b/po/en_US.po index f2810db..aa80424 100644 --- a/po/en_US.po +++ b/po/en_US.po @@ -543,6 +543,9 @@ msgstr "Bridged: VMs are connected to physical network directly" msgid "Destination" msgstr "Destination" +msgid "Enable VLAN" +msgstr "Enable VLAN" + msgid "No templates found." msgstr "No templates found." diff --git a/po/kimchi.pot b/po/kimchi.pot index 762f4e4..3a25dc5 100644 --- a/po/kimchi.pot +++ b/po/kimchi.pot @@ -523,6 +523,9 @@ msgstr "" msgid "Destination" msgstr "" +msgid "Enable VLAN" +msgstr "" + msgid "No templates found." msgstr "" diff --git a/po/pt_BR.po b/po/pt_BR.po index 7d59503..bff7c42 100644 --- a/po/pt_BR.po +++ b/po/pt_BR.po @@ -559,6 +559,9 @@ msgstr "Bridged: M��quinas virtuais est��o conectadas diretamente a rede f��sica msgid "Destination" msgstr "Destino" +msgid "Enable VLAN" +msgstr "Activar VLAN" + msgid "No templates found." msgstr "Nenhum modelo encontrado." diff --git a/po/zh_CN.po b/po/zh_CN.po index de759ef..addae1a 100644 --- a/po/zh_CN.po +++ b/po/zh_CN.po @@ -541,6 +541,9 @@ msgstr "������: ���������������������������������" msgid "Destination" msgstr "������������" +msgid "Enable VLAN" +msgstr "������VLAN" + msgid "No templates found." msgstr "������������������" diff --git a/ui/css/theme-default/network.css b/ui/css/theme-default/network.css index f7d1e14..8987e9a 100644 --- a/ui/css/theme-default/network.css +++ b/ui/css/theme-default/network.css @@ -351,6 +351,15 @@ margin-left: 28px; } +.network-config .VLAN { + margin-left: 28px; +} + +.network-config .VLAN input[type="text"] { + height: 25px; + width: 60px; +} + .network-config .input-hint-icon { margin: -1px 1px 0 0; display: inline-block; diff --git a/ui/js/src/kimchi.network.js b/ui/js/src/kimchi.network.js index f7b7eed..16b46f2 100644 --- a/ui/js/src/kimchi.network.js +++ b/ui/js/src/kimchi.network.js @@ -135,11 +135,13 @@ kimchi.initNetworkCreation = function() { var network = kimchi.getNetworkDialogValues(); var data = { name : network.name, - connection: network.type + connection: network.type, + vlan_id: network.vlan_id, }; if (network.type === kimchi.NETWORK_TYPE_BRIDGE) { data.connection = "bridge"; data.interface = network.interface; + data.vlan_id = network.vlan_id; } kimchi.createNetwork(data, function(result) { network.state = result.state === "active" ? "up" : "down"; @@ -193,6 +195,9 @@ kimchi.openNetworkDialog = function(okCallback) { okCallback(); $("#networkConfig").dialog("close"); }); + $("#enableVlan").on("click", function() { + $("#networkVlanID").prop("disabled", !this.checked); + }); $("#networkConfig").dialog("open"); }; @@ -210,6 +215,7 @@ kimchi.getNetworkDialogValues = function() { }; if (network.type === kimchi.NETWORK_TYPE_BRIDGE) { network.interface = $("#networkInterface").val(); + network.vlan_id = parseInt($("#networkVlanID").val()); } return network; }; @@ -224,6 +230,7 @@ kimchi.cleanNetworkDialog = function() { $("#networkInterface option").removeAttr("selected").find(":first").attr("selected", "selected"); $("#networkFormOk").off("click"); $("#networkFormOk").button("disable"); + $("#networkVlanID").prop("disabled", true); }; kimchi.setupNetworkFormEvent = function() { diff --git a/ui/pages/tabs/network.html.tmpl b/ui/pages/tabs/network.html.tmpl index d850140..308e6d5 100644 --- a/ui/pages/tabs/network.html.tmpl +++ b/ui/pages/tabs/network.html.tmpl @@ -75,6 +75,12 @@ <label>$_("Destination"): </label> <select id="networkInterface"></select> </div> + <div class="VLAN"> + <label>$_("Enable VLAN"): </label> + <input id="enableVlan" type="checkbox" value=""/> + <label>$_("VLAN ID"): </label> + <input type="text" id="networkVlanID" disabled> + </div> </div> </div> </div> -- 1.8.4.2
On 01/02/2014 07:50 AM, Mark Wu wrote:
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+)
diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
import ipaddr +import lxml.etree as ET
2 lines
+from lxml.builder import E
# FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m) diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest
import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils
class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest
from contextlib import closing +from lxml import etree
import kimchi.server @@ -159,3 +160,8 @@ def patch_auth():
import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True)))
Hi Aline, I would like to start a discussion about the code style for importing modules by this chance. I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules. 1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1] 2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from') For this patch, I don't think we need two blank to separate them because they belongs to the same group. Does it make thanks for you? Thanks. Mark. [1] http://www.python.org/dev/peps/pep-0008/ On 01/03/2014 02:00 AM, Aline Manera wrote:
On 01/02/2014 07:50 AM, Mark Wu wrote:
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+)
diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
import ipaddr +import lxml.etree as ET
2 lines
+from lxml.builder import E
# FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m) diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest
import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils
class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest
from contextlib import closing +from lxml import etree
import kimchi.server @@ -159,3 +160,8 @@ def patch_auth():
import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True)))
on 2014/01/03 14:23, Mark Wu wrote:
Hi Aline,
I would like to start a discussion about the code style for importing modules by this chance.
I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules.
1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1]
2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from')
For this patch, I don't think we need two blank to separate them because they belongs to the same group.
Does it make thanks for you?
Agree. In a word, I think it should like following. import standardLibA import standardLibB from standardLibX.Y import ... from standardLibX.Z import ... import thirdPartyLibA import thirdPartyLibB from thirdPartyLibX.Y import ... from thirdPartyLibX.Z import ... import kimchiLibA import kimchiLibB from kimchiLibX.Y import ... from kimchiLibX.Z import ... Since almost all Python projects agree with PEP 8, the current rule is very counterintuitive to me (especially the two-blank-line-before-kimchi-import rule). I think the rule suggested by Mark is more friendly to me. All PEPs are discussed by lots of Python developers, we should follow this collective intelligence unless we have very good reason or particular use case to break it.
Thanks. Mark.
-- Thanks and best regards! Zhou Zheng Sheng / 周征晟 E-mail: zhshzhou@linux.vnet.ibm.com Telephone: 86-10-82454397
On 01/03/2014 04:23 AM, Mark Wu wrote:
Hi Aline,
I would like to start a discussion about the code style for importing modules by this chance.
I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules.
1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1]
2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from')
For this patch, I don't think we need two blank to separate them because they belongs to the same group.
Does it make thanks for you?
As I've already said, we are a different rule from pep8 for imports We are using the following rule: import ... import ... import ... <2 lines> from ... import ... from ... import ... <2 lines> import kimchi... from kimchi import ... <2 lines> All those blocks must be in alphabetic order So, please, organize the imports accordingly to it
Thanks. Mark.
[1] http://www.python.org/dev/peps/pep-0008/
On 01/03/2014 02:00 AM, Aline Manera wrote:
On 01/02/2014 07:50 AM, Mark Wu wrote:
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+)
diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
import ipaddr +import lxml.etree as ET
2 lines
+from lxml.builder import E
# FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m) diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest
import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils
class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest
from contextlib import closing +from lxml import etree
import kimchi.server @@ -159,3 +160,8 @@ def patch_auth():
import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True)))
on 2014/01/04 00:32, Aline Manera wrote:
On 01/03/2014 04:23 AM, Mark Wu wrote:
Hi Aline,
I would like to start a discussion about the code style for importing modules by this chance.
I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules.
1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1]
2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from')
For this patch, I don't think we need two blank to separate them because they belongs to the same group.
Does it make thanks for you?
As I've already said, we are a different rule from pep8 for imports
We are using the following rule:
Hi Aline. Maybe you did not see my previous reply to Mark's proposal. I listed some reasons to stick to PEP 8 import rules. The problem here is not that we do not know the kimchi rule, but is actually we have different idea on the rule. Would you explain why the current rule is superior to PEP 8? As I wrote, since almost all Python projects comply with PEP, the current rule is very counterintuitive to me (especially the two-blank-line-between-import-section rule). I'd believe this rule would also be strange to other Python developers, and you create extra maintenance effort to reminding and describing the kimchi rule to Pythonic people who already "programmed" themselves to work with PEP. In fact the PEP 8 is more reasonable on the grouping. It differentiate standard lib and third-party lib import. This is very convenience if someone sees a new module to him, and want to lookup documentation. He can just refer to Python official site for standard lib and search on google for third-party lib. Otherwise, he has to read more code to determine if it's an third-party import. The one-blank-line-between-section rule of PEP 8 makes all the import sections compact. We use 2 blank lines between classes because classes are complicated logical entities. However import statements are just listings, they are very simple. Using one blank line is enough and we get compact information. Maybe the current kimchi rule is better in some cases, but the point is that all PEPs are discussed and recognize by lots of Python developers. PEPs are kind of collective intelligence. We should follow PEPs to make our project welcome to the whole Python community, unless we have good enough reason or special enough use case to break it. Here I'm proposing the PEP 8 rule again, while I'm not simply against the current kimchi rule. I'm open to here your considerations. Would you mind explaining your concerns on the current kimchi rule? Why it's good for us? How it's better than PEP 8? Why changing it to PEP 8 is not good for us? Are there some special use case to break PEP 8? What benefit do we get to abandon PEP 8?
import ... import ... import ...
<2 lines>
from ... import ... from ... import ...
<2 lines>
import kimchi... from kimchi import ...
<2 lines>
All those blocks must be in alphabetic order
So, please, organize the imports accordingly to it
Thanks. Mark.
-- Thanks and best regards! Zhou Zheng Sheng / 周征晟 E-mail: zhshzhou@linux.vnet.ibm.com Telephone: 86-10-82454397
On 01/05/2014 11:05 AM, Zhou Zheng Sheng wrote:
on 2014/01/04 00:32, Aline Manera wrote:
On 01/03/2014 04:23 AM, Mark Wu wrote:
Hi Aline,
I would like to start a discussion about the code style for importing modules by this chance.
I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules.
1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1]
2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from')
For this patch, I don't think we need two blank to separate them because they belongs to the same group.
Does it make thanks for you?
As I've already said, we are a different rule from pep8 for imports
We are using the following rule: Hi Aline.
Maybe you did not see my previous reply to Mark's proposal. I listed some reasons to stick to PEP 8 import rules. The problem here is not that we do not know the kimchi rule, but is actually we have different idea on the rule. Would you explain why the current rule is superior to PEP 8?
As I wrote, since almost all Python projects comply with PEP, the current rule is very counterintuitive to me (especially the two-blank-line-between-import-section rule). I'd believe this rule would also be strange to other Python developers, and you create extra maintenance effort to reminding and describing the kimchi rule to Pythonic people who already "programmed" themselves to work with PEP.
In fact the PEP 8 is more reasonable on the grouping. It differentiate standard lib and third-party lib import. This is very convenience if someone sees a new module to him, and want to lookup documentation. He can just refer to Python official site for standard lib and search on google for third-party lib. Otherwise, he has to read more code to determine if it's an third-party import.
The one-blank-line-between-section rule of PEP 8 makes all the import sections compact. We use 2 blank lines between classes because classes are complicated logical entities. However import statements are just listings, they are very simple. Using one blank line is enough and we get compact information.
Maybe the current kimchi rule is better in some cases, but the point is that all PEPs are discussed and recognize by lots of Python developers. PEPs are kind of collective intelligence. We should follow PEPs to make our project welcome to the whole Python community, unless we have good enough reason or special enough use case to break it.
Here I'm proposing the PEP 8 rule again, while I'm not simply against the current kimchi rule. I'm open to here your considerations. Would you mind explaining your concerns on the current kimchi rule? Why it's good for us? How it's better than PEP 8? Why changing it to PEP 8 is not good for us? Are there some special use case to break PEP 8? What benefit do we get to abandon PEP 8?
Oh... I think this discussion is becoming bigger than it really is. First of all, let me get it clear, I am not against the PEP 8 style. The intention with the imports organization patches was to organize them according to *a rule* (whatever is that) to avoid getting duplicated imports and so on. No patch is accepted upstream before being reviewed by at least one developer. Which means the imports organization patches were sent to review and accepted without other suggestion. Maybe because of the short time until the patch be applied or even because we are in a transition for the PEP 8 style. Because that, Kimchi is using a different rule than PEP. As you may know, I proposed the current import rule and I am not a PEP expert because that I didn't heed to it. But I am open to change it if all of you agree it is a better solution.
import ... import ... import ...
<2 lines>
from ... import ... from ... import ...
<2 lines>
import kimchi... from kimchi import ...
<2 lines>
All those blocks must be in alphabetic order
So, please, organize the imports accordingly to it
Thanks. Mark.
On 01/06/2014 09:18 PM, Aline Manera wrote:
On 01/05/2014 11:05 AM, Zhou Zheng Sheng wrote:
on 2014/01/04 00:32, Aline Manera wrote:
On 01/03/2014 04:23 AM, Mark Wu wrote:
Hi Aline,
I would like to start a discussion about the code style for importing modules by this chance.
I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules.
1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1]
2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from')
For this patch, I don't think we need two blank to separate them because they belongs to the same group.
Does it make thanks for you?
As I've already said, we are a different rule from pep8 for imports
We are using the following rule: Hi Aline.
Maybe you did not see my previous reply to Mark's proposal. I listed some reasons to stick to PEP 8 import rules. The problem here is not that we do not know the kimchi rule, but is actually we have different idea on the rule. Would you explain why the current rule is superior to PEP 8?
As I wrote, since almost all Python projects comply with PEP, the current rule is very counterintuitive to me (especially the two-blank-line-between-import-section rule). I'd believe this rule would also be strange to other Python developers, and you create extra maintenance effort to reminding and describing the kimchi rule to Pythonic people who already "programmed" themselves to work with PEP.
In fact the PEP 8 is more reasonable on the grouping. It differentiate standard lib and third-party lib import. This is very convenience if someone sees a new module to him, and want to lookup documentation. He can just refer to Python official site for standard lib and search on google for third-party lib. Otherwise, he has to read more code to determine if it's an third-party import.
The one-blank-line-between-section rule of PEP 8 makes all the import sections compact. We use 2 blank lines between classes because classes are complicated logical entities. However import statements are just listings, they are very simple. Using one blank line is enough and we get compact information.
Maybe the current kimchi rule is better in some cases, but the point is that all PEPs are discussed and recognize by lots of Python developers. PEPs are kind of collective intelligence. We should follow PEPs to make our project welcome to the whole Python community, unless we have good enough reason or special enough use case to break it.
Here I'm proposing the PEP 8 rule again, while I'm not simply against the current kimchi rule. I'm open to here your considerations. Would you mind explaining your concerns on the current kimchi rule? Why it's good for us? How it's better than PEP 8? Why changing it to PEP 8 is not good for us? Are there some special use case to break PEP 8? What benefit do we get to abandon PEP 8?
Oh... I think this discussion is becoming bigger than it really is.
First of all, let me get it clear, I am not against the PEP 8 style.
The intention with the imports organization patches was to organize them according to *a rule* (whatever is that) to avoid getting duplicated imports and so on.
No patch is accepted upstream before being reviewed by at least one developer. Which means the imports organization patches were sent to review and accepted without other suggestion. Actually, at that time, I have provided my suggestion on that: https://groups.google.com/forum/#!msg/project-kimchi/smxYARdcNEY/rGpxBjjW7JM... <https://groups.google.com/forum/#%21msg/project-kimchi/smxYARdcNEY/rGpxBjjW7JMJ>
As what I said before, the duplicate import can be resolved by the pep8 check.
Maybe because of the short time until the patch be applied or even because we are in a transition for the PEP 8 style. Because that, Kimchi is using a different rule than PEP.
As you may know, I proposed the current import rule and I am not a PEP expert because that I didn't heed to it.
But I am open to change it if all of you agree it is a better solution.
import ... import ... import ...
<2 lines>
from ... import ... from ... import ...
<2 lines>
import kimchi... from kimchi import ...
<2 lines>
All those blocks must be in alphabetic order
So, please, organize the imports accordingly to it
Thanks. Mark.
于 2014年01月06日 21:18, Aline Manera 写道:
On 01/05/2014 11:05 AM, Zhou Zheng Sheng wrote:
on 2014/01/04 00:32, Aline Manera wrote:
On 01/03/2014 04:23 AM, Mark Wu wrote:
Hi Aline,
I would like to start a discussion about the code style for importing modules by this chance.
I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules.
1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1]
2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from')
For this patch, I don't think we need two blank to separate them because they belongs to the same group.
Does it make thanks for you?
As I've already said, we are a different rule from pep8 for imports
We are using the following rule: Hi Aline.
Maybe you did not see my previous reply to Mark's proposal. I listed some reasons to stick to PEP 8 import rules. The problem here is not that we do not know the kimchi rule, but is actually we have different idea on the rule. Would you explain why the current rule is superior to PEP 8?
As I wrote, since almost all Python projects comply with PEP, the current rule is very counterintuitive to me (especially the two-blank-line-between-import-section rule). I'd believe this rule would also be strange to other Python developers, and you create extra maintenance effort to reminding and describing the kimchi rule to Pythonic people who already "programmed" themselves to work with PEP.
In fact the PEP 8 is more reasonable on the grouping. It differentiate standard lib and third-party lib import. This is very convenience if someone sees a new module to him, and want to lookup documentation. He can just refer to Python official site for standard lib and search on google for third-party lib. Otherwise, he has to read more code to determine if it's an third-party import.
The one-blank-line-between-section rule of PEP 8 makes all the import sections compact. We use 2 blank lines between classes because classes are complicated logical entities. However import statements are just listings, they are very simple. Using one blank line is enough and we get compact information.
Maybe the current kimchi rule is better in some cases, but the point is that all PEPs are discussed and recognize by lots of Python developers. PEPs are kind of collective intelligence. We should follow PEPs to make our project welcome to the whole Python community, unless we have good enough reason or special enough use case to break it.
Here I'm proposing the PEP 8 rule again, while I'm not simply against the current kimchi rule. I'm open to here your considerations. Would you mind explaining your concerns on the current kimchi rule? Why it's good for us? How it's better than PEP 8? Why changing it to PEP 8 is not good for us? Are there some special use case to break PEP 8? What benefit do we get to abandon PEP 8?
Oh... I think this discussion is becoming bigger than it really is.
First of all, let me get it clear, I am not against the PEP 8 style.
The intention with the imports organization patches was to organize them according to *a rule* (whatever is that) to avoid getting duplicated imports and so on.
Hi, thanks for the reply. I know you are for PEP 8, but using a different import rule other than PEP 8 is not fully PEP 8 compliant. As regard to duplicated imports, I think pyflakes can check this type of error, and lots of other errors. Many Python projects use pyflakes or pylint to check logical errors.
No patch is accepted upstream before being reviewed by at least one developer. Which means the imports organization patches were sent to review and accepted without other suggestion. Maybe because of the short time until the patch be applied or even because we are in a transition for the PEP 8 style. Because that, Kimchi is using a different rule than PEP.
Yes, but when we discovered a better way to arrange imports, we can slowly transit to the better one.
As you may know, I proposed the current import rule and I am not a PEP expert because that I didn't heed to it.
But I am open to change it if all of you agree it is a better solution.
import ... import ... import ...
<2 lines>
from ... import ... from ... import ...
<2 lines>
import kimchi... from kimchi import ...
<2 lines>
All those blocks must be in alphabetic order
So, please, organize the imports accordingly to it
Thanks. Mark.
-- Thanks and best regards! Zhou Zheng Sheng / 周征晟 E-mail: zhshzhou@linux.vnet.ibm.com Telephone: 86-10-82454397
On 01/07/2014 12:05 PM, Zhou Zheng Sheng wrote:
于 2014年01月06日 21:18, Aline Manera 写道:
On 01/05/2014 11:05 AM, Zhou Zheng Sheng wrote:
on 2014/01/04 00:32, Aline Manera wrote:
On 01/03/2014 04:23 AM, Mark Wu wrote:
Hi Aline,
I would like to start a discussion about the code style for importing modules by this chance.
I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules.
1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1]
2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from')
For this patch, I don't think we need two blank to separate them because they belongs to the same group.
Does it make thanks for you?
As I've already said, we are a different rule from pep8 for imports
We are using the following rule: Hi Aline.
Maybe you did not see my previous reply to Mark's proposal. I listed some reasons to stick to PEP 8 import rules. The problem here is not that we do not know the kimchi rule, but is actually we have different idea on the rule. Would you explain why the current rule is superior to PEP 8?
As I wrote, since almost all Python projects comply with PEP, the current rule is very counterintuitive to me (especially the two-blank-line-between-import-section rule). I'd believe this rule would also be strange to other Python developers, and you create extra maintenance effort to reminding and describing the kimchi rule to Pythonic people who already "programmed" themselves to work with PEP.
In fact the PEP 8 is more reasonable on the grouping. It differentiate standard lib and third-party lib import. This is very convenience if someone sees a new module to him, and want to lookup documentation. He can just refer to Python official site for standard lib and search on google for third-party lib. Otherwise, he has to read more code to determine if it's an third-party import.
The one-blank-line-between-section rule of PEP 8 makes all the import sections compact. We use 2 blank lines between classes because classes are complicated logical entities. However import statements are just listings, they are very simple. Using one blank line is enough and we get compact information.
Maybe the current kimchi rule is better in some cases, but the point is that all PEPs are discussed and recognize by lots of Python developers. PEPs are kind of collective intelligence. We should follow PEPs to make our project welcome to the whole Python community, unless we have good enough reason or special enough use case to break it.
Here I'm proposing the PEP 8 rule again, while I'm not simply against the current kimchi rule. I'm open to here your considerations. Would you mind explaining your concerns on the current kimchi rule? Why it's good for us? How it's better than PEP 8? Why changing it to PEP 8 is not good for us? Are there some special use case to break PEP 8? What benefit do we get to abandon PEP 8? Oh... I think this discussion is becoming bigger than it really is.
First of all, let me get it clear, I am not against the PEP 8 style.
The intention with the imports organization patches was to organize them according to *a rule* (whatever is that) to avoid getting duplicated imports and so on. Hi, thanks for the reply.
I know you are for PEP 8, but using a different import rule other than PEP 8 is not fully PEP 8 compliant. As regard to duplicated imports, I think pyflakes can check this type of error, and lots of other errors. Many Python projects use pyflakes or pylint to check logical errors. zhengsheng: add the doc "PEP8 Checking Using Syntastic" to git hub wiki. https://github.com/kimchi-project/kimchi/wiki/PEP8-Checking-Using-Syntastic
No patch is accepted upstream before being reviewed by at least one developer. Which means the imports organization patches were sent to review and accepted without other suggestion. Maybe because of the short time until the patch be applied or even because we are in a transition for the PEP 8 style. Because that, Kimchi is using a different rule than PEP. Yes, but when we discovered a better way to arrange imports, we can slowly transit to the better one. As you may know, I proposed the current import rule and I am not a PEP expert because that I didn't heed to it.
But I am open to change it if all of you agree it is a better solution.
import ... import ... import ...
<2 lines>
from ... import ... from ... import ...
<2 lines>
import kimchi... from kimchi import ...
<2 lines>
All those blocks must be in alphabetic order
So, please, organize the imports accordingly to it
Thanks. Mark.
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
I couldn't apply the patch 3/3 to test alinefm@alinefm:~/kimchi$ git am ../mail-patches/\[PATCH\ * Applying: Generate libvirt's interface XML definition for vlanned bridge Applying: Support creating vlan tagged virtual network fatal: cannot convert from y to UTF-8 When asked for UTF-8 encoding you should press ENTER instead of typing 'y'
On 01/03/2014 02:16 AM, Aline Manera wrote:
I couldn't apply the patch 3/3 to test
alinefm@alinefm:~/kimchi$ git am ../mail-patches/\[PATCH\ * Applying: Generate libvirt's interface XML definition for vlanned bridge Applying: Support creating vlan tagged virtual network fatal: cannot convert from y to UTF-8
When asked for UTF-8 encoding you should press ENTER instead of typing 'y'
OK, I didn't notice the prompt. I will fix it in next patch. Thanks for resolution.
On 01/03/2014 02:24 PM, Mark Wu wrote:
On 01/03/2014 02:16 AM, Aline Manera wrote:
I couldn't apply the patch 3/3 to test
alinefm@alinefm:~/kimchi$ git am ../mail-patches/\[PATCH\ * Applying: Generate libvirt's interface XML definition for vlanned bridge Applying: Support creating vlan tagged virtual network fatal: cannot convert from y to UTF-8
When asked for UTF-8 encoding you should press ENTER instead of typing 'y'
OK, I didn't notice the prompt. I will fix it in next patch. Thanks for resolution.
you can set your .git/config as follow, then git will not ask your the Encoding of mail. $ vim .git/config [sendemail] assume8bitEncoding = UTF-8 -- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
On 01/03/2014 03:48 PM, Sheldon wrote:
On 01/03/2014 02:24 PM, Mark Wu wrote:
On 01/03/2014 02:16 AM, Aline Manera wrote:
I couldn't apply the patch 3/3 to test
alinefm@alinefm:~/kimchi$ git am ../mail-patches/\[PATCH\ * Applying: Generate libvirt's interface XML definition for vlanned bridge Applying: Support creating vlan tagged virtual network fatal: cannot convert from y to UTF-8
When asked for UTF-8 encoding you should press ENTER instead of typing 'y'
OK, I didn't notice the prompt. I will fix it in next patch. Thanks for resolution.
you can set your .git/config as follow, then git will not ask your the Encoding of mail. $ vim .git/config [sendemail] assume8bitEncoding = UTF-8
Yes. thanks for the tip. I have added to the wiki page: https://github.com/kimchi-project/kimchi/wiki/i18n-Quick-Tips
On Thu, 2014-01-02 at 17:50 +0800, Mark Wu wrote:
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+)
diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
import ipaddr +import lxml.etree as ET +from lxml.builder import E
# FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m)
Could you add a check to make sure the interface passed in exists before adding a vlan bridge? And what if you call the routine, create_vlan_tagged_bridge_xml()? There are untagged vlans, and the word 'vlanned' just sounds funny to me.
diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest
import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils
class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest
from contextlib import closing +from lxml import etree
import kimchi.server @@ -159,3 +160,8 @@ def patch_auth():
import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True)))
On 01/03/2014 06:25 AM, Christy Perez wrote:
On Thu, 2014-01-02 at 17:50 +0800, Mark Wu wrote:
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+)
diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
import ipaddr +import lxml.etree as ET +from lxml.builder import E
# FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m) Could you add a check to make sure the interface passed in exists before adding a vlan bridge?
The validation is done in _set_network_bridge: def _set_network_bridge(self, params): ... elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): ----> this validation can guarantee the interface exists if params.get('vlan_id') is None: params['forward']['dev'] = iface else: params['bridge'] = \ self._create_vlanned_bridge(str(iface), str(params['vlan_id']))
And what if you call the routine, create_vlan_tagged_bridge_xml()? There are untagged vlans, and the word 'vlanned' just sounds funny to me.
ACK. I will update it in v2 patch. Thanks for the review.
diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest
import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils
class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest
from contextlib import closing +from lxml import etree
import kimchi.server @@ -159,3 +160,8 @@ def patch_auth():
import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True)))
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
On 01/03/2014 09:40 AM, Mark Wu wrote:
On 01/03/2014 06:25 AM, Christy Perez wrote:
On Thu, 2014-01-02 at 17:50 +0800, Mark Wu wrote:
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+)
diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
import ipaddr +import lxml.etree as ET +from lxml.builder import E
# FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m) Could you add a check to make sure the interface passed in exists before adding a vlan bridge?
The validation is done in _set_network_bridge: def _set_network_bridge(self, params): ... elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): ----> this validation can guarantee the interface exists if params.get('vlan_id') is None: params['forward']['dev'] = iface else: params['bridge'] = \ self._create_vlanned_bridge(str(iface), str(params['vlan_id']))
yes. the verify is in model. this file is just create xml.
And what if you call the routine, create_vlan_tagged_bridge_xml()? There are untagged vlans, and the word 'vlanned' just sounds funny to me.
ACK. I will update it in v2 patch. Thanks for the review.
diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest
import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils
class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest
from contextlib import closing +from lxml import etree
import kimchi.server @@ -159,3 +160,8 @@ def patch_auth():
import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True)))
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
lxml is a good xml tool. Maybe we can use it to refactor other xml code. On 01/02/2014 05:50 PM, Mark Wu wrote:
To support vlanned virtual network, kimchi needs create the underlying vlan and bridge interface. Libvirt's interface driver can do it with a given XML definition. This patch targets to generate the XML according to the interface and vlan id.
Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com> --- src/kimchi/networkxml.py | 17 +++++++++++++++++ tests/test_networkxml.py | 20 ++++++++++++++++++++ tests/utils.py | 6 ++++++ 3 files changed, 43 insertions(+)
diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py index 786cb69..25157fd 100644 --- a/src/kimchi/networkxml.py +++ b/src/kimchi/networkxml.py @@ -21,6 +21,8 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
import ipaddr +import lxml.etree as ET +from lxml.builder import E
# FIXME, do not support ipv6 @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): </network> """ % params return xml + + +def create_vlanned_bridge_xml(bridge, interface, vlan_id): + vlan = E.vlan(E.interface(name=interface)) + vlan.set('tag', vlan_id) + m = E.interface( + E.start(mode='onboot'), + E.bridge( + E.interface( + vlan, + type='vlan', + name='.'.join([interface, vlan_id]))), + type='bridge', + name=bridge) + return ET.tostring(m) diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py index 3073bce..1b2e77f 100644 --- a/tests/test_networkxml.py +++ b/tests/test_networkxml.py @@ -26,6 +26,7 @@ import unittest
import kimchi.networkxml as nxml from kimchi.xmlutils import xpath_get_text +import utils
class NetworkXmlTests(unittest.TestCase): @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] self.assertEquals(netmask, str(ipaddr.IPNetwork(params["net"]).netmask)) + + +class InterfaceXmlTests(unittest.TestCase): + + def test_vlanned_bridge_no_ip(self): + expected_xml = """ + <interface type='bridge' name='br10'> + <start mode='onboot'/> + <bridge> + <interface type='vlan' name='em1.10'> + <vlan tag='10'> + <interface name='em1'/> + </vlan> + </interface> + </bridge> + </interface> + """ + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', '10') + self.assertEquals(actual_xml, utils.normalize_xml(expected_xml)) diff --git a/tests/utils.py b/tests/utils.py index 008f668..79fc2e2 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,6 +32,7 @@ import unittest
from contextlib import closing +from lxml import etree
import kimchi.server @@ -159,3 +160,8 @@ def patch_auth():
import kimchi.auth kimchi.auth.authenticate = _authenticate + + +def normalize_xml(xml_str): + return etree.tostring(etree.fromstring(xml_str, + etree.XMLParser(remove_blank_text=True)))
-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center
participants (5)
-
Aline Manera -
Christy Perez -
Mark Wu -
Sheldon -
Zhou Zheng Sheng