Opened 9 years ago

Closed 9 years ago

#225 closed defect (fixed)

b10-auth fails if ran from source tree

Reported by: jreed Owned by: jelte
Priority: medium Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description (last modified by jreed)

 [AuthSrv] error: Cannot open SQLite database file: /home/jreed/tmp/bind10-devel-20100602--INSTALL/var/bind10-devel/zone.sqlite3

This is because it wants to place the file under the install destination and if that is not created yet, it fails. If running in source tree, it should use a location that exists (maybe some temp directory in the builddir).

Also the [AuthSrv] maybe should be [b10-auth]

Subtickets

Attachments (2)

bind10_database_file.patch (8.6 KB) - added by jelte 9 years ago.
bind10_database_file_2.patch (8.7 KB) - added by jelte 9 years ago.
second version of patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by jreed

  • Description modified (diff)

comment:2 Changed 9 years ago by zhanglikun

Once this ticket is fixed, the code in line 444 of bin/xfrin/xfrin.py.in also needs to be updated. since the default database file is '@@LOCALSTATEDIR@@/@PACKAGE@/zone.sqlite3'

comment:3 Changed 9 years ago by shane

  • Owner set to jelte
  • Status changed from new to reviewing

Jelte, this is another in a set of related tickets that I hope you can review. Thanks!

comment:4 follow-up: Changed 9 years ago by jelte

There wasn't anything to review yet, so I did the work instead, and I went a little bit further than just this one thing;

b10-auth now uses a zone database file in the build directory if run from source.

Xfrin and Xfrout now use the actual file as specified in auth's configuration (as opposed to something configured there or specified on the command). So if you change it they will magically use the new value.

I removed db_file from xfrout's spec, but left the command option for xfrin there (it'll default to whatever is configured)

Attaching path, not sure whether this needed a full branch, if so please say so and i'll apply it there ;)

Changed 9 years ago by jelte

comment:5 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned

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

  • I don't like to see hardcoded magic values such as "src/bin/auth". understanding this is probably a short-to-middle term hack, I can live with that, but if possible please consider centralizing the definition of these things
  • Please add tests for xfrin
  • xfrout: shouldn't we perform remove_remote_config() somewhere?
  • UnixSockServer.get_db_file() do we really need to acquire a lock here? with your change it seems to eliminate the possibility of race. besides, since get_remote_config_value() can be time consuming (right?), this can be a giant lock.
  • changelog: s/feature/func/ (unless you tried to rename func to feature, but that should go to a different ticket anyway)

comment:7 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jelte

comment:8 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by jelte

Replying to jinmei:

  • I don't like to see hardcoded magic values such as "src/bin/auth". understanding this is probably a short-to-middle term hack, I can live with that, but if possible please consider centralizing the definition of these things

Yes, let's make a separate ticket for that, I have some (incomplete) ideas on how to do this better, partly related to having access to configuration of non-running modules

  • Please add tests for xfrin

Any idea on how? The tests use a test-specific database file, so I think this is more integration-testing than unit testing. Something with cucumber perhaps?

  • xfrout: shouldn't we perform remove_remote_config() somewhere?

In principle it shouldn't be necessary, it should be handled by the cleanup routines of ccsession if not called manually. Though I now see what needs to be done is missing there (a call to group_unsubscribe), which is only partly related to this ticket, creating a new ticket for that too.

  • UnixSockServer.get_db_file() do we really need to acquire a lock here? with your change it seems to eliminate the possibility of race. besides, since get_remote_config_value() can be time consuming (right?), this can be a giant lock.

Actually it doesn't take much time, 'remote config' follows a push-mechanism, so the pain is there on config changes, not on config access, and this is simply a dict lookup. You're right right about not needing a lock here, but now that you mention it, we might need them in ccsession itself...

  • changelog: s/feature/func/ (unless you tried to rename func to feature, but that should go to a different ticket anyway)

ack.

comment:9 in reply to: ↑ 8 Changed 9 years ago by jinmei

Replying to jelte:

Responding to non trivial points only. Others are okay.

  • Please add tests for xfrin

Any idea on how? The tests use a test-specific database file, so I think this is more integration-testing than unit testing. Something with cucumber perhaps?

When I first saw the patch, I thought we could take a similar approach as xfrout_test, that is, defining a mock CCSession class for testing and letting it respond to get_remote_config_value().

But this may lead to a relatively larger refactoring. If so, I'm okay with deferring this to a separate ticket as long as it's addressed soon.

  • UnixSockServer.get_db_file() do we really need to acquire a lock here? with your change it seems to eliminate the possibility of race. besides, since get_remote_config_value() can be time consuming (right?), this can be a giant lock.

Actually it doesn't take much time, 'remote config' follows a push-mechanism, so the pain is there on config changes, not on config access, and this is simply a dict lookup. You're right right about not needing a lock here, but now that you mention it, we might need them in ccsession itself...

Ah...okay, I see the problem, I think.

Inter module mutex will lead to a nightmare, we should avoid that.

I guess a better solution is to have the main thread manage both the
remote and local config, and worker threads always refer to copies
closed in the xfrout module. Do do this, we could do, e.g.

  • add a hook to ModuleCCSession so that the calling module of check_command() can get a callback when a remote module is updated, or
  • make a communication channel (named pipe or something) between the main thread and its children, and pass any dynamic config parameters via that channel. This is expensive, but I suspect we'll need this type of channel anyway for faster shutdown (I guess the same problem I pointed out for xfrin exists), or
  • let the worker threads subscribe to the channel of the remote module (but in that case we may rather think about using separate processes than threads)

Anyway, this race condition is currently a hypothetical issue (python cannot run multiple threads on multiple processors/cores if I understand it correctly), and solutions won't be easy. So I think we can leave it to a separate ticket (maybe a backlog item). For now, it should be okay just to leave a comment about this problem.

comment:10 follow-up: Changed 9 years ago by zhanglikun

self._db_file = new_config.get('db_file') may make xfrout crash, If there isn't 'db_file' in new_config.

comment:11 in reply to: ↑ 10 Changed 9 years ago by zhanglikun

Replying to zhanglikun:

self._db_file = new_config.get('db_file') may make xfrout crash, If there isn't 'db_file' in new_config.

Oh, sorry, should not crash, but self._db_file will be set to None if there isn't 'db_file' in new_config.

comment:12 follow-up: Changed 9 years ago by jelte

jinmei: ok remote_config ticket created (#252), and removed the lock/release lines which aren't useful in that specific function.

Likun: I don't understand, the line you mentioned is removed by the patch (and since database_file has a default, getting it from the configuration should *always* return a value).

I think this one is ready for merge

comment:13 in reply to: ↑ 12 Changed 9 years ago by zhanglikun

Likun: I don't understand, the line you mentioned is removed by the patch (and since database_file has a default, getting it from the configuration should *always* return a value).

Sorry I made a mistake, the deleted line was treated as new added one.

Changed 9 years ago by jelte

second version of patch

comment:14 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Most of your comments have resulted in the creation of new tickets, so the changes aren't too much (feature->func and removal of the unhelpful lock() lines)

comment:15 in reply to: ↑ 14 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

Replying to jelte:

Most of your comments have resulted in the creation of new tickets, so the changes aren't too much (feature->func and removal of the unhelpful lock() lines)

Okay. Go ahead.

comment:16 Changed 9 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to trunk, r2325, closing ticket.

Note: See TracTickets for help on using tickets.