Re: [Kimchi-devel] [PATCH] [Kimchi] Fix handling of VLANs in backend

On 02/10/2016 02:38 PM, Lucio Correia wrote: > - Allow VLANs only for bridged virtual networks > - Code simplification > > Signed-off-by: Lucio Correia <luciojhc(a)linux.vnet.ibm.com&gt; > --- > i18n.py | 6 ++--- > model/networks.py | 71 > ++++++++++++++++++++++++++++++------------------------- > 2 files changed, 42 insertions(+), 35 deletions(-) > > diff --git a/i18n.py b/i18n.py > index 0b9fa39..bd36efa 100644 > --- a/i18n.py > +++ b/i18n.py > @@ -248,8 +248,8 @@ messages = { > > "KCHNET0001E": _("Network %(name)s already exists"), > "KCHNET0002E": _("Network %(name)s does not exist"), > - "KCHNET0003E": _("Subnet %(subnet)s specified for network > %(network)s is not valid."), > - "KCHNET0004E": _("Specify a network interface to create bridged > network %(name)s"), > + "KCHNET0003E": _("Subnet %(subnet)s specified for network > %(network)s is not valid."), > + "KCHNET0004E": _("Specify a network interface to create bridged > or macvtap networks."), > "KCHNET0005E": _("Unable to delete active network %(name)s"), > "KCHNET0006E": _("Interface %(iface)s specified for network > %(network)s is already in use"), > "KCHNET0007E": _("Interface should be bare NIC, bonding or > bridge device."), > @@ -270,8 +270,8 @@ messages = { > "KCHNET0022E": _("Failed to start network %(name)s. Details: > %(err)s"), > "KCHNET0024E": _("Unable to redefine interface %(name)s. > Details: %(err)s"), > "KCHNET0025E": _("Unable to create bridge %(name)s. Details: > %(err)s"), > - "KCHNET0026E": _("Open VSwitch bridges can only host bridged > networks."), > "KCHNET0027E": _("Unable to create bridge with NetworkManager > enabled. Disable it and try again."), > + "KCHNET0028E": _("Interface should be bare NIC or bonding."), > > "KCHSR0001E": _("Storage server %(server)s was not used by > Kimchi"), > > diff --git a/model/networks.py b/model/networks.py > index fba181f..9a49634 100644 > --- a/model/networks.py > +++ b/model/networks.py > @@ -86,21 +86,20 @@ class NetworksModel(object): > if name in self.get_list(): > raise InvalidOperation("KCHNET0001E", {'name': name}) > > + # handle connection type > connection = params["connection"] > - # set forward mode, isolated do not need forward > if connection == 'macvtap': > - params['forward'] = {'mode': 'bridge'} > - elif connection != 'isolated': > - params['forward'] = {'mode': connection} > + self._set_network_macvtap(params) > + elif connection == 'bridge': > + self._set_network_bridge(params) > + elif connection in ['nat', 'isolated']: > + if connection == 'nat': > + params['forward'] = {'mode': 'nat'} Just saw that you use a lot of strings like 'nat', 'bridge'. What about put them as constant?

> > - # set subnet, bridge network do not need subnet > - if connection in ["nat", 'isolated']: > + # set subnet; bridge/macvtap networks do not need subnet > self._set_network_subnet(params) > > - # only bridge network need bridge(linux bridge) or > interface(macvtap) > - if connection in ['bridge', 'macvtap']: > - self._set_network_bridge(params) > - > + # create network XML > params['name'] = escape(params['name']) > xml = to_network_xml(**params) > > @@ -165,7 +164,7 @@ class NetworksModel(object): > else: > raise OperationFailed("KCHNET0021E", {'iface': iface}) > > - def _set_network_bridge(self, params): > + def _check_network_interface(self, params): > try: > # fails if host interface is already in use by a libvirt > network > iface = params['interface'] > @@ -175,9 +174,24 @@ class NetworksModel(object): > except KeyError: > raise MissingParameter("KCHNET0004E", {'name': > params['name']}) > > - # Linux bridges cannot be the trunk device of a VLAN > - if 'vlan_id' in params and \ > - (netinfo.is_bridge(iface) or params['connection'] == > "bridge"): > + def _set_network_macvtap(self, params): > + self._check_network_interface(params) > + > + iface = params['interface'] > + if 'vlan_id' in params or not (netinfo.is_bare_nic(iface) or > + netinfo.is_bonding(iface)): > + raise InvalidParameter('KCHNET0028E', {'name': iface}) > + > + # set macvtap network > + params['forward'] = {'mode': 'bridge', 'dev': iface} > + > + def _set_network_bridge(self, params): > + self._check_network_interface(params) > + params['forward'] = {'mode': 'bridge'} > + > + # Bridges cannot be the trunk device of a VLAN > + iface = params['interface'] > + if 'vlan_id' in params and netinfo.is_bridge(iface): > raise InvalidParameter('KCHNET0019E', {'name': iface}) > > # User specified bridge interface, simply use it > @@ -189,29 +203,22 @@ class NetworksModel(object): > if netinfo.is_ovs_bridge(iface): > params['ovs'] = True > > - # OVS bridges don't work with macvtap > - if params['connection'] != "bridge": > - raise InvalidParameter('KCHNET0026E') > - > - # User wants Linux bridge network, but didn't specify bridge > interface > - elif params['connection'] == "bridge": > - > - # libvirt will fail to create bridge if NetworkManager is > enabled > - if self.caps.nm_running: > - raise InvalidParameter('KCHNET0027E') > - > - # create Linux bridge interface first and use it as > actual iface > - iface = self._create_linux_bridge(iface) > - params['bridge'] = iface > - > # connection == macvtap and iface is not bridge > elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface): > - if params.get('vlan_id') is None: > - params['forward']['dev'] = iface > - else: > + if 'vlan_id' in params: > params['bridge'] = \ > self._create_vlan_tagged_bridge(str(iface), > > str(params['vlan_id'])) > + else: > + # libvirt bridge creation will fail with > NetworkManager enabled > + if self.caps.nm_running: > + raise InvalidParameter('KCHNET0027E') > + > + # create Linux bridge interface and use it as actual > iface > + iface = self._create_linux_bridge(iface) > + params['bridge'] = iface > + > + # unrecognized interface type: fail > else: > raise InvalidParameter("KCHNET0007E") >