BIND 10 #2964: xfrin should use general datasource configuration, not Auth/database_file

[BIND 10 Development]

#2964: xfrin should use general datasource configuration, not Auth/database_file
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  xfrin         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130611
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:9 vorner]:

 > I think this approach is sane (at least in the current state of Bind10).
 However, I'd like to bring up several things:
 >  * The name `b10-loadzone` is probably no longer correct, because with
 `-e`, it doesn't load anything. Maybe we should rename it to something
 like `b10-zoneutil` (similar to current dbutil).
 >  * The descriptions of the flag all seem to concentrate on the fact that
 it takes a zone and erases its content. I believe the primary intention is
 to create an empty zone, not erasing content of one. I'm not against the
 erasure of existing zone with `-e`, as it seems consistent, but the
 documentation probably should concentrate more on the primary goal and not
 mentioning it just as a consequence of the behaviour, even if it is in
 some sense.
 >  * This is not a task for this ticket probably, but if we have `-e` now,
 it would be nice to have `-d` too, for deletion of a zone (not just
 erasing the content).
 >  * It would be comfortable to have some way to do this all from one
 place. Currently, to add a zone, I need to configure the remote server and
 call external script to create the empty zone, then transfer it. This
 seems like a lot to ask for. Of course, this is not for this ticket
 either, but unless you disagree with that, I'd like to take it to the
 list.

 I generally agree on these.  In fact, as I added a note the guide, I
 thought this should be revised in the context of comprehensive zone
 management/configuration framework.  At that point, all the user have
 to do configure some zone as a secondary on some data source.  Then
 the framework will take care of creating an empty zone in the data
 source, etc.

 Regarding the changes to loadzone:
 - for now I didn't change the program name as this will still not be
   the final solution.
 - I updated the documentation on the role of `-e` in the branch.

 > Regarding the code:
 >
 > There's a race condition about `DataSrcClientMgr.reconfigure` of a kind.

 Good point.  I've updated the documentation.

 > All the wrong configs about the `DataSrcClientMgr` seem to be violating
 the specs. Should there be a test with a valid spec (for example with non-
 existent type of data source)?

 In practice, this case shouldn't be so different from errors like
 nonexistent type of data source for higher level Python programs:

 {{{#!python
         # Another type of invalid configuration: exception would come from
         # the C++ wrapper.
         self.assertRaises(ConfigError,
                           self.__mgr.reconfigure, {"classes": {"IN": 42}})
 }}}

 as they are caught in the C++ wrapper and raised as `datasrc.Error` in
 the end.

 So I didn't add test cases for now.  If you strongly feel the need for
 it, however, I don't mind adding one.

 > This comment is misleading in some sense: „Note that this lock doesn't
 protect "updates" of the map content.  [...]

 Okay, I've tried to update it so it'll hopefully make more sense.

 > The second private should be protected here IMO:
 > {{{
 >         This is essentially private, but implemented as 'private' so
 tests
 >         can refer to it; other external use is prohibited.
 > }}}

 Good catch, corrected.

 > This seems slightly confusing:
 > {{{#!python
 >                 logger.error(XFRIN_NO_DATASRC,
 >                              format_zone_str(zone_name, rrclass))
 >                 return (1, 'no data source for transferring %s' %
 >                         format_zone_str(zone_name, rrclass))
 > }}}
 > The „No data source“ would sound like asking the admin to add one. I
 imagine users would bug-report that it says there's no data source, but
 they added one. May I suggest rephrasing it to something like „data source
 to transfer to not known“? The same problem is in the log message.

 Okay, updated basically as suggested.

 > This should be „belongs“:
 > {{{
 > but could not find a data source where the specified zone belong.
 > }}}

 Right, fixed.

 > I have trouble parsing this. What does convert mean? What is logged? I
 don't see any logging nearby.
 > {{{
 >         except isc.datasrc.Error as ex: # rare case error, convert (so
 logged)
 > }}}

 I've tried to detail it.

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