Merge pull request #2015 from anarkiwi/master

increase clib lint score, and fix flake in test_tcpdump_nextline as ping can exit before test terminates it.

Author: anarkiwi

clib/clib_mininet_test_unit.py

@@ -5,7 +5,6 @@
 
 import os
 import re
-import time
 
 from mininet.net import Mininet
 
@@ -16,6 +15,7 @@
 from tcpdump_helper import TcpdumpHelper
 from docker_host import MakeDockerHost
 
+
 class FaucetSimpleTest(mininet_test_base.FaucetTestBase):
     """Basic untagged VLAN test."""
 
@@ -49,18 +49,29 @@ def test_ping_all(self):
 class FaucetTcpdumpHelperTest(FaucetSimpleTest):
     """Test for TcpdumpHelper class"""
 
+
+    def _terminate_with_zero(self, tcpdump_helper):
+        term_returns = tcpdump_helper.terminate()
+        self.assertEqual(
+            0, term_returns, msg='terminate code not 0: %d' % term_returns)
+
+    def _terminate_with_nonzero(self, tcpdump_helper):
+        term_returns = tcpdump_helper.terminate()
+        self.assertNotEqual(
+            0, term_returns, msg='terminate code s 0: %d' % term_returns)
+
     def test_tcpdump_execute(self):
         """Check tcpdump filter monitors ping using execute"""
         self.ping_all_when_learned()
         from_host = self.net.hosts[0]
         to_host = self.net.hosts[1]
         tcpdump_filter = ('icmp')
         tcpdump_helper = TcpdumpHelper(to_host, tcpdump_filter, [
-                lambda: from_host.cmd('ping -c1 %s' % to_host.IP())])
+            lambda: from_host.cmd('ping -c1 %s' % to_host.IP())])
         tcpdump_txt = tcpdump_helper.execute()
         self.assertTrue(re.search(
             '%s: ICMP echo request' % to_host.IP(), tcpdump_txt))
-        self.assertEqual(tcpdump_helper.terminate(), 0, 'terminate result code')
+        self._terminate_with_zero(tcpdump_helper)
 
     def test_tcpdump_pcap(self):
         """Check tcpdump creates pcap output"""
@@ -69,11 +80,12 @@ def test_tcpdump_pcap(self):
         to_host = self.net.hosts[1]
         tcpdump_filter = ('icmp')
         pcap_file = os.path.join(self.tmpdir, 'out.pcap')
-        tcpdump_helper = TcpdumpHelper(to_host, tcpdump_filter, [
-                lambda: from_host.cmd('ping -c3 %s' % to_host.IP())],
-                pcap_out = pcap_file, packets = None)
-        tcpdump_txt = tcpdump_helper.execute()
-        self.assertEqual(tcpdump_helper.terminate(), 0, 'terminate result code')
+        tcpdump_helper = TcpdumpHelper(
+            to_host, tcpdump_filter,
+            [lambda: from_host.cmd('ping -c3 %s' % to_host.IP())],
+            pcap_out=pcap_file, packets=None)
+        tcpdump_helper.execute()
+        self._terminate_with_zero(tcpdump_helper)
         result = from_host.cmd('tcpdump -en -r %s' % pcap_file)
         self.assertEqual(result.count('ICMP echo reply'), 3, 'three icmp echo replies')
 
@@ -83,14 +95,15 @@ def test_tcpdump_noblock(self):
         from_host = self.net.hosts[0]
         to_host = self.net.hosts[1]
         tcpdump_filter = ('icmp')
-        tcpdump_helper = TcpdumpHelper(to_host, tcpdump_filter, [
-                lambda: from_host.cmd('ping -c10 %s' % to_host.IP())],
-                blocking = False, packets = None)
+        tcpdump_helper = TcpdumpHelper(
+            to_host, tcpdump_filter,
+            [lambda: from_host.cmd('ping -c10 %s' % to_host.IP())],
+            blocking=False, packets=None)
         count = 0
         while tcpdump_helper.next_line():
             count = count + 1
             self.assertTrue(count < 10, 'Too many ping results before noblock')
