[PATCH 1/4] RecipeParser: sepparate vlan parsing/validation

[Ondrej Lichtner]

From: Ondrej Lichtner

This commit separates the parsing/validation of <vlan> elements from the
group of virtual devices. The main reason is to check that the element
has _exactly one_ slave defined. Checking this in a different place
would be problematic...

Signed-off-by: Ondrej Lichtner 
---
 lnst/Controller/RecipeParser.py | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/lnst/Controller/RecipeParser.py b/lnst/Controller/RecipeParser.py
index 046d6d9..df0b47a 100644
--- a/lnst/Controller/RecipeParser.py
+++ b/lnst/Controller/RecipeParser.py
@@ -105,7 +105,7 @@ class RecipeParser(XmlParser):

if iface["type"] == "eth":
             iface["network"] = self._get_attribute(iface_tag, "label")
-        elif iface["type"] in ["bond", "bridge", "vlan", "macvlan", "team"]:
+        elif iface["type"] in ["bond", "bridge", "macvlan", "team"]:
             # slaves
             slaves_tag = iface_tag.find("slaves")
             if slaves_tag is not None and len(slaves_tag) > 0:
@@ -127,6 +127,27 @@ class RecipeParser(XmlParser):
             opts = self._proces_options(opts_tag)
             if len(opts) > 0:
                 iface["options"] = opts
+        elif iface["type"] in ["vlan"]:
+            # real_dev of the VLAN interface
+            slaves_tag = iface_tag.find("slaves")
+            if slaves_tag is None or len(slaves_tag) != 1:
+                msg = "VLAN '%s' need exactly one slave definition."\
+                        % iface["id"]
+                raise RecipeError(msg, vlan)
+
+            iface["slaves"] = XmlCollection(slaves_tag)
+
+            slave_tag = slaves_tag[0]
+            slave = XmlData(slave_tag)
+            slave["id"] = self._get_attribute(slave_tag, "id")
+
+            iface["slaves"].append(slave)
+
+            # interface options
+            opts_tag = iface_tag.find("options")
+            opts = self._proces_options(opts_tag)
+            if len(opts) > 0:
+                iface["options"] = opts
         elif iface["type"] == "ovs_bridge":
             slaves_tag = iface_tag.find("slaves")
             iface["slaves"] = XmlCollection(slaves_tag)

[PATCH 2/4] Machine: allow multiple non-primary interface masters

[Ondrej Lichtner]

From: Ondrej Lichtner

In the kernel a net_device uses two lists (upper and lower) to create
a hierarchy between the interfaces. This patch makes LNST reflect that
hierarchy - lower == Interface._slaves and upper == Interface._master
the patch also sepparates the master list into two parts - the _primary_
master (e.g. bridge, bond, team) and _other_ masters (e.g. vlan
interfaces). There can only be one primary master!

Since the other masters are not required by the slave configuration
(yet) the Slave modules don't have to be changed.

Signed-off-by: Ondrej Lichtner 
---
 lnst/Controller/Machine.py | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/lnst/Controller/Machine.py b/lnst/Controller/Machine.py
index e2f1d3e..56566a5 100644
--- a/lnst/Controller/Machine.py
+++ b/lnst/Controller/Machine.py
@@ -126,7 +126,7 @@ class Machine(object):
             ind1 = 0
             ind2 = 0
             for i in ordered_list:
-                master = i.get_master()
+                master = i.get_primary_master()
                 if master != None:
                     ind1 = ordered_list.index(i)
                     ind2 = ordered_list.index(master)
@@ -391,7 +391,7 @@ class Interface(object):
         self._addresses = []
         self._options = []

-        self._master = None
+        self._master = {"primary": None, "other": []}

self._ovs_conf = None

@@ -431,19 +431,26 @@ class Interface(object):
     def set_option(self, name, value):
         self._options.append((name, value))

-    def set_master(self, master):
-        if self._master != None:
-            msg = "Interface %s already has a master." % self._master.get_id()
+    def add_master(self, master, primary=True):
+        if primary and self._master["primary"] != None:
+            msg = "Interface %s already has a primary master."\
+                    % self._master.get_id()
             raise MachineError(msg)
         else:
