On Tue, May 16, 2017 at 10:00:18AM +0200, Jiri Pirko wrote:
Tue, May 16, 2017 at 09:38:24AM CEST, olichtne(a)redhat.com wrote:
>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...
MasterDevice is good.
>
>>
>>
>> >+
>> >+ 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?
Let's do the complete class Device api review as we discussed offline.
>
>>
>>
>> >+ """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.
Sounds good. I would rename master_set to __master_set so the user does
not use it directly
Sorry, my mistake, the method should be renamed to just "master" and the
@property decorator takes care of the dual 'master' method definition.
So the user won't have the ability to use the method directly.
>
>>
>> >+ """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...
Yep, rtnetlink.c will take care of the module load according to the
device kind
>
>>
>> >+
>> >+ 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...
Yep :)
>
>>"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.
Hmm, I think that we should have an alias then. User should be able to
work with all master devices the same way.
Well... I actually think OvSBridgeDevice shouldn't be a MasterDevice...
the problem with ovs is that if you port_add an Ethernet device. Then
the master of that device is not the OvSBridge... it's a dummy
ovs_system net_device that is exported by the ovs module AFAIK... this
is the case for all ports for all ovs bridges (they share the same
master). So the common 'slaves' getter won't work for them either.
Yes.. this does mean that I'm missing getters in this class... Will add
those where appropriate.
Adding a slave_{add, del} alias is fine with me, though I don't really
care because as I've explained, I view OvS as weird and completely
separate from everything else and am expecting that it will require
workarounds and other code exceptions....
>
> >
> >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.