Opened 7 years ago

Closed 7 years ago

#2964 closed defect (fixed)

xfrin should use general datasource configuration, not Auth/database_file

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130611
Component: xfrin Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 18.49 Internal?: no

Description (last modified by jinmei)

subject should say all. we'll need this by the time
we support non-SQLite3 based data source, so it's probably
the time to schedule it.

Subtickets

Change History (15)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 5

comment:3 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130611

comment:4 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:5 follow-up: Changed 7 years ago by jinmei

trac2964 is ready for review.

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 also introduced a new helper module under the isc.server_common
package to handle a set of data source client lists. Something like
this will be necessary for all variants of this task, so it should
make sense to introduce a common module.

Except these, the changes should be quite straightforward:

  • commits up to 159d0dd are initial cleanup and preparation refactoring. Basically there shouldn't be behavior change in effect. I think these commits are better reviewed separately.
  • 43c9101 and 7a0ffbd are related to the updates to b10-loadzone.
  • 1c50519 is the newly added helper class under server_common.
  • the rest of the branch implements the goal of this ticket on top of the above setup. These changes are probably easy to review as a single diff.

Proposed changelog:

623.?	[bug]*		jinmei
	b10-xfrin/b10-loadzone: b10-xfrin now refers to the unified
	"data_sources" module configuration instead of almost-deprecated
	the Auth/database_file configuration (Note: zonemgr still uses the
	latter, so a secondary server would still need it for the moment).
	Due to this change, b10-xfrin does not auto-generate an initial
	zone for the very first transfer anymore; b10-loadzone has been
	extended with a new -e option for the initial setup.
	(Trac #2946, git TBD)

comment:6 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:7 Changed 7 years ago by jinmei

(I've noticed distcheck failed so I made one other commit)

comment:8 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:9 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to 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:

                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)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jinmei

Thanks for the review.

Replying to 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:

        # 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:

                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.

comment:11 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 Changed 7 years ago by jinmei

note to myself: the resolution of this ticket will make #2663 moot.
we can/should close it with this ticket.

comment:13 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 0.83

Hello

Replying to jinmei:

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.

OK, I'll bring it up on the list.

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:

If you know the internal implementation, then they are indeed very similar. But they _could_ be very different, one could be eliminated much sooner if there was some validation according to the spec file.

I don't insist on it the hard way, but I believe adding such test would be a good thing and not much work.

I believe it can be merged with or without such test.

comment:14 in reply to: ↑ 13 Changed 7 years ago by jinmei

Replying to vorner:

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:

If you know the internal implementation, then they are indeed very similar. But they _could_ be very different, one could be eliminated much sooner if there was some validation according to the spec file.

I don't insist on it the hard way, but I believe adding such test would be a good thing and not much work.

I believe it can be merged with or without such test.

Okay, on thinking about it again, I chose not to add another test this
time. Generally, adding more tests is a good thing, but conciseness
is also important, and in this case the point of possible behavior
difference is quite far from the tested code, so I preferred the latter.

Now merge done, closing.

comment:15 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.83 to 18.49
Note: See TracTickets for help on using tickets.