-        self.assertNotEqual(tcpdump_helper.terminate(), 0, 'terminate result code')
+        self._terminate_with_nonzero(tcpdump_helper)
 
     def test_tcpdump_nextline(self):
         """Check tcpdump filter monitors ping using next_line"""
@@ -99,7 +112,7 @@ def test_tcpdump_nextline(self):
         to_host = self.net.hosts[1]
         tcpdump_filter = ('icmp')
         tcpdump_helper = TcpdumpHelper(to_host, tcpdump_filter, [
-                lambda: from_host.cmd('ping -c2 %s' % to_host.IP())])
+            lambda: from_host.cmd('ping -c5 -i2 %s' % to_host.IP())])
 
         self.assertTrue(re.search('proto ICMP', tcpdump_helper.next_line()))
         next_line = tcpdump_helper.next_line()
@@ -110,18 +123,18 @@ def test_tcpdump_nextline(self):
         self.assertFalse(re.search('ICMP', tcpdump_helper.next_line()))
         while tcpdump_helper.next_line():
             pass
-        self.assertEqual(tcpdump_helper.terminate(), 0, 'terminate result code')
+        self._terminate_with_zero(tcpdump_helper)
 
 
 class FaucetDockerHostTest(FaucetSimpleTest):
 
-    N_UNTAGGED=2
-    N_EXTENDED=2
-    EXTENDED_CLS=MakeDockerHost('faucet/test-host')
+    N_UNTAGGED = 2
+    N_EXTENDED = 2
+    EXTENDED_CLS = MakeDockerHost('faucet/test-host')
 
     def test_containers(self):
         """Test containers to make sure they're actually docker."""
-        count=0
+        count = 0
         host_name = None
 
         for host in self.net.hosts:
@@ -132,10 +145,13 @@ def test_containers(self):
                 host.activate()
                 host.wait()
 
