0.10-cs: client-server rewritten as modules / start in foreground / test coverage for cs #1413

sebres · 2016-05-12T11:11:27Z

closes #1121
closes #1139
Several enhancements, bug fixed and code optimization:

starting of the server (and client/server communication behavior during start and daemonize) completely rewritten:
- client/server functionality moved away from bin and using now the common interface (introduced in fail2bancmdline);
- start in foreground fixed;
- server can act as client corresponding command line;
- command "restart" added: in opposite to "reload" in reality restarts the server (new process);
- several client/server bugs during starting process fixed.
possibility to increase verbosity up to heavy debug (command line parameter -vvv)
increase readability and details level by increased verbosity
reader bug fix: prevent to silent "load" of not existing jail;
coverage of test cases increased;
several bug fixed: fork in client-server test cases prohibited, all worker threads daemonized (to prevent hanging on exit).

# Conflicts: # fail2ban/server/server.py

…ng start and daemonize) completely rewritten: - client/server functionality moved away from bin and using now the common interface (introduced in fail2bancmdline); - start in foreground fixed; - server can act as client corresponding command line; - command "restart" added: in opposite to "reload" in reality restarts the server (new process); - several client/server bugs during starting process fixed.

, closes fail2ban#1139 small code review and fixing of some bugs during client-server communication process (in the test cases);

…orker threads daemonized (to prevent hanging on exit).

…eview), wait 4 server ready, test cases fixed (py2/py3)

… of server/client (with or without fork)

coverage of test cases increased;

codecov-io · 2016-05-12T11:17:23Z

Current coverage is 92.11%

Merging #1413 into 0.10 will increase coverage by 0.75%

@@               0.10      #1413   diff @@
==========================================
  Files            71         75     +4   
  Lines          9235      10168   +933   
  Methods           0          0          
  Messages          0          0          
  Branches       1399       1548   +149   
==========================================
+ Hits           8437       9366   +929   
+ Misses          567        536    -31   
- Partials        231        266    +35

Powered by Codecov. Last updated by 7582f13...f0b2cd2

yarikoptic · 2016-05-12T12:17:49Z

bin/fail2ban-testcases

+	if verbosity > 2:
+	  fmt = ' +%(relativeCreated)5d %(thread)X %(levelname)-5.5s' + fmt
+	else:
+		fmt = ' %(asctime)-15s %(thread)X %(levelname)-5.5s' + fmt


Seems mixed indentation with spaces and tabs

yarikoptic · 2016-05-12T12:27:28Z

Why didn't you listen to a wise man? #1139 (comment)
It would be much easier to review if we first just merged the move files away PR, without any functionality change. Now impossible to see what was moved and what was moved and then changed

yarikoptic · 2016-05-12T12:28:35Z

fail2ban/tests/utils.py

@@ -386,6 +412,9 @@ def assertNotLogged(self, *s, **kwargs):
 	def pruneLog(self):
 		self._log.truncate(0)

+	def pruneLog(self):
+		self._log.truncate(0)
+


yarikoptic · 2016-05-12T12:34:26Z

So please do first the move, send it as a PR, we merge it after tests pass, and hopefully this diff becomes manageable

sebres · 2016-05-12T13:19:23Z

Why didn't you listen to a wise man? #1139 (comment)

Because:

it can't be test covered there;
in single commit I've moved bin/fail2ban-client -> fail2banclient and bin/fail2ban-server -> fail2banserver
I brought together CS in command line (double code -> single code)
the wise man can try to do it self, if he says it is lighter :))) With almost 30 years of developing behind back am I already wise enough to determine, that it is not so easy :)))

Now impossible to see what was moved and what was moved and then changed

Wrong, it is possible - just compare:

old bin/fail2ban-client with new fail2banclient / fail2bancmdline
old bin/fail2ban-server with new fail2banserver / fail2bancmdline

My git does it automatically (it can find the code lines, moved from other files)