-            self._master = master
+            if primary:
+                self._master["primary"] = master
+            else:
+                self._master["other"].append(master)

-    def get_master(self):
-        return self._master
+    def get_primary_master(self):
+        return self._master["primary"]

def add_slave(self, iface):
         self._slaves[iface.get_id()] = iface
-        iface.set_master(self)
+        if self._type in ["vlan"]:
+            iface.add_master(self, primary=False)
+        else:
+            iface.add_master(self)

def set_slave_option(self, slave_id, name, value):
         if slave_id not in self._slave_options:
@@ -473,10 +480,14 @@ class Interface(object):
                   "addresses": self._addresses, "slaves": self._slaves.keys(),
                   "options": self._options,
                   "slave_options": self._slave_options,
-                  "master": None, "ovs_conf": self._ovs_conf}
+                  "master": None, "other_masters": [],
+                  "ovs_conf": self._ovs_conf}
+
+        if self._master["primary"] != None:
+            config["master"] = self._master["primary"].get_id()

-        if self._master != None:
-            config["master"] = self._master.get_id()
+        for m in self._master["other"]:
+            config["other_masters"].append(m.get_id())

return config

[PATCH 3/4] InterfaceManager: use conf["name"] when Device not initialized

[Ondrej Lichtner]

From: Ondrej Lichtner

When using NetworkManager we don't create devices until we have created
connection objects for all the configured interfaces. The corresponding
Device objects therefore weren't initialized which caused crashes when
creating connection objects of devices that were higher in the
hierarchy. This patch fixes that.

Signed-off-by: Ondrej Lichtner 
---
 lnst/Slave/InterfaceManager.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lnst/Slave/InterfaceManager.py b/lnst/Slave/InterfaceManager.py
index 547662d..43c53eb 100644
--- a/lnst/Slave/InterfaceManager.py
+++ b/lnst/Slave/InterfaceManager.py
@@ -176,6 +176,7 @@ class InterfaceManager(object):

class Device(object):
     def __init__(self, if_manager):
+        self._initialized = False
         self._configured = False

self._if_index = None
@@ -198,6 +199,8 @@ class Device(object):
         self._ip = None #TODO
         self._master = nl_msg.get_attr("IFLA_MASTER")

+        self._initialized = True
+
     def update_netlink(self, nl_msg):
         if self._if_index == nl_msg['index']:
             self._hwaddr = normalize_hwaddr(nl_msg.get_attr("IFLA_ADDRESS"))
@@ -217,6 +220,8 @@ class Device(object):
             if self._conf_dict:
                 self._conf_dict["name"] = self._name