-        self.assertTrue(count == self.N_EXTENDED,
+        self.assertTrue(
+            count == self.N_EXTENDED,
             'Found %d containers, expected %d' % (count, self.N_EXTENDED))
 
-        self.assertTrue(os.path.exists(os.path.join(self.tmpdir, host_name, 'tmp')),
+        self.assertTrue(
+            os.path.exists(
+                os.path.join(self.tmpdir, host_name, 'tmp')),
             'container tmp dir missing')
 
         host_log = os.path.join(self.tmpdir, host_name, 'activate.log')

clib/docker_host.py

@@ -4,20 +4,20 @@
 import operator
 import os
 import pty
-import re
 import select
-import time
+from subprocess import PIPE, STDOUT
 
 # pylint: disable=import-error
 from mininet.log import error, debug
 from mininet.node import Host
-from subprocess import PIPE, STDOUT
 
 from mininet_test_util import DEVNULL
 
 def MakeDockerHost(image, prefix='mininet', startup_timeout_ms=None):
+
     class ImageHost(DockerHost):
-        def __init__( self, *args, **kwargs ):
+
+        def __init__(self, *args, **kwargs):
             host_name = args[0]
             kwargs['image'] = image
             assert kwargs['tmpdir'], 'tmpdir required for docker host'
@@ -29,7 +29,7 @@ def __init__( self, *args, **kwargs ):
                 env_val = os.environ['DOCKER_STARTUP_TIMEOUT_MS']
                 if env_val:
                     kwargs['startup_timeout_ms'] = int(env_val)
-            DockerHost.__init__(self, *args, **kwargs )
+            DockerHost.__init__(self, *args, **kwargs)
     return ImageHost
 
 
@@ -59,22 +59,24 @@ class DockerHost(Host):
     vol_maps = None
     prefix = None
     startup_timeout_ms = None
-
+    container = None
+    pollIn = None
+    active_log = None
 
     def __init__(self, name, image=None, tmpdir=None, prefix=None, env_vars=[],
-            vol_maps=[], startup_timeout_ms=STARTUP_TIMEOUT_MS, **kwargs ):
+                 vol_maps=[], startup_timeout_ms=STARTUP_TIMEOUT_MS, **kwargs):
         self.image = image
         self.tmpdir = tmpdir
         self.prefix = prefix
         self.env_vars = env_vars
         self.vol_maps = vol_maps
         self.startup_timeout_ms = startup_timeout_ms
-        Host.__init__( self, name, **kwargs )
+        Host.__init__(self, name, **kwargs)
 
-    def startShell( self ):
-        "Start a shell process for running commands"
+    def startShell(self):
+        """Start a shell process for running commands."""
         if self.shell:
-            error( "%s: shell is already running" )
+            error('shell is already running')
             return
 
         self.container = '%s-%s' % (self.prefix, self.name)
@@ -86,12 +88,13 @@ def startShell( self ):
         container_tmp_dir = os.path.join(os.path.abspath(self.tmpdir), 'tmp')
         tmp_volume = container_tmp_dir + ':/tmp'
 
-        base_cmd = [ "docker", "run", "-ti", "--privileged", "--entrypoint", "env",
-                     "--net=none", "-h", self.name, "--name", self.container ]
-        env_args = reduce(operator.add, ([ '--env', var ] for var in self.env_vars), [])
-        vol_args = reduce(operator.add, ([ '-v', var ] for var in self.vol_maps), [ '-v', tmp_volume ])
-        image_args = [ self.image, "TERM=dumb", "PS1=" + chr(127), "bash", "--norc",
-            "-is", "mininet:" + self.name ]
+        base_cmd = ["docker", "run", "-ti", "--privileged", "--entrypoint", "env",
+                    "--net=none", "-h", self.name, "--name", self.container]
+        env_args = reduce(operator.add, (['--env', var] for var in self.env_vars), [])
+        vol_args = reduce(operator.add, (['-v', var] for var in self.vol_maps), ['-v', tmp_volume])
+        image_args = [
+            self.image, "TERM=dumb", "PS1=" + chr(127), "bash", "--norc",
+            "-is", "mininet:" + self.name]
         cmd = base_cmd + env_args + vol_args + image_args
         self.master, self.slave = pty.openpty()
         debug('docker command "%s", fd %d, fd %d' % (' '.join(cmd), self.master, self.slave))
@@ -130,11 +133,11 @@ def startShell( self ):
     def kill(self, purge=False):
         debug('killing container %s.' % self.container)
         if purge:
-            kill_cmd = [ "docker", "rm", "-f", self.container ]
+            kill_cmd = ["docker", "rm", "-f", self.container]
         else:
-            kill_cmd = [ "docker", "kill", self.container ]
+            kill_cmd = ["docker", "kill", self.container]
         try:
-            kill_pipe = self._popen( kill_cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT)
+            kill_pipe = self._popen(kill_cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT)
             kill_pipe.stdout.readlines()
             kill_pipe.stdout.close()
         except:
@@ -144,8 +147,8 @@ def kill(self, purge=False):
 
     def inspect_pid(self):
         try:
-            pid_cmd = ["docker","inspect","--format={{ .State.Pid }}", self.container]
-            pid_pipe = self._popen( pid_cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT)
+            pid_cmd = ["docker", "inspect", "--format={{ .State.Pid }}", self.container]
+            pid_pipe = self._popen(pid_cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT)
             ps_out = pid_pipe.stdout.readlines()
             pid_pipe.stdout.close()
             return int(ps_out[0])
@@ -200,37 +203,39 @@ def wait(self):
             self.active_pipe.returncode = self.active_pipe.wait()
             self.terminate()
             return self.active_pipe_returncode
-        except Exception as e:
-            error('Exception waiting for %s: %s' % (self.container, e))
+        except Exception as err:
+            error('Exception waiting for %s: %s' % (self.container, err))
             self.terminate()
             raise
 
-    def read( self, maxbytes=1024 ):
+    def read(self, maxbytes=1024):
         poll_results = self.pollIn.poll(self.startup_timeout_ms)
         data_ready = poll_results and (poll_results[0][1] & select.POLLIN)
-        assert data_ready, ('Timeout waiting for read data on %d after %ds' %
-            (self.stdout.fileno(), self.startup_timeout_ms / 1000))
+        assert data_ready, (
+            'Timeout waiting for read data on %d after %ds' %
+            (self.stdout.fileno(), self.startup_timeout_ms / 1e3))
         return Host.read(self, maxbytes)
 
     def terminate(self):
         """Override Mininet terminate() to partially avoid pty leak."""
-        debug('Terminating container %s, shell %s, pipe %s' % (self.container, self.shell, self.active_pipe))
+        debug('Terminating container %s, shell %s, pipe %s' % (
+            self.container, self.shell, self.active_pipe))
         if self.slave:
             os.close(self.slave)
             self.slave = None
         if self.shell is not None:
             self.stdin.close()
             self.stdin = None
             self.master = None
-            if self.shell.returncode == None:
+            if self.shell.returncode is None:
                 self.shell.kill()
                 self.shell.poll()
             self.kill()
             self.shell = None
         if self.active_pipe:
             if self.active_pipe.stdout:
                 self.active_pipe.stdout.close()
-            if self.active_pipe.returncode == None:
+            if self.active_pipe.returncode is None:
                 self.active_pipe.kill()
                 self.active_pipe.poll()
             self.active_pipe_returncode = self.active_pipe.returncode
@@ -241,15 +246,15 @@ def terminate(self):
         self.cleanup() # pylint: disable=no-member
         return self.active_pipe_returncode
 
-    def popen( self, *args, **kwargs ):
+    def popen(self, *args, **kwargs):
         """Return a Popen() object in node's namespace
            args: Popen() args, single list, or string
            kwargs: Popen() keyword args"""
         # -t is necessary to prevent docker from buffering output. It might cause
         # problems with some commands like shells that then assume they can output
         # all sorts of crazy control characters b/c it's a terminal.
-        mncmd = [ 'docker', 'exec', '--env', 'TERM=dumb', '-t', self.container ]
-        pipe = Host.popen( self, mncmd=mncmd, *args, **kwargs )
+        mncmd = ['docker', 'exec', '--env', 'TERM=dumb', '-t', self.container]
+        pipe = Host.popen(self, mncmd=mncmd, *args, **kwargs)
         if pipe:
             debug('docker pid %d: %s %s %s' % (pipe.pid, mncmd, args, kwargs))
         return pipe
@@ -259,7 +264,7 @@ def _popen(self, cmd, **params):
         # a normal interactive terminal would. So, put it in a separate process group
         # so it doesn't receive stray SIGINTs, rather relying on the message sent
         # from the owning process through the pty.
-        if not 'preexec_fn' in params:
+        if 'preexec_fn' not in params:
             params['preexec_fn'] = os.setpgrp
         pipe = super(DockerHost, self)._popen(cmd, **params)
         if pipe:

clib/mininet_test_topo.py

@@ -81,7 +81,7 @@ def terminate(self):
         if self.shell is not None:
             os.close(self.slave)
             self.stdin.close()
-            if self.shell.returncode == None:
+            if self.shell.returncode is None:
                 self.shell.kill()
                 self.shell.poll()
         self.cleanup() # pylint: disable=no-member
@@ -116,11 +116,11 @@ def start(self, controllers):
                          for intf in switch_intfs)
         # Command to create controller entries
         clist = [(self.name + c.name, '%s:%s:%d' %
-                 (c.protocol, c.IP(), c.port))
+                  (c.protocol, c.IP(), c.port))
                  for c in controllers]
         if self.listenPort:
             clist.append((self.name + '-listen',
-                           'ptcp:%s' % self.listenPort))
+                          'ptcp:%s' % self.listenPort))
         ccmd = '-- --id=@%s create Controller target=\\"%s\\"'
         if self.reconnectms:
             ccmd += ' max_backoff=%d' % self.reconnectms
@@ -138,12 +138,12 @@ def start(self, controllers):
                    ' -- add-br %s' % self +
                    ' -- set bridge %s controller=[%s]' % (self, cids) +
                    self.bridgeOpts() +
-                   intfs )
+                   intfs)
         # switch interfaces on mininet host, must have no IP config.
         for intf in switch_intfs:
             for ipv in (4, 6):
                 self.cmd('ip -%u addr flush dev %s' % (ipv, intf))
-            assert '' == self.cmd('echo 1 > /proc/sys/net/ipv6/conf/%s/disable_ipv6' % intf)
+            assert self.cmd('echo 1 > /proc/sys/net/ipv6/conf/%s/disable_ipv6' % intf) == ''
         # If necessary, restore TC config overwritten by OVS
         if not self.batch:
             for intf in self.intfList():