Hi,
----- Original Message -----
From: "Ondrej Lichtner" <olichtne(a)redhat.com>
To: csfakian(a)redhat.com
Cc: lnst-developers(a)lists.fedorahosted.org
Sent: Monday, July 22, 2019 3:48:05 PM
Subject: Re: [PATCH-next 7/8] lnst.Devices.OvsBridgeDevice: add config getters
On Fri, Jul 19, 2019 at 01:36:50PM +0200, csfakian(a)redhat.com wrote:
> From: Christos Sfakianakis <csfakian(a)redhat.com>
>
> Add getter properties for ports, internal ports, tunnels,
> flows, bonds. Return values are based on the output of:
> - for ports/internal/ports/tunnels: 'ovs-vsctl show',
> 'ovs-ofctl dump-ports-desc <dev>'
> - for bonds: 'ovs-appctl bond/list'
> - for flows: 'ovs-ofctl dump-flows <dev>'
>
> Signed-off-by: Christos Sfakianakis <csfakian(a)redhat.com>
> ---
> lnst/Devices/OvsBridgeDevice.py | 111 ++++++++++++++++++++++++++++++++
> 1 file changed, 111 insertions(+)
>
> diff --git a/lnst/Devices/OvsBridgeDevice.py
> b/lnst/Devices/OvsBridgeDevice.py
> index 643fa7d..d9bba5a 100644
> --- a/lnst/Devices/OvsBridgeDevice.py
> +++ b/lnst/Devices/OvsBridgeDevice.py
> @@ -10,6 +10,8 @@ __author__ = """
> olichtne(a)redhat.com (Ondrej Lichtner)
> """
>
> +import re
> +import pprint
> from lnst.Common.Utils import check_process_running
> from lnst.Common.ExecCmd import exec_cmd
> from lnst.Common.DeviceError import DeviceError
> @@ -22,6 +24,8 @@ class OvsBridgeDevice(SoftDevice):
> def __init__(self, ifmanager, *args, **kwargs):
> super(OvsBridgeDevice, self).__init__(ifmanager)
> self._type_init()
> + self._numbered_ports = {}
> + self._port_lines = []
i assume these are supposed to work as a sort of cache for the data?
Does it make sense to only load them up once with the get_port_info()?
Could the bridge configuration change during recipe execution and we'd
end up getting out of date info from this?
I will need to ensure these are always up to date (as you said) to avoid
outdated info being sent back in case a custom test case is used.
>
> @classmethod
> def _type_init(cls):
> @@ -112,3 +116,110 @@ class OvsBridgeDevice(SoftDevice):
>
> def flows_del(self, entry):
> exec_cmd("ovs-ofctl del-flows %s" % (self.name))
> +
> + @property
> + def ports(self):
> + if not self._numbered_ports:
> + self._get_port_info()
> + ports = {}
> +
> + for line in self._port_lines:
> + if not re.search('type=', line):
> + self._index(line, ports)
> +
> + return ports
> +
> + @property
> + def internal_ports(self):
> + if not self._numbered_ports:
> + self._get_port_info()
> + int_ports = {}
> +
> + for line in self._port_lines:
> + if re.search('type=internal', line):
> + line = re.sub(r",?\stype=internal", "", line)
> + self._index(line, int_ports)
> +
> + return int_ports
> +
> + @property
> + def tunnels(self):
> + if not self._numbered_ports:
> + self._get_port_info()
> + tunnels = {}
> +
> + for line in self._port_lines:
> + if re.search('type=(?!internal)', line):
> + self._index(line, tunnels)
> +
> + return tunnels
> +
> + @property
> + def bonds(self):
> + bonds = {}
> + bond_list = []
> + out = exec_cmd("ovs-appctl bond/list", log_outputs=False)[0]
> +
> + for line in out.split('\n'):
> + if line:
> + bond_list.append(line.split('\t'))
> +
> + for bond in bond_list[1:]:
> + bonds[bond[0]] = {'type' : bond[1], 'slaves' :
bond[3]}
> +
> + return bonds
> +
> + @property
> + def flows(self):
> + flows = []
> + ignore_exprs = [r"cookie", r"duration",
r"n_packets",
> + r"n_bytes", r"idle_age"]
> + out = exec_cmd("ovs-ofctl dump-flows %s" % self.name,
> + log_outputs=False)[0]
> +
> + for line in out.split('\n'):
> + if line:
> + flows.append(line.split(', '))
> +
> + for flow in flows[1:]:
> + for entry in list(flow):
> + for expr in ignore_exprs:
> + if re.search(expr, entry):
> + del flow[flow.index(entry)]
> + break
> +
> + return pprint.pformat(flows[1:])
The previous properties return a data structure (a dict), but this one
returns a formatted string, is that intentional? If so would it make
more sense to name the method with something more descriptive e.g.
flows_desc or flows_str. Or if not then returning a data structure would
be better?
This was intentional, I tried to filter out non-relevant info from the output,
while still storing it line by line. I will name the method differently.
> +
> + def _get_port_info(self):
> + dumped_ports = exec_cmd("ovs-ofctl dump-ports-desc %s" %
> + self.name, log_outputs=False)[0]
> +
> + for match in re.finditer(r'(\w+)\((\w*)\)',
> + dumped_ports):
> + self._numbered_ports[match.groups()[1]] = match.groups()[0]
> +
> + ovs_show = exec_cmd("ovs-vsctl show",
> + log_outputs=False)[0]
> + regex = r'(Port[\w\W]*?)(?=Port|ovs_version)'
> +
> + for match in re.finditer(regex, ovs_show):
> + line = match.groups()[0].replace('\n', ' ')
> + line = self._port_format(line)
> + self._port_lines.append(line)
wrt. the first comment, yeah I think saving this method output data into
instance attributes is a bit weird, considering that almost immediately
after they get used they can be considered "outdated", in such a case it
might make more sense to call this method every time you call one of the
"ports" properties and return the output values directly instead of
going through instance attributes.
Sure, I will change the code accordingly.
> +
> + def _port_format(self, line):
> + res = re.sub(r":", "", line)
> + res = re.sub(r"(\S[^,])\s(\S)", "\\1=\\2", res)
> + res = re.sub(r"\s{2,}(?=\S)", ", ", res)
> + res = re.sub(r"\s*$", "", res)
> +
> + return res
> +
> + def _index(self, line, result):
I'm not sure what this method is doing exactly, could a more descriptive
name be used?
This creates a pair held in 'result', where the key is the port number
and the value is 'line' (which includes the port's name).
Perhaps a different name (again), like 'line_to_port_number', would be
better.
Thanks,
Christos
> > + name = re.match(r"Port=\"(\w+)\"",
line).groups()[0]
> > +
> > + try:
> > + number = self._numbered_ports[name]
> > + result[number] = line
> > + except KeyError:
> > + pass
> > --
> > 2.17.1
> > _______________________________________________
> > LNST-developers mailing list -- lnst-developers(a)lists.fedorahosted.org
> > To unsubscribe send an email to
> > lnst-developers-leave(a)lists.fedorahosted.org
> > Fedora Code of Conduct:
> >
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives:
> >
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...
>