[Kimchi-devel] [PATCH 2/3] Support creating vlan tagged virtual network

[Sheldon]

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 at 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 at linux.vnet.ibm.com>
IBM Linux Technology Center