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
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:5 jinmei]:
 > There is one thing I didn't anticipate for this task, which made the
 > branch bigger and possibly controversial: with the generic data source
 > configuration, xfrin would not be able to create initial zone itself
 > any more (it wouldn't even know which data source it should use for
 > that).  There can be several ways to address this issue, but I've
 > personally been believing such an operation should be done outside of
 > xfrin, so I handled it by moving this feature from xfrin.  I did it by
 > extending b10-loadzone so it'll simply create an empty zone (it
 > already has an ability of creating a new zone).  We can discuss this
 > is a valid way to address the xfrin issue, but I believe the added
 > ability to loadzone can be useful for other purposes in any case.

 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.

 Regarding the code:

 There's a race condition about `DataSrcClientMgr.reconfigure` of a kind.
 If I would handle updates to configuration each from a new thread and two
 came close to each other in time, the older config could take longer to
 load and „win“ over the newer one. I don't think we want to use the class
 that way, but from the documentation it seems such mode should be
 supported. I propose to make clear that there must be only one reconfigure
 running at each given time.

 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)?

 This comment is misleading in some sense: „Note that this lock doesn't
 protect "updates" of the map content.  `__clients_map` shouldn't be
 accessed by class users directly.“ This is near the `__clients_map`
 variable, the fact that it should not be accessed is quite clearly
 demonstrated by the double underscore at the beginning. If someone makes
 the effort to manually mangle the name to get in from outside, that
 comment won't stop them anyway. And for the others, it makes people wonder
 if there's some „private“ variable that may be accessed.

 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.
 }}}

 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.

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

 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)
 }}}

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