Various fixes to functional tests #783

intelfx · 2017-08-01T13:49:30Z

(Mostly) fixes #759.

Remaining issues are:

rare sporadic failures (empty replies) in functional tests
rare sporadic failures in stress tests when all three components are ran on the same host

vankoven · 2017-08-01T18:38:19Z

tempesta_fw/t/functional/helpers/control.py

        self.node = remote.tempesta
        self.workdir = self.node.workdir
-        self.config_name = 'tempesta_fw.conf'
+        self.config_name = tempfile.mktemp(dir = '/tmp', prefix = 'tempesta_fw.conf.')


I believe that location of config file should be configured in tests_config.ini with defaults /tmp/tempesta_fw.conf. It is good to have the only place to control filenames for generated files.

vankoven · 2017-08-01T18:44:34Z

tempesta_fw/t/functional/helpers/control.py

@@ -30,6 +31,7 @@ def __init__(self, binary, uri=''):
        in `form_command()` function. This would allow to update options until
        client will be started. See `Wrk` class for example
        """
+        remote.connect_client()


I see that you have updated run_tests.sh to support running only specific tests. There is no need to create new connections by calling remote.connect_*() in every test. run_tests.sh seems to be a good place to setup connections to remote nodes.

This also appy Nginx and Tempesta classes below.

I assumed you still want to launch testcase modules as stand-alone programs (OTOH, I tried that and failed, there are problems with relative imports).

However — it seems like modules are re-imported afresh in each testcase, so that won't work.

I'm not sure, that running test cases as standalone programs is required. Running only a few test cases instead of entire test framework is desired. Actually old variant has at lest one serious drawback: you just can't interrupt test cases gracefully, so Tempesta, backend, and client may be still running after interruption of the test cases. You have added a possibility to run only specific test cases via run_tests.py. I really like that.

And concerning re-importing all modules. As i said i'm not good in python, so there can be dirty python code and wrong python libraries usage. On some reason i don't have issues with paramiko you have faced, and i see only one call of remote.create_node() per node on current master. If you say the change is needed, then ok, let's have connect() and disconnect() present in TestCase classes. Is it possible to move connect() and disconnect() to unittest.Testcase.setUpClass() and tearDownClass()?

vankoven · 2017-08-01T19:01:22Z

tempesta_fw/t/functional/helpers/control.py

@@ -330,7 +333,7 @@ def __init__(self, listen_port, workers=1):
        self.requests = 0

    def get_name(self):
-        return ':'.join([self.node.host, str(self.config.port)])
+        return ':'.join([self.ip, str(self.config.port)])


Good catch!

vankoven · 2017-08-01T19:17:09Z

tempesta_fw/t/functional/helpers/remote.py

+    def copy_file(self, filename, content):
+        # workdir will be ignored if an absolute filename is passed
+        filename = os.path.join(self.workdir, filename)
+        self.mkdir(os.path.dirname(filename))


Better to create directory it it doesn't equal to self.workdir. Or the self.workdir will be created too often, e.g. some tests creates 32 instances of nginx and each have its own config file. This slows down tests a lot.

vankoven · 2017-08-01T19:18:10Z

tempesta_fw/t/functional/helpers/remote.py

-                err_msg=''):
-        tf_cfg.dbg(4, "\tRun command '%s' on host %s" % (cmd, self.host))
+                err_msg='', env={}):
+        tf_cfg.dbg(4, "\tRun command '%s' on host %s with environment %s" % (cmd, self.host, env))


Too long line

vankoven · 2017-08-01T19:29:49Z

tempesta_fw/t/functional/helpers/tf_cfg.py

+
+# the default value of fs.nr_open
+nofile = 1048576
+resource.setrlimit(resource.RLIMIT_NOFILE, (nofile, nofile))


why put here but not in run_tests.sh?

As per above, I assumed you wanted to preserve the possibility of launching testcases as standalone programs.

vankoven · 2017-08-01T19:54:02Z

tempesta_fw/t/functional/run_tests.py

+if not remainder:
+    tests = loader.discover('.')
+else:
+    tests = loader.loadTestsFromNames(remainder)


Really good change: much easier to run only specified tests and have the same control on verbose and interruption by ^C. And much better than running pyhton2 -m unittest <testname>. Please update tests readme with the new instructions on running only specified tests.

vankoven · 2017-08-01T19:56:19Z

tempesta_fw/t/functional/testers/functional.py

@@ -64,6 +62,7 @@ def tearDown(self):
            self.tempesta.stop()
        if self.tester:
            self.tester.close_all()
+        remote.disconnect()


I have proposed to move connect() to run_tests.sh, so disconnect() won't be needed here.

vankoven · 2017-08-01T19:59:27Z

tempesta_fw/t/functional/tests_config.ini.sample

@@ -54,29 +62,29 @@ wrk = wrk
 #
 # ex.: siege = /usr/bin/siege (default siege)
 #
-siege = /usr/bin/siege
+siege = siege


I have experienced a lot of troubles with siege. Sometimes it performs god, but in other cases it is too buggy. It is obsoleted and can be removed without any problems

vankoven · 2017-08-01T20:02:45Z

tempesta_fw/t/functional/run_tests.py

@@ -79,7 +79,11 @@ def usage():

 #run tests
 loader = unittest.TestLoader()
-tests = loader.discover('.')
+if not remainder:
+    tests = loader.discover('.')


Not a problem of this patchset, just remark on current problems. discover() sorts tests in very strange way, e.g. selftests for the test suit is done somewhere in the middle of test cases.

…ions

cf. https://stackoverflow.com/questions/443387/why-does-paramiko-hang-if-you-use-it-while-loading-a-module, paramiko/paramiko#104

principle of least astonishment

vankoven

Looks good for me, but a few more errors in functional tests must be fixed in this patch set or in a new one. See #789 (comment) , #782 (comment)

vankoven · 2017-08-07T07:51:35Z

tempesta_fw/t/functional/sessions/test_sticky_sess_stress.py

+        # wait for responses to last requests in each connection before closing it
+        # and does not account for those requests.
+        # So, [0; concurrent_connections] requests will be missed by the client.
+        exp_max = self.wrk.requests + self.wrk.connections


Positive allowance is common across all stress test cases using wrk, so it should be documented in testers.stress whis is a base class for all such tests.

vankoven · 2017-08-07T07:53:09Z

tempesta_fw/t/functional/sessions/test_sticky_sess_stress.py

-        exp_min = self.wrk.requests -  expected_err - 1
-        exp_max = self.wrk.requests
+        # Negative allowance: this means some requests are missed by the server.
+        # This happens because some (at most one) requests in each connection


This happens because some (at most one)

At least one in wrk thread, at most one in each connection

vankoven · 2017-08-07T07:54:00Z

tempesta_fw/t/functional/sessions/test_sticky_sess_stress.py

+        # This happens because some (at most one) requests in each connection
+        # are sent without a session cookie and replied 302 by Tempesta without
+        # any forwarding, which is considered a "success" by wrk.
+        # So, [0; concurrent_connections] requests will be missed by the backend.


[1; concurrent_connections]

vankoven · 2017-08-07T07:55:26Z

tempesta_fw/t/functional/sessions/test_sticky_sess_stress.py

-        expected_err = int(tf_cfg.cfg.get('General', 'concurrent_connections'))
-        exp_min = self.wrk.requests -  expected_err - 1
-        exp_max = self.wrk.requests
+        # Negative allowance: this means some requests are missed by the server.


Not "missed by the server", but "are not forwarded to server"

vankoven

Just forgot to choose Approve tick in the previous review

…ility)

…the test

Each thread in wrk has its own Lua context and hence get its own session cookie, failing the test. Found by @ikoveshnikov

…discrepancies

… run_tests.py

intelfx · 2017-08-08T13:56:54Z

I have shuffled commits a bit (misrebase; two commits got their contents swapped). No real code changes.

intelfx added 2 commits August 1, 2017 06:46

func tests: allow running tests selectively

6f753ab

func tests: include command's stdout/stderr in Error exception messages

1fec8c4

intelfx requested a review from vankoven August 1, 2017 13:49

vankoven suggested changes Aug 1, 2017

View reviewed changes

intelfx added 4 commits August 3, 2017 15:18

func tests: include previous exception details in nested Error except…

1a144e6

…ions

func tests: do not create SSH connections at import time

c6013df

cf. https://stackoverflow.com/questions/443387/why-does-paramiko-hang-if-you-use-it-while-loading-a-module, paramiko/paramiko#104

func tests: increase start/stop command timeouts

64ca29c

func tests: helpers/remote.py: cleanup Node.copy_file()

a6a6c09

principle of least astonishment

intelfx force-pushed the is-759 branch 4 times, most recently from ea97f3b to c7a6750 Compare August 4, 2017 13:29

vankoven reviewed Aug 7, 2017

View reviewed changes

vankoven approved these changes Aug 7, 2017

View reviewed changes

intelfx force-pushed the is-759 branch from c7a6750 to a040266 Compare August 7, 2017 13:24

krizhanovsky mentioned this pull request Aug 8, 2017

Crash in tfw_sched_hash_del_grp() #794

Closed

intelfx added 14 commits August 8, 2017 16:53

func tests: do not fail on inexistent files in RemoteNode.remove_file()

3a261c3

func tests: Nginx.get_name(): return ip, not hostname

7b2d5e2

func tests: flacky_net.test_sockets: improve error messages

8f356a2

func tests: store servers' addresses in their objects (improve readab…

06c8acc

…ility)

func tests: check for EUID == 0

6f683bb

func tests: increase RLIMIT_NOFILE to 1048576 (fs.nr_open)

138a24a

func tests: improve path handling (use os.path.join)

c2fc209

func tests: create a temporary tempesta config and clean it up after …

324901e

…the test

func tests: sessions.test_sticky_sess_stress: run wrk with single thread

0e0b9ad

Each thread in wrk has its own Lua context and hence get its own session cookie, failing the test. Found by @ikoveshnikov

func tests: sessions.test_sticky_sess_stress: account for more stats …

90f728d

…discrepancies

func tests: testers.stress: wording, slight fixes to stats checks

4aac090

func tests: drop mentions of siege (unused)

e79788d

func tests: add missing fields to the sample config and README

9596e44

func tests: improve wording in sample config and README; document new…

3853da8

… run_tests.py

intelfx force-pushed the is-759 branch from a040266 to 3853da8 Compare August 8, 2017 13:55

vankoven mentioned this pull request Aug 8, 2017

run_tests.py requires dummy tests_config.ini #793

Closed

intelfx merged commit 85ac463 into master Aug 9, 2017

intelfx deleted the is-759 branch August 9, 2017 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various fixes to functional tests #783

Various fixes to functional tests #783

intelfx commented Aug 1, 2017

vankoven Aug 1, 2017

vankoven Aug 1, 2017

intelfx Aug 1, 2017

vankoven Aug 2, 2017 •

edited

Loading

vankoven Aug 1, 2017

vankoven Aug 1, 2017

vankoven Aug 1, 2017

vankoven Aug 1, 2017

intelfx Aug 1, 2017

vankoven Aug 1, 2017

vankoven Aug 1, 2017

vankoven Aug 1, 2017

vankoven Aug 1, 2017

vankoven left a comment •

edited

Loading

vankoven Aug 7, 2017

vankoven Aug 7, 2017

vankoven Aug 7, 2017

vankoven Aug 7, 2017

vankoven left a comment

intelfx commented Aug 8, 2017

Various fixes to functional tests #783

Various fixes to functional tests #783

Conversation

intelfx commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vankoven left a comment

Choose a reason for hiding this comment

intelfx commented Aug 8, 2017

vankoven Aug 2, 2017 •

edited

Loading

vankoven left a comment •

edited

Loading