So please do first the move, send it as a PR, we merge it after tests pass, and hopefully this diff becomes manageable

Move what and where?

yarikoptic · 2016-05-12T13:28:20Z

On Thu, 12 May 2016, Serg G. Brester wrote:

 Why didn't you listen to a wise man? #1139 (comment)

Because:

 * it can't be test covered there;

I am not arguing against moving altogether, and understand that it
couldn't test covered there

 * in single commit I've moved bin/fail2ban-client -> fail2banclient and
   bin/fail2ban-server -> fail2banserver

good!

 * I brought together CS in command line (double code -> single code)

good -- that could be a good point [lets call it POINTX] to send a
PR at without piling up additional fixes/etc. Just code move +
refactoring to reduce duplication

 * the wise man can try to do it self, if he says it is lighter :))) With
   almost 30 years of developing behind back am I already wise enough to
   determine, that it is not so easy :)))

not sure what was the difficulty to have a PR for POINTX

 Now impossible to see what was moved and what was moved and then changed

Wrong, it is possible - just compare:

 * old bin/fail2ban-client with new fail2banclient / fail2bancmdline
 * old bin/fail2ban-server with new fail2banserver / fail2bancmdline

I meant while reviewing PR on github. I often do reviews from the
phone so can't easily checkout /compare files otherwise

My git does it automatically (it can find the code lines, moved from other
files)
 So please do first the move, send it as a PR, we merge it after tests
 pass, and hopefully this diff becomes manageable
Move what and where?

could there be a PR for POINTX mentioned above?

Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik

sebres · 2016-05-12T13:42:18Z

I don't know all the test cases of this commit pass, I'll make a commit 556ddaa as separated PR.

I can remember a "bug" for 'flushed' vs 'rolled over' in testFlushLogs, was fixed later from me...

yarikoptic · 2016-05-12T13:56:24Z

btw -- tests fail for me (only) strangely since seems to (rightfully?) need a sample log file which is not in git... confused (don't have time atm to investigate) how it passed on travis:

$> bin/fail2ban-testcases testCheckStockJailActions
Fail2ban 0.9.4.dev0 test suite. Python 2.7.11+ (default, Apr 17 2016, 14:00:29) [GCC 5.3.1 20160409]. Please wait...
F
======================================================================
FAIL: testCheckStockJailActions (fail2ban.tests.servertestcase.ServerConfigReaderTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./fail2ban/tests/servertestcase.py", line 1046, in testCheckStockJailActions
    self.fail("Command %r has failed. Received %r" % (cmd, e))
AssertionError: Command ['set', 'pass2allow-ftp', 'addlogpath', './fail2ban/tests/files/logs/pass2allow-ftp', 'head'] has failed. Received IOError(2, 'No such file or directory')

----------------------------------------------------------------------
Ran 1 test in 0.397s

FAILED (failures=1)

$> ls -l './fail2ban/tests/files/logs/pass2allow-ftp' 
ls: cannot access './fail2ban/tests/files/logs/pass2allow-ftp': No such file or directory

sebres · 2016-05-12T14:07:42Z

You have not clean copy - perhabs one custom config pass2allow-ftp local (uncommited) and for it no logs.
testCheckStockJailActions loads all stock filters / action and transmit it to the TestServer...

I can rewrite this test case, that will ignore all configs, that don't have corresponding test log file...

Or just ignore this one...

bin/fail2ban-testcases -i testCheckStockJailActions

…ameter `-vvv`)

…re readable) case shows how evil except Exception can be ;)

yarikoptic · 2016-06-19T03:48:04Z

fail2ban/client/fail2bancmdline.py

+		return True
+
+	@staticmethod
+	def exit(code=0): # pragma: no cover - can't test


nope -- pragma is incorrect -- could be tested via mocking at least for the smoke testing and/or logic

Currently the global exit handler is overwritten in test cases (to test forked processes exit code), so it can't (we need a return code).

Indeed, we could rewrite it using mock module, but the overhead is unnecessarily (several times larger) for coverage of this 2 lines.

