Re: [lnst] ensure correct team cleanup

[Jan Tluka]

Hi, I'm seeing some oddness in this patch. See inline ...

Thu, Aug 16, 2012 at 01:43:48PM CEST, jirka(a)fedoraproject.org wrote:
> commit 46e03e6884b0817a962b414beff25c1eb2e1af5f
> Author: Jiri Pirko <jiri(a)resnulli.us&gt;
> Date:   Thu Aug 16 13:35:01 2012 +0200
>
>     ensure correct team cleanup
>     
>     Killall teamd eventualy removes all team devices from the system.
>     However it may happen after lnst does netdev rescan and therefore lnst
>     may still see it. Resolve this by waiting for all kmod users to
>     disappear and, just to be save, remove all team kmods.
>     
>     Signed-off-by: Jiri Pirko <jiri(a)resnulli.us&gt;
>
>  Common/Utils.py              |   18 ++++++++++++++++++
>  NetConfig/NetConfigDevice.py |   20 ++++++++------------
>  2 files changed, 26 insertions(+), 12 deletions(-)
> ---
> diff --git a/Common/Utils.py b/Common/Utils.py
> index f6ba201..5f3f251 100644
> --- a/Common/Utils.py
> +++ b/Common/Utils.py
> @@ -11,6 +11,7 @@ jzupka(a)redhat.com (Jiri Zupka)
>  """
>  import logging
>  import time
> +import re
>  
>  def die_when_parent_die():
>      try:
> @@ -52,3 +53,20 @@ def wait_for(func, timeout, first=0.0, step=1.0, text=None):
>  
>      logging.debug("Timeout elapsed")
>      return None
> +
> +def kmod_in_use(modulename, tries = 1):
> +    tries -= 1
> +    ret = False
> +    mod_file = "/proc/modules"
> +    handle = open(mod_file, "r")
> +    for line in handle:
> +        match = re.match(r'^(\S+)\s\d+\s(\d+).*$', line)
> +        if not match or not match.groups()[0] in re.split('\s+', modulename):
> +            continue
> +        if int(match.groups()[1]) != 0:
> +            ret = True
> +        break
> +    handle.close()
> +    if (ret and tries):
> +        return kmod_in_use(modulename, tries)
> +    return ret
> diff --git a/NetConfig/NetConfigDevice.py b/NetConfig/NetConfigDevice.py
> index 1e8a9aa..534e914 100644
> --- a/NetConfig/NetConfigDevice.py
> +++ b/NetConfig/NetConfigDevice.py
> @@ -16,6 +16,7 @@ import re
>  import sys
>  from Common.ExecCmd import exec_cmd
>  from NetConfigCommon import get_slaves, get_option
> +from Common.Utils import kmod_in_use
>  
>  class NetConfigDeviceGeneric:
>      '''
> @@ -23,6 +24,7 @@ class NetConfigDeviceGeneric:
>      extend this one.
>      '''
>      _modulename = ""
> +    _moduleload = True
>      _moduleparams = ""
>      _cleanupcmd = ""
>  
> @@ -58,24 +60,16 @@ class NetConfigDeviceGeneric:
>  
>      @classmethod
>      def type_init(self):
> -        if self._modulename:
> +        if self._modulename and self._moduleload:
>              exec_cmd("modprobe %s %s" % (self._modulename,
> self._moduleparams))
>  
>      @classmethod
> -    def _module_check(self):
> -        if self._modulename:
> -            output = exec_cmd("modinfo -F filename %s" % self._modulename,
> die_on_err=False)[0]
> -            for line in output.split("\n"):
> -                if re.match(r'^.*\/%s\.ko$' % self._modulename, line):
> -                    return True
> -        False
> -
> -    @classmethod
>      def type_cleanup(self):
> -        if self._modulename and self._module_check():
> -            exec_cmd("modprobe -r %s" % self._modulename, die_on_err=False)
>          if self._cleanupcmd:
>              exec_cmd(self._cleanupcmd, die_on_err=False)
> +        if self._modulename:
> +            kmod_in_use(self._modulename, 300) 
... kmod_in_use returns True or False, so I think we're missing some
condition check, like:
              if not kmod_in_use(...):
                  remove_module

> +            exec_cmd("modprobe -r %s" % self._modulename,
> die_on_err=False)
>  
>  class NetConfigDeviceEth(NetConfigDeviceGeneric):
>      def configure(self):
> @@ -217,6 +211,8 @@ class NetConfigDeviceVlan(NetConfigDeviceGeneric):
>  
>  class NetConfigDeviceTeam(NetConfigDeviceGeneric):
>      _pidfile = None
> +    _modulename = "team_mode_roundrobin team_mode_activebackup team_mode_broadcast
> team_mode_loadbalance team"
> +    _moduleload = False
>      _cleanupcmd = "killall -q teamd"
>  
>      def configure(self):
> _______________________________________________
> LNST-developers mailing list
> LNST-developers(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers