On 10-02-2016 14:41, Ramon Medeiros wrote:
- Allow VLANs only for bridged virtual networks - Code simplification
Signed-off-by: Lucio Correia <luciojhc@linux.vnet.ibm.com> --- 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
On 02/10/2016 02:38 PM, Lucio Correia wrote: put them as constant?
'nat' is only used here, but 'bridge' is spread out through all the code and has two different, independent meanings. Considering that, I prefer to leave them hardcoded for now, since the changes would require lots of more tests than the focus of this patch.
- # 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")
-- Lucio Correia Software Engineer IBM LTC Brazil