coveralls · 2016-07-11T11:31:54Z

Coverage increased (+0.9%) to 94.737% when pulling 6a2b70f on sebres:f2b-perfom-prepare-716-cs-0.10 into 7582f13 on fail2ban:0.10.

sebres · 2016-07-11T12:14:37Z

I'm done, ready to merge to 0.10...

Still again: that is a first shot (minimal invasive surgery), better mocking can be made later, because the test cases are almost wholehearted (I mean expected behavior of CS).

… PEP8 friendly

…ace need for custom --s)

…d runs per client/server

yarikoptic · 2016-07-12T04:09:58Z

sent you a PR sebres#6

pid_exists already available in Utils; small bug and stability fixing;

sebres · 2016-07-12T10:16:40Z

sebres#6 merged, but rewritten a little bit (see 2 commits above)
Thanks

…ers + minor PEP8ing

…cking it

…ation)

coveralls · 2016-07-13T07:47:10Z

Coverage increased (+0.9%) to 94.716% when pulling 53da35e on sebres:f2b-perfom-prepare-716-cs-0.10 into 7582f13 on fail2ban:0.10.

yarikoptic · 2016-07-13T16:01:07Z

I still don't like having tests specific logic within code itself (all the PRODUCTION thing) and persistent mocking of stuff within test module, but I guess we will leave it for the future

sebres added 9 commits May 12, 2016 11:51

try to start server in foreground

4ce240e

# Conflicts: # fail2ban/server/server.py

temporary commit (move client/server from bin)

556ddaa

client/server (bin) test cases introduced, ultimate closes fail2ban#1121

afa1cdc

, closes fail2ban#1139 small code review and fixing of some bugs during client-server communication process (in the test cases);

several bug fixed: fork in client-server test cases prohibited, all w…

2fcb635

…orker threads daemonized (to prevent hanging on exit).

some compatibility fixes (prevent forking of testcase-process, code r…

0b41437

…eview), wait 4 server ready, test cases fixed (py2/py3)

code review, timeout fix, better tracing (and test coverage) by start…

22576d7

… of server/client (with or without fork)

reader bug fix: prevent to silent "load" of not existing jail;

060ea08

coverage of test cases increased;

increase readability and details level by increased verbosity

5395650

sebres added this to the 0.10.0 milestone May 12, 2016

sebres mentioned this pull request May 12, 2016

WiP: do not merge! 0.10 -> master merge whenever it is ready to be named a 'stable'-ish one #1410

Closed

yarikoptic reviewed May 12, 2016
View reviewed changes

sebres mentioned this pull request May 12, 2016

Preliminary PR for 0.10-cs (gh-1413) #1414

Closed

sebres mentioned this pull request May 12, 2016

0.10 fixes #1415

Merged

sebres and others added 4 commits May 17, 2016 12:26

possibility to increase verbosity up to heavy debug (command line par…

6353de8

…ameter `-vvv`)

BF+ENH: import signal used in the code, withtmpdir -> with_tmpdir (mo…

e57321a

…re readable) case shows how evil except Exception can be ;)

RF+ENH: @with_kill_srv fixture to kill_srv in the tests

d7ff7d1

fixed mixed indentation (spaces through tabs), duplicate code removed

06dcad7

sebres force-pushed the f2b-perfom-prepare-716-cs-0.10 branch from dfd9121 to 06dcad7 Compare May 17, 2016 10:27

yarikoptic reviewed Jun 19, 2016
View reviewed changes

sebres added 3 commits July 11, 2016 11:28

Merge branch '0.10' into f2b-perfom-prepare-716-cs-0.10

4c1bcac

code review

4180cc3

os.path.join instead of "/" - concatenation

6a2b70f

yarikoptic added 5 commits July 11, 2016 22:42

RF(PEP8 etc): Make new fail2banclienttestcase a bit more readable and…

94cada0

… PEP8 friendly

