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?
+
+ 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 :)
+
+ 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.
[...]
+
+ @property
+ def ifi_type(self):
What is "ifi"?
+ """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.
+
+ @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.
+ 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.
+ """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
+ """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.
+
+ 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". "cls._modulename" is meaningless.
+
+ 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"?
+ 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.
+ _module_name = "8021q"
You don't need this modprobe.