----- Original Message -----
From: "Ondrej Lichtner" <olichtne(a)redhat.com>
To: "Christos Sfakianakis" <csfakian(a)redhat.com>
Cc: lnst-developers(a)lists.fedorahosted.org
Sent: Wednesday, February 27, 2019 4:51:52 PM
Subject: Re: [PATCH-next 1/2] lnst.Devices.Device: handle coalesce
On Wed, Feb 27, 2019 at 10:40:10AM -0500, Christos Sfakianakis wrote:
>
>
> ----- Original Message -----
> > From: "Ondrej Lichtner" <olichtne(a)redhat.com>
> > To: csfakian(a)redhat.com
> > Cc: lnst-developers(a)lists.fedorahosted.org
> > Sent: Wednesday, February 27, 2019 12:39:08 PM
> > Subject: Re: [PATCH-next 1/2] lnst.Devices.Device: handle coalesce
> >
> > On Tue, Feb 26, 2019 at 04:34:43PM +0100, csfakian(a)redhat.com wrote:
> > > From: Christos Sfakianakis <csfakian(a)redhat.com>
> > >
> > > Add methods for handling coalesce settings. Raise errors
> > > when these settings cannot be read or modified correctly.
> > >
> > > Signed-off-by: Christos Sfakianakis <csfakian(a)redhat.com>
> > > ---
> > > lnst/Devices/Device.py | 47 ++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 47 insertions(+)
> > >
> > > diff --git a/lnst/Devices/Device.py b/lnst/Devices/Device.py
> > > index 20cddcf..6a04d1b 100644
> > > --- a/lnst/Devices/Device.py
> > > +++ b/lnst/Devices/Device.py
> > > @@ -17,10 +17,12 @@ import pyroute2
> > > import logging
> > > import pprint
> > > from abc import ABCMeta
> > > +from itertools import product
> > > from pyroute2.netlink.rtnl import ifinfmsg
> > > from lnst.Common.Logs import log_exc_traceback
> > > from lnst.Common.NetUtils import normalize_hwaddr
> > > from lnst.Common.ExecCmd import exec_cmd
> > > +from lnst.Common.LnstError import LnstError
> > > from lnst.Common.DeviceError import DeviceError, DeviceDeleted,
> > > DeviceDisabled
> > > from lnst.Common.DeviceError import DeviceConfigError,
> > > DeviceConfigValueError
> > > from lnst.Common.IpAddress import ipaddress
> > > @@ -58,6 +60,7 @@ class Device(object):
> > > self._deleted = False
> > >
> > > self._ip_addrs = []
> > > + self._coalesce_settings = ['', '']
> > >
> > > self._nl_update = {}
> > > self._bulk_enabled = False
> > > @@ -603,6 +606,50 @@ class Device(object):
> > > """disable automatic negotiation of speed for this
device"""
> > > exec_cmd("ethtool -s %s autoneg off" % self.name)
> > >
> > > + def _read_coalesce(self):
> > > + try:
> > > + res = exec_cmd("ethtool -c %s" % self.name)
> > > + except:
> > > + raise DeviceError("Could not fetch coalesce settings of
"
> > > + "%s." % self.name)
> > > + res = re.findall('\wX: (o[n|f]*)', res[0])
> > ^^^^^^^
> > i think this can just be (on|off)
> > > + if not tuple(res) in product(['off', 'on'],
repeat=2):
> > > + raise LnstError("Invalid values for coalesce settings of
"
> > > + "{} : {}".format(self.name,
res))
> > ^^^
> > use DeviceError for exceptions from the Device class
> >
> > So if I understand this correctly, the condition checks if all the
> > strings found by the re.findall are "on" or "off", and that
there's
> > exactly two of them. But the regex already ensures that it's only
"on"
> > or "off" (with my modification at least). And you can ensure the
> > "exactly two values" by modifying the regex a bit more:
> >
> > "Adaptive RX: (on|off) TX: (on|off)"
> >
> > then I'd just use re.search and return the result.groups().
> >
> > In case the regex search fails, then throw the exception.
> >
> Agree, will apply these changes.
>
> > > + return res
> > > +
> > > + def _store_coalesce(self):
> > > + coal = self._read_coalesce()
> > > + self._coalesce_settings = coal
> > > +
> > > + def _write_coalesce(self, rx_val, tx_val):
> > > + if self._coalesce_settings == [rx_val, tx_val]:
> >
> > Depending on if you store a tuple or a list into the _coalesce_settings,
> > this comparison might always fail, tuple never equals a list... just a
> > note in case you rewrite the read method with match.groups() which
> > returns a tuple...
> >
> > > + return
> > > + try:
> > > + exec_cmd("ethtool -C %s adaptive-rx %s adaptive-tx
%s" %
> > > + (self.name, rx_val, tx_val))
> > > + except:
> > > + raise DeviceConfigError("Not allowed to modify coalesce
"
> > > + "settings for %s." %
self.name)
> > > + if self._read_coalesce != [rx_val, tx_val]:
> > ^^^
> > this is just a reference to a method, you're missing ()
> > to actually call it so this condition is always True
> > > + raise LnstError("Could not apply coalesce settings for
"
> > > + "%s effectively." % self.name)
> > DeviceError instead of LnstError, DeviceConfigError
> > is also appropriate here I think
> >
> > > +
> > > + def _disable_coalesce(self):
> > > + self._store_coalesce()
> > > + self._write_coalesce('off', 'off')
> > > +
> > > + def _restore_coalesce(self):
> > > + rx_val = self._coalesce_settings[0]
> > > + tx_val = self._coalesce_settings[1]
> > > + self._write_coalesce(rx_val, tx_val)
> > > +
> > > + def disable_coalesce(self):
> > > + self._disable_coalesce()
> >
> > Does it make sense to also have an enable_coalesce method for
> > completeness?
> >
> Adding an enable() method. I will assume the initial values need to be
> stored only once prior to the first change (and then restored in the end
> of the test), so modifying the disable method slightly as well.
> -Christos
Yeah I guess I didn't properly notice... I'd expect store and restore
methods to be independent of enable/disable. I'm not even sure if I'd
store the state inside the Device class, in case someone wants to
change this multiple times to different values and calling store/restore
during a single recipe... might be better to directly expose
read/write_coalesce methods as public (but at that point I think I'd
want to rename them to get/set instead of read/write) so that the tester
can store multiple "states" and set whatever, whenever during a recipe.
And then the enable/disable would just be shortcuts for on/off.
Ok, but for the config phase, it would be good to have the initial values stored
in the Device class in order to restore the original settings in the deconfig phase.
So my proposal is to keep _store method there and call it only once, irrespective of
what the user/tester tests.
So in a generic scenario,
config phase:
- Store coal settings if you need to do any kind of coal tuning
- Proceed to set/get/enable/dsiable coal as you wish
deconfig phase:
- Always call restore function, which does nothing in the worst case.
We can then expose the functions you mentioned without issues.
What do you think? This might mean that the recipes need to use the
API
differently but that's fine, the API should come first...
+1
>
> -Ondrej
>
> > > > +
> > > > + def restore_coalesce(self):
> > > > + self._restore_coalesce()
> > > > +
> > > > #TODO implement proper Route objects
> > > > #consider the same as with tc?
> > > > # def route_add(self, dest):
> > > > --
> > > > 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://getfedora.org/code-of-conduct.html
> > > > List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> > > > List Archives:
> > > >
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...
> > > _______________________________________________
> > > 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://getfedora.org/code-of-conduct.html
> > > List Guidelines:
https://fedoraproject.org/wiki/Mailing_list_guidelines
> > > List Archives:
> > >
https://lists.fedorahosted.org/archives/list/lnst-developers@lists.fedora...
> > >
>