Re: [Kimchi-devel] [PATCH] [Kimchi 1/2] Add backend support for editing virtual networks

On 04/12/2016 11:47 AM, Lucio Correia wrote: > Signed-off-by: Lucio Correia <luciojhc(a)linux.vnet.ibm.com&gt; > --- > API.json | 31 +++++++++++++++++++++++++++++++ > control/networks.py | 1 + > docs/API.md | 8 ++++++++ > i18n.py | 9 ++++++--- > model/networks.py | 50 > +++++++++++++++++++++++++++++++++++++++++++++++++- > 5 files changed, 95 insertions(+), 4 deletions(-) > > diff --git a/API.json b/API.json > index 6f0b484..2205be6 100644 > --- a/API.json > +++ b/API.json > @@ -395,6 +395,37 @@ > } > } > }, > + "network_update": { > + "type": "object", > + "error": "KCHAPI0001E", > + "properties": { > + "name": { > + "description": "The new name of the network", > + "type": "string", > + "minLength": 1, > + "pattern": "^[^/\"]*$", > + "required": true, I should be able to update all the other fields without changing the name. So there is no need to set is as required.

> + "error": "KCHNET0011E" > + }, > + "subnet": { > + "description": "Network segment in > slash-separated format with ip address and prefix or netmask", > + "type": "string", > + "error": "KCHNET0013E" > + }, > + "interfaces": { > + "description": "An array of network interfaces of > the host", > + "type": "array", > + "error": "KCHNET0014E" > + }, Set the minLength as 1 - so we make sure to have at least one interface.

> + "vlan_id": { > + "description": "Network's VLAN ID", > + "type": "integer", > + "maximum": 4094, > + "minimum": 1, > + "error": "KCHNET0015E" > + } > + } > + }, > "vmifaces_create": { > "type": "object", > "error": "KCHVMIF0007E", > diff --git a/control/networks.py b/control/networks.py > index 8ba2206..c87b5a6 100644 > --- a/control/networks.py > +++ b/control/networks.py > @@ -27,6 +27,7 @@ NETWORKS_REQUESTS = { > > NETWORK_REQUESTS = { > 'DELETE': {'default': "Remove virtual network '%(ident)s'"}, > + 'PUT': {'default': "Update virtual network '%(ident)s'"}, > 'POST': { > 'activate': "Activate virtual network '%(ident)s'", > 'deactivate': "Deactivate virtual network '%(ident)s'", > diff --git a/docs/API.md b/docs/API.md > index f1a13b4..491e887 100644 > --- a/docs/API.md > +++ b/docs/API.md > @@ -736,6 +736,14 @@ A interface represents available interface on host. > * **DELETE**: Remove the Network > * **POST**: *See Network Actions* > > +* **PUT**: Update the Network. The network type cannot be changed. > + * name *(optional)*: The new name of the Network > + * subnet *(optional)*: Network segment in slash-separated format > with ip address and prefix. > + Applies only to NAT and isolated networks > + * interfaces *(optional)*: An array of network interfaces that > belongs to this network. > + Applies only to bridge, macvtap and VEPA networks. 'For bridge and macvtap, only one interface is allowed. For VEPA networks, you can specify multiple interfaces'

> + * vlan_id *(optional)*: VLAN tagging ID for the bridge network. > + > **Actions (POST):** > > * activate: Activate an inactive Network > diff --git a/i18n.py b/i18n.py > index 6214687..b413dcd 100644 > --- a/i18n.py > +++ b/i18n.py > @@ -255,10 +255,10 @@ messages = { > "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 > or macvtap networks."), > - "KCHNET0005E": _("Unable to delete active network %(name)s"), > + "KCHNET0005E": _("Unable to delete or update 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."), > - "KCHNET0008E": _("Unable to create network %(name)s. Details: > %(err)s"), > + "KCHNET0008E": _("Unable to create or update network %(name)s. > Details: %(err)s"), It'd be better to have separated error message for update instead of trying to reuse an existing one.

> "KCHNET0009E": _("Unable to find a free IP address for network > '%(name)s'"), > "KCHNET0010E": _("The interface %(iface)s already exists."), > "KCHNET0011E": _("Network name must be a string without slashes > (/) or quotes (\")"), > @@ -267,7 +267,7 @@ messages = { > "KCHNET0014E": _("Network interfaces must be an array."), > "KCHNET0015E": _("Network VLAN ID must be an integer between 1 > and 4094"), > "KCHNET0016E": _("Specify name and type to create a Network"), > - "KCHNET0017E": _("Unable to delete network %(name)s. There are > some virtual machines %(vms)s and/or templates linked to this network."), > + "KCHNET0017E": _("Unable to delete or update network %(name)s. > There are some virtual machines %(vms)s and/or templates linked to > this network."), Is there a problem to edit a network in use if the user does not change the name? Maybe, we only need to block the name in this case (and proper add an information about it on UI)

> "KCHNET0018E": _("Unable to deactivate network %(name)s. There > are some virtual machines %(vms)s and/or templates linked to this > network."), > "KCHNET0019E": _("Bridge device %(name)s can not be the trunk > device of a VLAN."), > "KCHNET0020E": _("Failed to activate interface %(iface)s: > %(err)s."), > @@ -279,6 +279,9 @@ messages = { > "KCHNET0028E": _("Interface should be bare NIC or bonding."), > "KCHNET0029E": _("Network interfaces parameter must contain at > least one interface."), > "KCHNET0030E": _("Only one interface is allowed for 'bridge' and > 'macvtap' networks."), > + "KCHNET0031E": _("Invalid parameter 'connection'. Virtual network > type cannot be changed."), To avoid the need to manually check it, you can add "additionalProperties: false' to API.json That way, only the parameters specified there will be allowed. For example: "additionalProperties": false, "error": "<error code>" The error message will be something like: "To update a virtual network only the following parameters are allowed: name, subnet, .."

> + "KCHNET0032E": _("Subnet is not a valid parameter for this type > of virtual network."), > + "KCHNET0033E": _("VLAN ID and interfaces are not valid parameters > for this type of virtual network."), > > "KCHSR0001E": _("Storage server %(server)s was not used by > Kimchi"), > > diff --git a/model/networks.py b/model/networks.py > index 2664af3..fdf102a 100644 > --- a/model/networks.py > +++ b/model/networks.py > @@ -17,6 +17,7 @@ > # License along with this library; if not, write to the Free Software > # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301 USA > > +import copy > import ipaddr > import libvirt > import sys > @@ -33,6 +34,8 @@ from wok.plugins.kimchi import netinfo > from wok.plugins.kimchi import network as knetwork > from wok.plugins.kimchi.config import kimchiPaths > from wok.plugins.kimchi.model.config import CapabilitiesModel > +from wok.plugins.kimchi.netinfo import get_vlan_device, is_bridge, > is_vlan > +from wok.plugins.kimchi.netinfo import ports > from wok.plugins.kimchi.osinfo import defaults as tmpl_defaults > from wok.plugins.kimchi.xmlutils.interface import get_iface_xml > from wok.plugins.kimchi.xmlutils.network import create_linux_bridge_xml > @@ -109,7 +112,7 @@ class NetworksModel(object): > > try: > network = conn.networkDefineXML(xml.encode("utf-8")) > - network.setAutostart(True) > + network.setAutostart(params.get('autostart', True)) > except libvirt.libvirtError as e: > raise OperationFailed("KCHNET0008E", > {'name': name, 'err': > e.get_error_message()}) > @@ -339,6 +342,7 @@ class NetworkModel(object): > def __init__(self, **kargs): > self.conn = kargs['conn'] > self.objstore = kargs['objstore'] > + self.collection = NetworksModel(**kargs) > > def lookup(self, name): > network = self.get_network(self.conn.get(), name) > @@ -499,3 +503,47 @@ class NetworkModel(object): > iface = conn.interfaceLookupByName(bridge) > iface.isActive() and iface.destroy(0) > iface.undefine() > + > + def update(self, name, params): > + info = self.lookup(name) > + original = copy.deepcopy(info) > + original['name'] = name > + > + # validate parameters > + if 'connection' in params: > + raise InvalidParameter("KCHNET0031E") > + If you change API.json as I commented above, this verification is not needed.

> + connection = info['connection'] > + if connection in ['bridge', 'macvtap', 'vepa']: > + if params.get('subnet'): > + raise InvalidParameter("KCHNET0032E") > + elif connection in ['nat', 'isolated']: > + if params.get('vlan_id') or params.get('interfaces'): > + raise InvalidParameter("KCHNET0033E") > + > + # get target device if bridge was created by Kimchi > + if connection == 'bridge': > + iface = info['interfaces'][0] Verify only ONE interface is passed for bridge and macvtap and raise an error otherwise. Multiples interfaces are only allowed for VEPA networks.

> + if is_bridge(iface) and > iface.startswith(KIMCHI_BRIDGE_PREFIX): > + port = ports(iface)[0] > + if is_vlan(port): > + info['interfaces'] = [get_vlan_device(port)] > + # nic > + else: > + info['interfaces'] = [port] > + > + # merge parameters > + info.update(params) > + > + # delete original network > + self.delete(name) > + > + try: > + # create new network > + network = self.collection.create(info) > + except: > + # restore original network > + self.collection.create(original) > + raise > + > + return network