Tue, Jul 23, 2019 at 11:21:59AM CEST, csfakian(a)redhat.com wrote:
----- Original Message -----
> From: "Jan Tluka" <jtluka(a)redhat.com>
> To: "Christos Sfakianakis" <csfakian(a)redhat.com>
> Cc: "Ondrej Lichtner" <olichtne(a)redhat.com>,
lnst-developers(a)lists.fedorahosted.org
> Sent: Tuesday, July 23, 2019 10:56:22 AM
> Subject: Re: [PATCH-next 7/8] lnst.Devices.OvsBridgeDevice: add config getters
>
> Mon, Jul 22, 2019 at 04:45:12PM CEST, csfakian(a)redhat.com wrote:
> >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>
> >> > + @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.
>
> When you say non-relevant I assume the code would work even without
> filtering. Is this really necessary or just some sort of optimization?
>
'Non-relevant' in this case means that you do not control it or set it explicitly
in the
recipe. Cookies, bytes, packets, idle_age or duration do not belong to the explicitly
configured
part of the device and are rather informative fields that accompany the 'flows'
information
of it.
Also, most of these fields contain data that is non-static, meaning these values may
change
over time, without the tester's control. I think such data should be filtered out. The
original
intention was to use these properties to present a description of the configuration of an
OvsBridgeDevice
to the tester while actually running the recipe (this could be useful in a number of ways
and had already been supported anyways, but we were missing these properties).
> And I wonder if we find out that we need some of the 'non-relevant' data
> some time in the future.
>
If we found out we do need some of these 5 fields, the
fix should be easy. But IMO we will not be needing that info:)
> If the code works without filtering I think it should be removed.
>
If you still preserve this opinion, I will proceed to remove these
filters, please confirm.
No, I don't. With your explanation it completely makes sense.
Thanks,
Jan