BIND 10 #2210: support "reconfigure" command in the configurator thread

[BIND 10 Development]

#2210: support "reconfigure" command in the configurator thread
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121023
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:9 jinmei]:
 > I have one substantial point to discuss.  Others are minor.
 >
 > '''substantial'''
 >
 > - `doReconfigure()` in datasrc_clients.mgr: I think it's safer to
 >   define new_clients_map outside of the try block (more essentially
 >   the scope for the locker):
 > {{{#!cpp
 >             isc::datasrc::DataSrcClientListsPtr new_clients_map;
 >             try {
 >                 new_clients_map = configureDataSource(config);
 > }}}
 >   to make it more clear that swapped map (stored in new_clients_map
 >   after swap) will be released outside of the lock.  That's probably
 >   already the case in the current code due to the ordering of the
 >   variable definition, but I'm afraid it's subtle and possibly
 >   fragile.  We also probably need to comment about the rationale of
 >   the lock timing, just like we did in main.c:datasrcConfigHandler().
 >

 Actually, I did this, then came to your later suggestion to add log
 messages, and since I decided to put the 'done' log message in the try
 block as well (so that it can say 'success' instead of 'done whether or
 not there was an error'), I just put the lock+swap code into its own
 explicit block scope (ie. not rely on the scope of the try/catch).

 Either way with an explicit block there is less chance one of us adds code
 later that keeps the lock longer than necessary :)

 > '''general'''
 >
 > - I made some minor style fixes.
 >

 thnx

 > '''auth_messages.mes'''
 >
 > - don't we need to be more specific like clarifying it's about data
 >   source configuration in the short description?
 > {{{
 > % AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR Error in
 configuration: %1
 > }}}
 >   Or would that be redundant because it should be pretty obvious from
 >   the message ID?  Same for the other two messages.
 >

 this is something i have been wondering in general, actually; essentially
 it would mean all info is there twice, which would 1. make the
 'reasonably-readable-identifiers' redundant indeed, and 2. make every
 single log message very long (note that this is already a full line even
 before the argument, so in this case it's already pretty long and saving a
 few words doesn't gain us much).

 OTOH, part of the problem is that our log message identifiers get very
 long, so maybe we should abbreviate more there and put the full names/info
 in the description. For now I've added 'data source' there, but this is
 probably something we should discuss in a wider scope.

 > - Is "from before the reconfigure command" grammatically correct?  (I
 >   can understand what it means though)
 > {{{
 > an error. The system is still running with the data sources from before
 > the reconfigure command, and the configuration data needs to be checked.
 > }}}
 >

 I *think* it is correct, but it may be a tiny bit unclear; how does this
 sound:
 {{{
 The system is still running with the data sources that were
 previously configured (i.e. as if the configuration has not changed),
 but the configuration data needs to be checked.
 }}}

 (I now wonder if I should also add that if this is not fixed, upon a
 restart, the system would revert to defaults)

 > '''datasrc_clients_mgr.h'''
 >
 > - perhaps a matter of taste, but you can generally omit "isc::" in
 >   namespace specifications as it's already in the isc namespace, e.g.:
 > {{{#!cpp
 >     isc::datasrc::DataSrcClientListsPtr clients_map_;
 > }}}
 >   at least this was intentionally omitted (for brevity) in other part
 >   of this file.
 >

 ok done

 > - regarding the TODO, I think this can be a brief one.  and in any
 >   case I'd fold the line (it's currently long)
 > {{{#!cpp
 >     RECONFIGURE,  ///< Reconfigure the datasource client lists,
 configuration argument (TODO: describe here?)
 > }}}
 >

 updated, added short description

 > - maybe some short description here?  at least clarify map_mutex_ is
 >   to protect clients_map_.
 > {{{#!cpp
 >     isc::datasrc::DataSrcClientListsPtr clients_map_;
 >     MutexType map_mutex_;
 > }}}
 >

 added

 > - I guess it's probably better to create an empty map in the
 >   `DataSrcClientsMgrBase` constructor, instead of starting from a NULL
 >   pointer.  That way we won't have to worry about the case it's a NULL
 >   every time we use it.  (Note that this will require changes to some
 >   tests).
 >

 as you posted later, if you did this in 2211, i'll leave it as is here
 (cherry-picking will probably result in conflicts)

 > - `doReconfigure(): related to the previous point, we might want to
 >   check new_client_map isn't NULL.  This should be ensured by
 >   configureDataSource() which also belongs to auth, and in that sense
 >   we could even assert() it.  throwing on NULL would also be okay
 >   though.
 >

 er, moot now, right (or at least, after #2211)?

 > - in `doReconfigure()`, you can use shared_ptr::swap:
 > {{{#!cpp
 >                 //std::swap(new_clients_map, *clients_map_);
 >                 new_clients_map.swap(*clients_map_);
 > }}}
 >   (in this particular case there wouldn't be much difference in
 >   effect, though).
 >

 ok changed

 > - is `data::TypeError` the only possible error from the datasrc layer
 >   (propagated via configureDataSource) for configuration error?
 >             } catch (const isc::data::TypeError& type_error) {
 >                 LOG_ERROR(auth_logger,
 >
 AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR).
 >                     arg(type_error.what());
 >

 Grm. No, it is the only one raised by the 'toplevel' call, but below it
 there are several isc::Exceptions that can be raised, some of which about
 contents, some about internal errors. Unfortunately these are not in any
 decent hierarchy to make out which type it is without listing them *all*,
 which would mean either keeping around the type and error message, or
 putting in X log messages. The type-agnostic nature of
 DataSourceReconfigureGeneric template also makes it a bad place to raise a
 ClientList-specific error (ConfigurationError, as defined in
 client_list.h).

 As a compromise, I've added one more catch and updated the existing one,
 thereby making it four cases instead of 3:
 - ConfigurationError (from clientlist::configure() itself) (user error)
 - type-error (the top-level format is not a map of lists) (bug, but check)
 - any other error (std::exception, most of these are raised by
 clientlist::configure(), but they can be raised by the clients themselves
 too) (probably bug, but check)
 - any other other error (...) (certainly bug)

 i.e. the format and the configurationerror are separately handled. Any
 other one is caught by isc::exception (since datasources can by definition
 throw anything anyway)

 also added a test so it covers the new case as well (now that it are
 handled with a different catch)

 > - full data source reconfiguration is a big event, so I think it
 >   should be logged for non error cases too, maybe both before
 >   and after, at the INFO level.
 >

 added

 > '''datasrc/client_list.h'''
 >
 > Just a comment: I don't object to defining `DataSrcClientListsPtr`
 > here instead of `AuthSrv`.  At least the concept isn't specific to the
 > auth server implementation, but I'm also not sure if it's of general
 > interest.  We could consider many kinds of variations like this, such
 > as a set of client lists, maps from strings to lists, etc, depending
 > on the purpose of the application.  Clearly we wouldn't like to define
 > a shortcut typedef for each and every one of such things.
 >

 True, but my real reason was mostly that within Auth it is currently used
 in quite a lot of places, all of which had to include auth_srv.h now, and
 most of them already included client_list.h anyway.

 Perhaps, after all tickets have been done, it turns out the usage is once
 again quite localized. In which case we can certainly reconsider.

 > Also, I'm afraid `DataSrcClientListsPtr` might not really be a good
 > name (disclaimer: it was me who named it), especially if it's defined
 > in a more public space.  It doesn't clarify it's a (pointer to) map,
 > and the sense of 's' in 'Lists' could be easily overlooked.  I
 > actually didn't like that name, but I couldn't come up with a clearer
 > but not too verbose one (even this one is quite long, and
 > `DataSrcClientListMapPtr` sounded awful).  So, if you have a better
 > idea of naming it, please also make that change at this opportunity.

 Renamed it to ClientListMapPtr. Still not ideal, but as discussed it
 should be a reasonable compromise.

 Renaming done in separate commit, as it touches quite a few files.

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