BIND 10 #2862: update b10-auth to recognize data source memory segments

[BIND 10 Development]

#2862: update b10-auth to recognize data source memory segments
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  b10-auth      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130723
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Replying to [comment:6 vorner]:
 > Hello
 >
 > It is ready for review. However, there are some points worth
 > discussing.
 >
 > There are some new functionalities in the `ModuleCCSession`. It seems
 > we are bending it for something that it doesn't want to do right now
 > (pretending to be some other module). Also, part of that new
 > functionality is not really clean. I needed something, but the ticket
 > is taking a long time already and it didn't seem directly relevant, so
 > I hacked it in. But a real solution would be needed.

 I am not very familiar with CC so I ask.. what is the proposed solution
 to replace the code that you have put in place?

 > Also, there's a related part of the auth code that is not tested,
 > because it sounds it is very hard to test (see the last commit). I
 > think this will be better tested by a lettuce test (and that we'll
 > need one in the end anyway, because I'm bit worried if the
 > communication in auth will match the one in the memmgr).

 Nod. Please add a ticket towards this so it's not forgotten, and add it
 to the meta ticket to do after the memmgr implementation is in place.

 > So, there are two possibilities. Create a ticket to clean up the
 > `ModuleCCSession`, check that there's a ticket for the lettuce
 > test. Or the other is to redesign the communication with primitives we
 > already have (eg. notifications and commands).

 See above. I'm not familiar with how CC is lacking here. Please explain
 what you propose.

 > The other thing is, what to do if the update of the segment fails. We
 > currently just skip it (hoping it is just out of sync with the memmgr
 > and because of different configuration, and that'll will get synced
 > soon). But we ACK it while still using the old segment
 > (possibly). Should we release the segment? Or detect the error somehow
 > and signal it?

 From Jinmei's design descriptions, it seems memmgr assumes that once a
 `segment_info_update` command is acknowledged, the reader stops using
 any older mapped file.

 We can also have multiple instances of auth where, for example, just one
 process fails to map it. In this case, there'd arise the case where some
 auth processes are serving an old segment and some are serving the new.

 In some cases, such as during memmgr restart when there are existing
 auth processes already, memmgr could send (after checksum verification)
 a `segment_info_update` with the same existing mapped file as an
 argument (the mapped file that auth already has open). In this case (due
 to some lower-level design issues with mapped files), datasrc closes the
 existing mapped file first before it attempts to open the new one. auth
 does not have a mapped file open anymore at that point. During reset,
 any strong exception safety guarantees are also conditional based on the
 open mode, etc.

 Considering all this, I think we should (or rather, are forced to)
 release the old segment before we ACK.

 Here are some code comments:

 * `readers_subscribed_` in `AuthSrvImpl` should include the `group` word
   (such as `readers_group_subscribed_`. I know it's bikeshedding, but
   `readers_subscribed` somehow seems lacking.

 * The opposite of "local" is not "remote", but "mapped" in the case of
   `hasRemoteSegment()`. It should be renamed to
   `hasMappedSegment()`. Also I think it's better to specifically search
   for "mapped" from `getSegmentType()` instead of !"local".

 * Is it possible (or meaningful) to have a mapped segment and
   `cached-enable: false`?

 * The ticket description seems to incorrectly miss conditions, like when
   the cache state is not WAITING and auth still needs to subscribe to
   the readers group to get updates for future resets. Your code seems to
   handle it.

 * The "segment_info_update" command seems renamed to
   "sgmtinfo-update". Is there any reason for this, when most of the
   design documentation (in other tickets also) refers to
   "segment_info_update"? Same with "sgmtinfo-update-ack".

 * In the "sgmtinfo-update-ack", are you sending back all the params sent
   with the "segment_info_update" command? Isn't it enough to send the
   class and data source name?

 * If there is a failure, shall we add an arg to notify memmgr of a
   failure to reset condition in the "sgmtinfo-update-ack" reply (in any
   case, regardless of how auth decides to handle it internally - see
   discussion above)?

 * What happens when an exception occurs during `resetMemorySegment()`?
   Currently the code seems to be catching the exception and logging
   it. But do you have an understanding of how auth handles further
   queries? The strong exception guarantee during reset is conditional.
   This particular portion needs to be tested.

 * I couldn't figure this out quickly reading the auth code, but is
   `doSegmentUpdate()` synchronized with the rest of auth? I.e., when the
   builder thread is resetting the segment, does auth respond to queries
   during that time?

 * I don't see `DATASOURCE_CONFIGURED` at all in the code. Is this
   supposed to be handled in `reconfigureDone()` ? From the ticket
   description does the first `DATASOURCE_CONFIGURED` map to
   `listsReconfigured()` and the second to `reconfigureDone()` ? I'm
   afraid I don't follow this at all in the ticket description:

 > Also, if this is the last outstanding segment, it sends the
 DATASRC_CONFIGURED command with {"cache-state": INUSE}.

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