On Mon, May 15, 2017 at 04:03:16PM +0200, Jiri Pirko wrote:
Thu, May 11, 2017 at 02:41:04PM CEST, olichtne(a)redhat.com wrote:
>From: Ondrej Lichtner <olichtne(a)redhat.com>
>
>This package contains the implementation of Device
>configuration/manipulation classes that are currently supported by LNST.
>This implementation has been moved into a separate package that will be
>distributed together with the Controller package. These will then be
>sent to the Slave during recipe execution so the slave can use them when
>required.
>
>The reasoning behind this is that it is often easier to update or make
>changes on the Controller than on all the Slaves. Making the Slave
>simpler and independent of the implementation of device configuration
>means we don't need to update all our slaves as often and also that
>changes to the Device configuration implementation don't require any
>changes in Slave or Controller code.
>
>Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com>
>---
> lnst/Devices/BondDevice.py | 48 ++++++
> lnst/Devices/BridgeDevice.py | 49 ++++++
> lnst/Devices/Device.py | 368 ++++++++++++++++++++++++++++++++++++++++
> lnst/Devices/MacvlanDevice.py | 41 +++++
> lnst/Devices/OvsBridgeDevice.py | 116 +++++++++++++
> lnst/Devices/RemoteDevice.py | 90 ++++++++++
> lnst/Devices/SoftDevice.py | 51 ++++++
> lnst/Devices/TeamDevice.py | 64 +++++++
> lnst/Devices/VethDevice.py | 59 +++++++
> lnst/Devices/VethPair.py | 24 +++
> lnst/Devices/VirtualDevice.py | 99 +++++++++++
> lnst/Devices/VlanDevice.py | 38 +++++
> lnst/Devices/VtiDevice.py | 71 ++++++++
> lnst/Devices/VxlanDevice.py | 66 +++++++
> lnst/Devices/__init__.py | 45 +++++
> 15 files changed, 1229 insertions(+)
> create mode 100644 lnst/Devices/BondDevice.py
> create mode 100644 lnst/Devices/BridgeDevice.py
> create mode 100644 lnst/Devices/Device.py
> create mode 100644 lnst/Devices/MacvlanDevice.py
> create mode 100644 lnst/Devices/OvsBridgeDevice.py
> create mode 100644 lnst/Devices/RemoteDevice.py
> create mode 100644 lnst/Devices/SoftDevice.py
> create mode 100644 lnst/Devices/TeamDevice.py
> create mode 100644 lnst/Devices/VethDevice.py
> create mode 100644 lnst/Devices/VethPair.py
> create mode 100644 lnst/Devices/VirtualDevice.py
> create mode 100644 lnst/Devices/VlanDevice.py
> create mode 100644 lnst/Devices/VtiDevice.py
> create mode 100644 lnst/Devices/VxlanDevice.py
> create mode 100644 lnst/Devices/__init__.py
>
[...]
>diff --git a/lnst/Devices/BridgeDevice.py b/lnst/Devices/BridgeDevice.py
>new file mode 100644
>index 0000000..4babe8a
>--- /dev/null
>+++ b/lnst/Devices/BridgeDevice.py
>@@ -0,0 +1,49 @@
>+"""
>+Defines the BridgeDevice class.
>+
>+Copyright 2017 Red Hat, Inc.
>+Licensed under the GNU General Public License, version 2 as
>+published by the Free Software Foundation; see COPYING for details.
>+"""
>+
>+__author__ = """
>+olichtne(a)redhat.com (Ondrej Lichtner)
>+"""
>+
>+import logging
>+from lnst.Common.ExecCmd import exec_cmd
>+from lnst.Devices.SoftDevice import SoftDevice
>+
>+class BridgeDevice(SoftDevice):
>+ _name_template = "t_br"
>+
>+ @property
>+ def slaves(self):
>+ ret = []
>+
>+ for dev in self._if_manager.get_devices():
>+ if dev.master is self:
>+ ret.append(dev)
>+ return ret
Why is this specific to bridge? That should be defined in "class
Device", shouldn't it?
Since not all soft device types support slaves I think it's more
appropriate to not have a "slave_add" method at all to indicate that,
instead of having a common method that raises an Exception when the
operation fails.
>+
>+ def create(self):
>+ exec_cmd("ip link add dev {} type bridge".format(self._name))
>+
>+ def slave_add(self, dev):
>+ dev.master_set(self)
This could be also put to a common code. But I don't mind :)
Same as above.
So I wouldn't put it in the Device class... but I guess maybe I could
add one more class layer for "Device types that can have slaves" that
would inherit from SoftDevice and implement 'slave*' methods?
Any idea how to call this class? MasterSoftDevice? MasterDevice? Nothing
good comes to mind...
>+
>+ def slave_del(self, dev):
>+ dev.master_set(None)
[...]
>+class Device(object):
>+ """The base Device class
>+
>+ Implemented using the pyroute2 package to access different attributes of
>+ a kernel netdevice object.
>+ Changing attributes of a netdevice is right now implemented by calling
>+ shell commands (e.g. from iproute2 package).
Yeah, we should eventually convert this as well.
Agreed, but this can be done "in background" later as it won't change
the APIs
[...]
>+
>+ @property
>+ def ifi_type(self):
What is "ifi"?
to be honest... i don't know. I assumed this was the correct name since
it's what pyroute2 calls it, but now that I'm look at the kernel code I
guess it just refers to the type attribute of the net_device struct and
the 'ifi' can be removed?
>+ """ifi_type attribute
>+
>+ Returns the integer type of the device as reported by the kernel.
>+ """
>+ return self._nl_msg['ifi_type']
>+
>+ @property
>+ def name(self):
>+ """name attribute
>+
>+ Returns string name of the device as reported by the kernel.
>+ """
>+ return self._nl_msg.get_attr("IFLA_IFNAME")
I think that now is a good time to sit down and review the names of the
getters and setters. I think that we might want to clean this up a bit.
Agreed, that's kind of the point of the patchset review. I kind of
arbitrarily decided on most of these so I'm waiting for feedback if you
have better ideas.
>+
>+ @property
>+ def hwaddr(self):
>+ """hwaddr attribute
>+
>+ Returns string hardware address of the device as reported by the kernel.
>+ """
>+ return normalize_hwaddr(self._nl_msg.get_attr("IFLA_ADDRESS"))
>+
>+ @property
>+ def state(self):
>+ """state attribute
>+
>+ Returns string state of the device as reported by the kernel.
>+ """
>+ #TODO check flags for admin up and lower up!!!
>+ #TODO or expand this to check all possibilities?
>+ #TODO also, add passive wait until lower up, with timeout
>+ return self._nl_msg.get_attr("IFLA_OPERSTATE")
>+
>+ @property
>+ def ips(self):
>+ """list of configured ip addresses
>+
>+ Returns list of BaseIpAddress objects.
>+ """
>+ return self._ip_addrs
>+
>+ @property
>+ def mtu(self):
>+ """mtu attribute
>+
>+ Returns integer mtu as reported by the kernel.
>+ """
>+ return self._nl_msg.get_attr("IFLA_MTU")
>+
>+ @property
>+ def master(self):
>+ """master device
>+
>+ Returns Device object of the master device or None when the device has
>+ no master.
>+ """
>+ master_if_index = self._nl_msg.get_attr("IFLA_MASTER")
>+ if master_if_index is not None:
>+ return self._if_manager.get_device(master_if_index)
>+ else:
>+ return None
>+
>+ @property
>+ def driver(self):
>+ """driver attribute
>+
>+ Returns string name of the device driver based on an ethtool -i call
>+ """
>+ if self.ifi_type == 772: #loopback ifi type
>+ return 'loopback'
>+ out, _ = exec_cmd("ethtool -i %s" % self.name, False, False,
False)
>+ match = re.search("^driver: (.*)$", out, re.MULTILINE)
>+ if match is not None:
>+ return match.group(1)
>+ else:
>+ return None
>+
>+ @property
>+ def link_stats(self):
>+ """Link statistics
>+
>+ Returns dictionary of interface statistics returned by ip -s link show
>+ """
>+ stats = {"devname": self.name,
>+ "hwaddr": self.hwaddr}
>+ out, _ = exec_cmd("ip -s link show %s" % self.name)
getter using "ip"? That contradics what you write in the comment above.
Slipped my mind, will change.
>+ lines = iter(out.split("\n"))
>+ for line in lines:
>+ if (len(line.split()) == 0):
>+ continue
>+ if (line.split()[0] == "RX:"):
>+ rx_stats = map(int, lines.next().split())
>+ stats.update({"rx_bytes" : rx_stats[0],
>+ "rx_packets": rx_stats[1],
>+ "rx_errors" : rx_stats[2],
>+ "rx_dropped": rx_stats[3],
>+ "rx_overrun": rx_stats[4],
>+ "rx_mcast" : rx_stats[5]})
>+ if (line.split()[0] == "TX:"):
>+ tx_stats = map(int, lines.next().split())
>+ stats.update({"tx_bytes" : tx_stats[0],
>+ "tx_packets": tx_stats[1],
>+ "tx_errors" : tx_stats[2],
>+ "tx_dropped": tx_stats[3],
>+ "tx_carrier": tx_stats[4],
>+ "tx_collsns": tx_stats[5]})
>+ return stats
>+
>+ def _clear_ips(self):
>+ self._ip_addrs = []
>+
>+ def _clear_tc_qdisc(self):
>+ exec_cmd("tc qdisc replace dev %s root pfifo" % self.name)
>+ out, _ = exec_cmd("tc filter show dev %s" % self.name)
>+ ingress_handles = re.findall("ingress (\\d+):", out)
>+ for ingress_handle in ingress_handles:
>+ exec_cmd("tc qdisc del dev %s handle %s: ingress" %
>+ (self.name, ingress_handle))
>+ out, _ = exec_cmd("tc qdisc show dev %s" % self.name)
>+ ingress_qdiscs = re.findall("qdisc ingress (\\w+):", out)
>+ if len(ingress_qdiscs) != 0:
>+ exec_cmd("tc qdisc del dev %s ingress" % self.name)
>+
>+ def _clear_tc_filters(self):
>+ out, _ = exec_cmd("tc filter show dev %s" % self.name)
>+ egress_prefs = re.findall("pref (\\d+) .* handle", out)
>+
>+ for egress_pref in egress_prefs:
>+ exec_cmd("tc filter del dev %s pref %s" % (self.name,
>+ egress_pref))
>+
>+ def ip_add(self, addr):
>+ """add an ip address
>+
>+ Args:
>+ addr -- accepts a BaseIpAddress object
>+ """
>+ if addr not in self.ips:
>+ exec_cmd("ip addr add %s/%d dev %s" % (addr, addr.prefixlen,
>+ self.name))
>+
>+ def ip_del(self, addr):
>+ """remove an ip address
>+
>+ Args:
>+ addr -- accepts a BaseIpAddress object
>+ """
>+ if addr in self.ips:
>+ exec_cmd("ip addr del %s/%d dev %s" % (addr, addr.prefixlen,
>+ self.name))
>+
>+ def ip_flush(self):
>+ """flush all ip addresses of the device"""
>+ for ip in self.ips:
>+ self.ip_del(ip)
>+
>+ def master_set(self, dev):
"master" is a nice example of a change I would like to see. Now you have
getter "master" and setter "master_set". I think what we need only
"master" to read and write.
With the @property decorator, this is actually very easy, really I just
need to add a "(a)master.setter" line above the "def master_set" line
and
it will work as you described, we can use this approach in other
appropriate places as well.
>+ """set dev as the master of this device
>+
>+ Args:
>+ dev -- accepts a Device object of the master object.
>+ When None, removes the current master from the
Device."""
>+ if isinstance(dev, Device):
>+ exec_cmd("ip link set %s master %s" % (self.name, dev.name))
>+ elif dev is None:
>+ exec_cmd("ip link set %s nomaster" % self.name)
>+ else:
>+ raise DeviceError("Invalid dev argument.")
>+
>+ def up(self):
>+ """set device up"""
>+ exec_cmd("ip link set %s up" % self.name)
>+
>+ def down(self):
>+ """set device down"""
>+ exec_cmd("ip link set %s down" % self.name)
>+
>+ def add_route(self, dest):
Another thing. "add_route" vs "ip_add". We should be consistent in:
WHAT_OP
Agreed, must have missed this.
>+ """add specified route for this device
>+
>+ Args:
>+ dest -- string accepted by the "ip route add " command
>+ """
>+ exec_cmd("ip route add %s dev %s" % (dest, self.name))
>+
>+ def del_route(self, dest):
>+ """remove specified route for this device
>+
>+ Args:
>+ dest -- string accepted by the "ip route del " command
>+ """
>+ exec_cmd("ip route del %s dev %s" % (dest, self.name))
>+
[...]
>+class MacvlanDevice(SoftDevice):
>+ _name_template = "t_macvlan"
>+
>+ _modulename = "macvlan"
Why do you need this here? "ip link add" will take care of the module
load.
Wasn't sure if it did... will sending my own netlink message to the
kernel also take care of the module? Might make sense to keep it here
until when we decide to do this, just so we don't forget about it...
>+
>+ def __init__(self, ifmanager, *args, **kwargs):
>+ super(MacvlanDevice, self).__init__(ifmanager, args, kwargs)
>+
>+ self._real_dev = kwargs["realdev"]
>+ self._mode = kwargs.get("mode", None)
>+ self._hwaddr = kwargs.get("hwaddr", None)
>+
>+ def create(self):
>+ create_cmd = "ip link add link {} {}".format(self._real_dev.name,
>+ self.name)
>+
>+ if self._hwaddr is not None:
>+ create_cmd += " address {}".format(self._hwaddr)
>+
>+ if self._mode is not None:
>+ create_cmd += " mode {}".format(self._mode)
>+
>+ create_cmd += " type macvlan"
>+
>+ exec_cmd(create_cmd)
>diff --git a/lnst/Devices/OvsBridgeDevice.py b/lnst/Devices/OvsBridgeDevice.py
>new file mode 100644
>index 0000000..a948e69
>--- /dev/null
>+++ b/lnst/Devices/OvsBridgeDevice.py
>@@ -0,0 +1,116 @@
>+"""
>+Defines the OvsBridgeDevice class.
>+
>+Copyright 2017 Red Hat, Inc.
>+Licensed under the GNU General Public License, version 2 as
>+published by the Free Software Foundation; see COPYING for details.
>+"""
>+
>+__author__ = """
>+olichtne(a)redhat.com (Ondrej Lichtner)
>+"""
>+
>+import logging
>+from lnst.Common.ExecCmd import exec_cmd
>+from lnst.Devices.Device import Device, DeviceError
>+from lnst.Devices.SoftDevice import SoftDevice
>+
>+class OvsBridgeDevice(SoftDevice):
>+ _name_template = "t_ovsbr"
>+
>+ _modulename = "openvswitch"
>+
>+ @classmethod
>+ def _type_init(cls):
>+ if cls._modulename and not cls._type_initialized:
>+ exec_cmd("modprobe %s %s" % (cls._modulename,
cls._moduleparams))
>+
>+ exec_cmd("mkdir -p /var/run/openvswitch/")
>+ exec_cmd("ovsdb-server --detach --pidfile "\
>+ "--remote=punix:/var/run/openvswitch/db.sock",
>+ die_on_err=False)
>+ exec_cmd("ovs-vswitchd --detach --pidfile", die_on_err=False)
>+
>+ cls._type_initialized = True
I think you should let the parent class to modprobe and set
_type_initialized. Just do the ovs initialization if "not
cls._type_initialized".
That was the original idea... but both the parent and the child class
check and set the _type_initialized value -> if the child calls the
parent _type_init method, then the _type_initialized attribute is set to
True. I guess it didn't occur to me that I could just nest the call to
the parent method after the _type_initialized check in the child...
"cls._modulename" is meaningless.
Will remove the modulename check.
>+
>+ def create(self):
>+ exec_cmd("ovs-vsctl add-br %s" % self.name)
>+
>+ def destroy(self):
>+ exec_cmd("ovs-vsctl del-br %s" % self.name)
>+
>+ def port_add(self, dev, **kwargs):
"slave_add"?
OvS is weird and doesn't work the same as any other device, because of
that the entire class kind of works on it's own as just a proxy to the
openvswitch utilities.
The port_{add, del} name because I wanted to keep name consistency with
those utilities.
AFAIK this is the one class we won't be able to reimplement to "set"
changes over Netlink, though we might look into APIs provided by the ovs
database/daemons (other than calling shellu utilities).
>+ options = ""
>+ for opt_name, opt_value in kwargs.items():
>+ options += " %s=%s" % (opt_name, opt_value)
>+
>+ exec_cmd("ovs-vsctl add-port %s %s%s" % (self.name, dev.name,
options))
>+
>+ def port_del(self, dev):
"slave_del"?
>+ if isinstance(dev, Device):
>+ exec_cmd("ovs-vsctl del-port %s %s" % (self.name, dev.name))
>+ elif isinstance(dev, str):
>+ exec_cmd("ovs-vsctl del-port %s %s" % (self.name, dev))
>+ else:
>+ raise DeviceError("Invalid port_del argument %s" % str(dev))
[...]
>+class VlanDevice(SoftDevice):
>+ _name_template = "t_vlan"
>+
You don't need this empty line.
ok.
>+ _module_name = "8021q"
You don't need this modprobe.
ok.