ENH: testcases - verbosity > 2 now also includes logger name (to repl…

4df06f5

…ace need for custom --s)

RF: group common tests in base class, specialize assesment of good/ba…

1417cc9

…d runs per client/server

RF: revertably mock out exit call while testing new client/servers

fcda7c9

minor residual cleanups

3c42b4f

sebres added 2 commits July 12, 2016 12:02

code review after enhancements of Yaroslav

7242c9c

test of _kill_srv added, don't kill server if foreground;

0dc3db1

pid_exists already available in Utils; small bug and stability fixing;

yarikoptic and others added 4 commits July 12, 2016 22:24

RF: _rebindSignal helper to centralize assignment of new signal handl…

52cd8a9

…ers + minor PEP8ing

BF+TST: isStarted was not checking __asyncServer for exist before che…

dfcf6f1

…cking it

outsource assert..isStarted from setup into separate test case

ad393fc

resolves Christmas-tree in logging (replaced with duplex space indent…

53da35e

…ation)

remove no more necessary PRODUCTION from fail2banclient.py

f0b2cd2

yarikoptic merged commit bf3188c into fail2ban:0.10 Jul 13, 2016

This was referenced Jul 13, 2016

Move all bin/fail2ban- functionality under fail2ban/cmdline/ module #1121

Closed

Support foreground with fail2ban-server #1139

Closed

sebres mentioned this pull request Jun 9, 2021

Improve systemd service #2537

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.10-cs: client-server rewritten as modules / start in foreground / test coverage for cs #1413

0.10-cs: client-server rewritten as modules / start in foreground / test coverage for cs #1413

sebres commented May 12, 2016 •

edited

codecov-io commented May 12, 2016 •

edited

yarikoptic May 12, 2016

sebres May 12, 2016

yarikoptic commented May 12, 2016

yarikoptic May 12, 2016

sebres May 12, 2016

yarikoptic commented May 12, 2016

sebres commented May 12, 2016

yarikoptic commented May 12, 2016

sebres commented May 12, 2016

yarikoptic commented May 12, 2016

sebres commented May 12, 2016

yarikoptic Jun 19, 2016

sebres Jul 11, 2016

coveralls commented Jul 11, 2016 •

edited

sebres commented Jul 11, 2016 •

edited

yarikoptic commented Jul 12, 2016

sebres commented Jul 12, 2016

coveralls commented Jul 13, 2016 •

edited

yarikoptic commented Jul 13, 2016

0.10-cs: client-server rewritten as modules / start in foreground / test coverage for cs #1413

0.10-cs: client-server rewritten as modules / start in foreground / test coverage for cs #1413

Conversation

sebres commented May 12, 2016 • edited

codecov-io commented May 12, 2016 • edited

Current coverage is 92.11%

yarikoptic May 12, 2016

Choose a reason for hiding this comment

sebres May 12, 2016

Choose a reason for hiding this comment

yarikoptic commented May 12, 2016

yarikoptic May 12, 2016

Choose a reason for hiding this comment

sebres May 12, 2016

Choose a reason for hiding this comment

yarikoptic commented May 12, 2016

sebres commented May 12, 2016

yarikoptic commented May 12, 2016

sebres commented May 12, 2016

yarikoptic commented May 12, 2016

sebres commented May 12, 2016

yarikoptic Jun 19, 2016

Choose a reason for hiding this comment

sebres Jul 11, 2016

Choose a reason for hiding this comment

coveralls commented Jul 11, 2016 • edited

sebres commented Jul 11, 2016 • edited

yarikoptic commented Jul 12, 2016

sebres commented Jul 12, 2016

coveralls commented Jul 13, 2016 • edited

yarikoptic commented Jul 13, 2016

sebres commented May 12, 2016 •

edited

codecov-io commented May 12, 2016 •

edited

coveralls commented Jul 11, 2016 •

edited

sebres commented Jul 11, 2016 •

edited

coveralls commented Jul 13, 2016 •

edited