BIND 10 #930: update stats modules to get statistics list from cfgmgr, and define the list in spec file of each module

[BIND 10 Development]

#930: update stats modules to get statistics list from cfgmgr, and define the
list in spec file of each module
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  naokikambe                         |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110816
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  statistics                         |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  20.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by naokikambe):

 * owner:  naokikambe => vorner


Comment:

 Hello, thank you for giving comments again.

 Replying to [comment:11 vorner]:
 > By the added code, you mean the validation?
 Yes.

 > I'm slightly nervous about these changes. While validation is important,
 I think it should be done on the receiving side (in the stats module), for
 two reasons.
 >
 > One is, if it is checked by the module which is sending the data, it
 does not solve much. If it is buggy, it can as well do the validation
 wrong and send wrong data anyway. And if the module wants to be malicious,
 nothing stops it from being so. Therefore I don't think validation on the
 sending side solves any problem (or, I at last don't see any problem being
 solved by this).

 I'm very sorry if I am misunderstanding. Of course the stats module
 validates statistics data received from other modules. Here the validation
 is just a spec-format validation before sending statistics data. The
 validation is done by a non-stats module itself (like auth, boss etc). For
 example the implementation of this validation is like
 ModuleSpec::validateConfig or like ModuleSpec::validateCommand already
 implemented. And Jelte-san commented in #928 that statistics validation
 also would be required for other modules.([comment:6:ticket:928])

 > Another is, when it is in the stats module, it is enough to be
 implemented once for all modules and save a lot of almost duplicate code
 in many modules.

 Yes, we need to consider more about the duplicate implementation. But I
 think that might be beyond the scope of #930 a little.

 > As I'm looking into it, I think just placing the _started.set() at the
 right place would be enough - like when the thing is already prepared to
 do whatever is needed. In most cases it looked like it already is placed
 where it should anyway, but I didn't study it too properly.

 That's right but it seems that _started.set()/wait() actually works
 properly. I confirmed it by a simple script as following.

 {{{#!python
 import time, threading
 import test_utils

 class MockThreadingServerManager(test_utils.ThreadingServerManager):
     def run(self):
         assert self.server._started.is_set() is False
         self.server._thread.start()
         print(str(time.time()))
         self.server._started.wait()
         assert self.server._started.is_set() is True
         self.server._started.clear()
         assert self.server._started.is_set() is False
         print(str(time.time()))

 class Mock:
     def __init__(self):
         self._started = threading.Event()
         self.running = False
     def run(self):
         time.sleep(3)
         assert self._started.is_set() is False
         self._started.set()
         assert self._started.is_set() is True
         self.running = True
         while self.running: time.sleep(1)
     def shutdown(self):
         self.running = False

 if __name__ == "__main__":
     mock = MockThreadingServerManager(Mock)
     mock.run()
     mock.shutdown()
 }}}

 However actually more wait is required after _started.set() is done (in
 other word after _started.wait() is done). As one solution, should we move
 _started.set() to inside of the target module, for example, just before
 the while-loop in the target module is started? But it might be not good
 to customize inside of the target module for test only.

 > It might be a nice bonus, but nothing really important IMO. Let's leave
 it for now, I was just curious because I didn't find the code producing
 them originally.

 OK. Thank you.

 > The `test_command_show` still uses the numeric "magic“ constants. It is
 correct, as they are set and checked 2 lines from each other and it is a
 different test fixture than the rest, so it is maybe even easier, I just
 wanted to point it out, as it might be less clean. But if you like it this
 way, it is OK.

 I also moved these variables into the setUP function as constants.

 > No, no need to remove it, actually testing the assumption is better. It
 is just unusual for the variable being set, maybe adding a comment to the
 assert it comes from the Makefile to clarify?

 OK, but I moved it from the setUp, and I merged it with another OS
 environment test(test_osenv) into another unittest class.

 > Well, they help a little. But I'd appreciate something like listing of
 what is being done and how. I'm not sure this is true for the code (so
 check after me it doesn't lie), but maybe something like this?:
 >
 > {{{
 > In each of these tests we start several virtual components. They are not
 the
 > real components, no external processes are started. They are just simple
 mock
 > objects running each in its own thread and pretending to be bind10
 modules. This
 > helps testing the stats module in a close to real environment.
 > }}}

 Very good comment, thank you! I almost pasted it in header of each
 unittest script as it is.

 That changes are pushed at git 3e3f4cad1dd4068070cac2e322d070df563565eb.
 And I also added trivial changes at
 81724ff9d7b84fffbef12d5fe29110ac648cc27f before. Please see these.

 Best regards,

-- 
Ticket URL: <http://bind10.isc.org/ticket/930#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development