BIND 10 #1429: Commands for sockets

[BIND 10 Development]

#1429: Commands for sockets
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111206
                  Component:  Boss   |            Resolution:
  of BIND                            |             Sensitive:  0
                   Keywords:         |           Sub-Project:  Core
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
  Socket creator                     |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:6 jinmei]:
 > - command_handler() is getting quite big.  Maybe we should start
 >   considering refactoring it, e.g., using the command design pattern.
 >   but in any event it's not a direct subject of this ticket.  this is
 >   just a comment at a higher level.

 Yes, it might be nice to have a dict name:function and pick them from
 there, or something like this. But it's not something I'd like to include
 here, while simple, it will be change with large diff and will take
 nontrivial time.

 > - I don't know the details of socket cache's drop_socket()
 >   implementation, but depending on how it handles invalid input, isn't
 >   it simpler to let it check the case where 'token' isn't specified?
 > {{{#!python
 >             elif command == "drop_socket":
 >                 try:
 >                     self._socket_cache.drop_socket(args.get("token"))
 >                     answer = isc.config.ccsession.create_answer(0)
 >                 except Exception as e:
 >                     answer = isc.config.ccsession.create_answer(1,
 str(e))
 >
 > }}}

 Well, it would certainly be slightly simpler, but the error message would
 be less descriptive, as it would say something like there's no socket
 identified by token None. On the other hand, user probably should never
 call this method manually, so it's a question what we want to do.

 > - socket_request_handler
 >  - Why is logging still TODO?  Same for socket_consumer_dead()

 It was caused by inherently unreliable human memory. It should be fixed
 now.

 >  - I don't know how exactly this method is used, but
 >    get_socket()/sendall()/send_fd() could block, right?  Is it okay
 >    for the boss to hang due to an unresponsive remote end?  This may
 >    not only be for the socket passing, but would also be an issue for,
 >    e.g., command interface even if it's not hang but delay, but as we
 >    introduce more communication channels/blocking points, I think we
 >    should start considering the effect more seriously.  Not
 >    necessarily a blocking issue for this ticket, though.

 Yes, they could block. Actually, get_socket can't block, it's get_token
 which can, as it talks to the socket creator.

 However, I believe that:
  * Socket creator is on the same machine and the chance of it to lock up
 is as low as having an infinite loop in the boss itself (or I could think
 even lower, as the boss is more complicated code). So the only delay here
 would be two context switches, which is less than is needed for a command
 to pass to remote application and answer to return.
  * The sendall and send_fd ‒ the boss is sending only small amounts of
 data. It should fit into the OS buffer of the socket without any problems.
 If the remote application locks up, it doesn't request any new sockets, so
 the buffer wouldn't fill.
  * There'll be only small number of sockets requested and only on startup
 and reconfiguration, so even if there are small delays, they shouldn't
 cause any performance problems.

 On the other hand, if we were to make sure they don't block, the handling
 around select would become significantly more complicated or we would need
 to introduce threads, with some kind of locking, etc.

 My assumptions may be wrong, but if they are right, then solving the
 problem would be waste of time. So I'd propose waiting with the solution
 until we find out it is a problem. Would it be OK?

 > - socket_consumer_dead()
 >   - is it okay not to catch exceptions?

 Ups, you're right. It's maybe even worse than you thought, as the
 exception can happen in normal operation and is perfectly OK. Added.

 >   - if the argument to drop_application() is the same one as the
 >     'application' parameter of get_socket(), the assumptions seem to
 >     be consistent: here it assumes to be an FD; in the latter it seems
 >     to be undecided yet according to the docstring.

 Hmm, I fixed the docstring in the cache. Or did you mean some other? The
 cache itself doesn't care, so it was actually decided in this ticket.

 > - insert_creator(): maybe set_creator() is a better name based on
 >   general convention used throughout the BIND 10 code

 OK, no problem.

 > '''bind10_test.py'''
 >
 > - Maybe we should rather import everything from bind10_src now?
 > {{{#!python
 > from bind10_src import *
 > #from bind10_src import ProcessInfo, BoB, parse_args, dump_pid,
 unlink_pid_file, _BASETIME
 > #import bind10_src
 > }}}

 The lower import of bind10_src is there because I need to replace a call
 from library that one uses, this one:
 {{{#!python
 bind10_src.libutil_io_python.send_fd = self.__send_fd
 }}}

 I'm not sure if it worked if it was imported by * (eg. if * imports the
 things the other module imports into itself), but even if it did, I thing
 it would be quite unreadable. This intention is more visible I believe.

 > - "get_socket" test: I'd use an IPv6 address for this test (so we can
 >   cover both the future and mostly-deprecated versions of IP).  We'll
 >   need to replace _socket_cache.get_token to make it meaningful.

 Hmm, I'm not sure if I got the correct idea which test you mean. The stuff
 in this ticket only passes the address through (OK, it creates the IPAddr
 from the string), so it doesn't really matter which address it is. It is
 distinguished in the IPAddr class (which is tested) and deep down in the
 socket creator class that encodes it to the socket and sends it to the
 socket creator process (which is tested too). Did you mean this test, or
 another?

 Thank you

 With regard

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