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

[Mark Wu]

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.

 >> +        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