BIND 10 #2003: support DDNS/TCP response

[BIND 10 Development]

#2003: support DDNS/TCP response
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  low    |  Sprint-20120612
                  Component:  DDNS   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:  DDNS   |  Estimated Difficulty:  5
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 vorner]:

 Thanks for the review.

 > Replying to [comment:4 jinmei]:
 > > I believe the intent of the former is clear from its documentation.
 > > I've made this as a separate module so it can be eventually used
 > > for revised versions of xfrin/xfrout, too.
 >
 > I thought we would use blocking reads/writes in xfrin/out (such as
 socket.sendall). I thought this is the reason why we wanted separate
 forked processes for them.

 I know, and, I still thought utility class(es) like this would be
 useful for them.  We'll still need to handle the 2-byte length field
 some way, so something like `DNSTCPSendBuffer` will be helpful even if
 we use blocking write.  Also, unless we concatenate the length field
 and the actual data binary, we'll still need to call socket.send()
 variants multiple times.  An imaginary extended version of
 `DNSTCPContext` (which would support a blocking mode) would be
 helpful.  Further, at least xfrin will need to receive incoming
 messages, which will require multiple calls to recv variants even if
 each call is blocking.  Another imaginary extended version of
 `DNSTCPContext` (which would support receiving messages, and it's
 necessary even for ddns if we want to keep the TCP connection for
 multiple requests) will be helpful in that case.

 > Not that it would be a problem for the class, but I think it would be an
 overkill for such purpose.

 So, I thought these utilities will be useful even for blocking-based
 applications.  It might still be an overkill if they are only intended
 for blocking apps, but as long as we have non blocking apps (like
 b10-ddns) we'll still need some code that is implemented in this
 module, and, if the support for the blocking apps can be done with a
 relatively minor extensions to the base implementation, I believe it's
 worth doing.  This is basically why I made it as a separate module and
 placed it under server_common from the beginning.

 That said, I also reckon that it may be possible that we won't need
 such abstraction for other applications in the end.  So I tried to
 make it not too generalized at the moment, restricting the necessary
 features for b10-ddns.  If it becomes clear that b10-ddns is the only
 user of these utilities in the end, we can move it inside ddns.py or
 some ddns specific package.

 > I think you forgot to put the dns_tcp_test.py to git. I hope you still
 have it somewhere. This is what I get (both check and distcheck) and I
 don't think I saw the test in the diff:
 > {{{
 > make[6]: Leaving directory
 `/home/vorner/work/bind10/src/lib/python/isc/server_common/tests'
 > make[6]: *** No rule to make target `dns_tcp_test.py', needed by
 `distdir'.  Stop.
 > make[5]: Leaving directory
 `/home/vorner/work/bind10/src/lib/python/isc/server_common'
 > }}}

 Oops, sorry.  I naively thought I'd added everything by confirming
 distcheck passed.  I've git-added and pushed it.

 > Is dropping the connection silently on the quota the best thing to do?

 I agree that's debatable.  Which options other than this are you
 thinking about?

 - returning SERVFAIL or REFUSED?  but it requires to keep the
   connection at least until the transmission of the response is
   completed.
 - buffering the pending requests?  It will still require the server to
   maintain more resource than it's configured, and, it'll be more
   likely that the client times out the request.
 - killing oldest client to allow a new client?  Maybe, but maybe not;
   if the server is simply busy, it could be possible that the server
   is just killing any TCP client before completing any of the
   requests.

 I checked what other server implementations (BIND 9, NSD and unbound)
 do for TCP clients beyond the configured quota.  All of them are
 basically pending the requests by not doing accept() on the listening
 socket.  So it's closer to the second option, although the resource is
 maintained in the kernel, not in the application.  Also, if it takes
 too long for existing clients to be completed, newer requests will be
 refused at the TCP level anyway.

 So, none of the options looked ideal to me, and I chose the simplest
 solution, which is actually not so different from other
 implementations in the end result.

 We can certainly revisit the behavior, but that will be a topic of a
 separate ticket, maybe starting from a team wide discussion.

 > And, generally speaking, isn't 10 as the quota really low?

 This is also debatable, and I'm not sure.  BIND 9's default of
 tcp-clients (not specific to ddns) is 100, so maybe we should use the
 same value.  In any case, as long as we use select, we cannot increase
 it too much due to its scalability.

 > Should the `PYSERVER_COMMON_DNS_TCP_SEND_ERROR` be logged as an ERROR
 instead of WARN? AFAIK WARN is for things that are possibly correct but
 might be wrong, while I don't see a way this could be correct in any
 situation.

 I changed it to WARN, after thinking about it more, with the rationale
 that socket error can happen for reasons outside of the server itself,
 so ERROR seemed to strong.  Also, WARN is the BIND 9's log level for
 an event that sending a response fails (for an error at the layer of
 socket API or below it).  I do not necessarily insist on it though -
 we can discuss the appropriate level for this type of events.

 > The two of the new log messages include the amount of data sent so far.
 Should it log how much it is total as well? (like „%2 of %3 bytes sent so
 far“)?

 Done at commit 85fcef3.  I considered it in the original
 implementation, but didn't do it at that time because it would require
 a cleaner interface to get the total length of the data including the
 length field.  But now that it was asked explicitly, I added the
 interface and updated the log.

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