+            self._initialized = True
+
             #return an update message that will be sent to the controller
             return {"type": "if_update",
                     "devname": self._name,
@@ -248,6 +253,9 @@ class Device(object):
         self._conf_dict = conf
         self._conf = NetConfigDevice(conf, self._if_manager)

+        if not self._initialized:
+            self._name = conf["name"]
+
     def get_configuration(self):
         return self._conf

[PATCH 4/4] InterfaceManager: reconstruct upper/lower hierarchy

[Ondrej Lichtner]

From: Ondrej Lichtner

This patch makes the internal Device objects reconstruct the upper/lower
device hierarchy that can be found in the kernel. This is usefull when
we are deconfiguring devices without information from the kernel - the
upper and lower references will ensure the proper ordering of
deconfiguration.

Internal variables are named like on the controller:
upper == master
lower == slaves
master["primary"] is the primary master of an interface (IFLA_MASTER)

Signed-off-by: Ondrej Lichtner 
---
 lnst/Slave/InterfaceManager.py | 96 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 79 insertions(+), 17 deletions(-)

diff --git a/lnst/Slave/InterfaceManager.py b/lnst/Slave/InterfaceManager.py
index 43c53eb..d00ee4d 100644
--- a/lnst/Slave/InterfaceManager.py
+++ b/lnst/Slave/InterfaceManager.py
@@ -91,6 +91,7 @@ class InterfaceManager(object):
                 self._devices[msg['index']] = dev
         elif msg['header']['type'] == RTM_DELLINK:
             if msg['index'] in self._devices:
+                self._devices[msg['index']].del_link()
                 del self._devices[msg['index']]
         else:
             return
@@ -186,8 +187,8 @@ class Device(object):
         self._conf_dict = None
         self._ip = None
         self._state = None
-        self._master = None
-        self._parent = None
+        self._master = {"primary": None, "other": []}
+        self._slaves = []

self._if_manager = if_manager

@@ -197,7 +198,7 @@ class Device(object):
         self._name = nl_msg.get_attr("IFLA_IFNAME")
         self._state = nl_msg.get_attr("IFLA_OPERSTATE")
         self._ip = None #TODO
-        self._master = nl_msg.get_attr("IFLA_MASTER")
+        self.set_master(nl_msg.get_attr("IFLA_MASTER"), primary=True)

self._initialized = True

@@ -206,7 +207,8 @@ class Device(object):
             self._hwaddr = normalize_hwaddr(nl_msg.get_attr("IFLA_ADDRESS"))
             self._name = nl_msg.get_attr("IFLA_IFNAME")
             self._ip = None #TODO
-            self._master = nl_msg.get_attr("IFLA_MASTER")
+            self.set_master(nl_msg.get_attr("IFLA_MASTER"), primary=True)
+
             link = nl_msg.get_attr("IFLA_LINK")
             if link != None:
                 # IFLA_LINK is an index of device that's closer to physical
@@ -215,7 +217,12 @@ class Device(object):
                 # parent index in the child device; this is the opposite
                 # to IFLA_MASTER
                 link_dev = self._if_manager.get_device(link)
-                link_dev.set_parent(self._if_index)
+                if link_dev != None:
+                    link_dev.set_master(self._if_index, primary=False)
+                # This reference shouldn't change - you can't change the realdev
+                # of a vlan, you need to create a new vlan. Therefore the
+                # the following add_slave shouldn't be a problem.
+                self.add_slave(link)

if self._conf_dict:
                 self._conf_dict["name"] = self._name
@@ -228,6 +235,23 @@ class Device(object):
                     "hwaddr": self._hwaddr}
         return None

+    def del_link(self):
+        if self._master["primary"]:
+            primary_id = self._master["primary"]
+            primary_dev = self._if_manager.get_device(primary_id)
+            if primary_dev:
+                primary_dev.del_slave(self._if_index)
+
+        for m_id in self._master["other"]:
+            m_dev = self._if_manager.get_device(m_id)
+            if m_dev:
+                m_dev.del_slave(self._if_index)
+
+        for dev_id in self._slaves:
+            dev = self._if_manager.get_device(dev_id)
+            if dev != None:
+                dev.del_master(self._if_index)
+
     def get_if_index(self):
         return self._if_index

@@ -260,11 +284,16 @@ class Device(object):
         return self._conf

def clear_configuration(self):
-        if self._master or self._parent:
-            super_id = self._master if self._master else self._parent
-            super_dev = self._if_manager.get_device(super_id)
-            if super_dev:
-                super_dev.clear_configuration()
+        if self._master["primary"]:
+            primary_id = self._master["primary"]
+            primary_dev = self._if_manager.get_device(primary_id)
+            if primary_dev:
+                primary_dev.clear_configuration()
+
+        for m_id in self._master["other"]:
+            m_dev = self._if_manager.get_device(m_id)
+            if m_dev:
+                m_dev.clear_configuration()

if self._conf != None:
             self.down()
@@ -272,8 +301,36 @@ class Device(object):
             self._conf = None
             self._conf_dict = None

-    def set_parent(self, if_index):
-        self._parent = if_index
+    def set_master(self, if_index, primary=True):
+        if primary:
+            prev_master_id = self._master["primary"]
+            if prev_master_id != None and if_index != prev_master_id:
+                prev_master_dev = self._if_manager.get_device(prev_master_id)
+                if prev_master_dev != None:
+                    prev_master_dev.del_slave(self._if_index)
+
+            self._master["primary"] = if_index
+            if self._master["primary"] != None:
+                master_id = self._master["primary"]
+                master_dev = self._if_manager.get_device(master_id)
+                if master_dev != None:
+                    master_dev.add_slave(self._if_index)
+        elif if_index not in self._master["other"]:
+            self._master["other"].append(if_index)
+
+    def del_master(self, if_index):
+        if self._master["primary"] == if_index:
+            self._master["primary"] = None
+        elif if_index in self._master["other"]:
+            self._master["other"].remove(if_index)
+
+    def add_slave(self, if_index):
+        if if_index not in self._slaves:
+            self._slaves.append(if_index)
+
+    def del_slave(self, if_index):
+        if if_index in self._slaves:
+            self._slaves.remove(if_index)

def configure(self):
         if self._conf != None and not self._configured:
@@ -281,11 +338,16 @@ class Device(object):
             self._configured = True

def deconfigure(self):
-        if self._master or self._parent:
-            super_id = self._master if self._master else self._parent
-            super_dev = self._if_manager.get_device(super_id)
-            if super_dev:
-                super_dev.deconfigure()
+        if self._master["primary"]:
+            primary_id = self._master["primary"]
+            primary_dev = self._if_manager.get_device(primary_id)
+            if primary_dev:
+                primary_dev.deconfigure()
+
+        for m_id in self._master["other"]:
+            m_dev = self._if_manager.get_device(m_id)
+            if m_dev:
+                m_dev.deconfigure()

if self._conf != None and self._configured:
             self._conf.deconfigure()

[Jiri Pirko]

patchset applied. Thanks!

Mon, May 12, 2014 at 03:43:30PM CEST, [e-mail] wrote:
> From: Ondrej Lichtner
>
> This commit separates the parsing/validation of <vlan> elements from the
> group of virtual devices. The main reason is to check that the element
> has _exactly one_ slave defined. Checking this in a different place
> would be problematic...
> Signed-off-by: Ondrej Lichtner
>
> lnst/Controller/RecipeParser.py | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/lnst/Controller/RecipeParser.py b/lnst/Controller/RecipeParser.py
> index 046d6d9..df0b47a 100644
> --- a/lnst/Controller/RecipeParser.py
> +++ b/lnst/Controller/RecipeParser.py
> @@ -105,7 +105,7 @@ class RecipeParser(XmlParser):
>     if iface["type"] == "eth":
>         iface["network"] = self._get_attribute(iface_tag, "label")
>    elif iface["type"] in ["bond", "bridge", "vlan", "macvlan", "team"]:
>    elif iface["type"] in ["bond", "bridge", "macvlan", "team"]:
>       # slaves
>       slaves_tag = iface_tag.find("slaves")
>       if slaves_tag is not None and len(slaves_tag) > 0:
>
> @@ -127,6 +127,27 @@ class RecipeParser(XmlParser):
>             opts = self._proces_options(opts_tag)
>             if len(opts) > 0:
>                 iface["options"] = opts
>    elif iface["type"] in ["vlan"]:
>        # real_dev of the VLAN interface
>        slaves_tag = iface_tag.find("slaves")
>        if slaves_tag is None or len(slaves_tag) != 1:
>            msg = "VLAN '%s' need exactly one slave definition."\
>                    % iface["id"]
>            raise RecipeError(msg, vlan)
>        iface["slaves"] = XmlCollection(slaves_tag)
>        slave_tag = slaves_tag[0]
>        slave = XmlData(slave_tag)
>        slave["id"] = self._get_attribute(slave_tag, "id")
>        iface["slaves"].append(slave)
>        # interface options
>        opts_tag = iface_tag.find("options")
>        opts = self._proces_options(opts_tag)
>        if len(opts) > 0:
>            iface["options"] = opts
>   elif iface["type"] == "ovs_bridge":
>       slaves_tag = iface_tag.find("slaves")
>       iface["slaves"] = XmlCollection(slaves_tag)
>
> -- 
> 1.9.0
>
> LNST-developers mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers