BIND 10 #1292: Xfrin retransfer shared object "sqlite3_ds.so" not found

[BIND 10 Development]

#1292: Xfrin retransfer shared object "sqlite3_ds.so" not found
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20111206
  critical                           |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  High   |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  5      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:25 jinmei]:
 > Replying to [comment:24 jelte]:
 >
 > First off, distcheck failed:
 >
 > {{{
 > ERROR: files left in build directory after distclean:
 > ./src/lib/datasrc/datasrc_config.h
 > }}}
 >

 fixed

 > > > > BTW, I have not checked, but if this works, i suspect it solves
 several other related tickets as well (#1385 and #1421, maybe more)
 > > >
 > > > For #1421, probably.  I suspect #1385 is a separate issue.
 > >
 > > he hasn't answered my question there yet, but i have seen this problem
 > > too; when i had an older installed version that wasn't
 > > binary-compatible with recent changes. Exact same symptoms, caused by
 > > dlopen picking the wrong one. So if that is the case for him too, the
 > > root cause is of course different (we need version check), but the
 > > system should fall in that trap a lot less (theoretically, never,
 > > even, unless you start mixing versions).
 >
 > I'm still not sure whether dlopen() helps #1385 - could it be an
 > incompatible version of our own loadable modules?  Anyway, that's not
 > a showstopper issue for this ticket.  As for #1385 itself maybe you
 > should poke Kevin on jabber or give the ticket back to him.
 >

 Yes, incompatible loaded libraries (at least in the case where I ran
 into it), needs some form of version check, but it also needs to
 (be able to) pick the right file in the first place (otherwise you can
 only detect something is wrong, not do anything about it).

 > > > About the branch:
 > > >
 >
 > I've not fully thought about it either, but I can imagine something
 > like this:
 > - We (eventually) introduce more generic data source configuration
 >   where we can specify the default module path
 > - We specify the installed version of module path for the installed
 >   version of the spec file for the data source config "module"
 > - For in-source run environment, we might either provide a hacked
 >   version of spec file (if possible) or provide a template b10-config
 >   file that overrides the module path config to the in-source path
 > - For unit tests, we can probably provide a utility module/helper
 >   library that tweaks the path and have tests that need to use
 >   data sources import/call it
 >
 > This is not a baked idea and may or may not be feasible, but in
 > general if we can replace this kind of harcode in the core
 > module/program with configuration and helper modules specific for
 > tests (while reasonably avoiding duplicate setups for tests), I'd
 > prefer that.
 >

 (this has now been kind of superseded by the meeting discussion, but
 leaving it in for reference)

 Hmm, differing spec-files may indeed be the best approach here. One
 open question could be whether it should be possible to work with
 'staging setups', where the target of 'make install' is not the final
 location on the system (not saying that it should, just wondering).

 This could then also be used for things like the current
 Auth/database_file problem (where some environment variables even
 cause failing tests right now).

 Also not fully thought out yet, but possibly a nice approach could be
 to just modify them upon installation (or recreate them with
 install-values at that point), so there's no confusion in-tree.

 I think we should make removing environment variables a goal (or at
 least reduce them to the bare minimum). A slightly different,
 but related issue is the current run-scripts, and generated python
 files.

 So I propose to keep the approach in the branch for now, then
 - identify the environment variables we use
 - discuss a final approach to remove or consolidate them
 - implement that approach
 And the same for the generated files and run_scripts (perhaps even in
 one go, though that might be a bit much).

 I'll suggest this as a topic for the meeting today.

 > Anyway, for the purpose of this ticket I'm okay with the current
 > approach (even if the above idea makes sense it would require generic
 > data source config framework, which would be way beyond the scope for
 > this ticket).
 >

 ack

 >
 > Ah, yes, what I meant was libexec rather than localstatedir (maybe I
 > incorrectly assumed the latter would expand to libexec when I made the
 > comment).  The revised code on this point looks okay.
 >
 > And this means the existing modules could now be cleaned up, and to
 > note that I think this ticket needs a changelog entry.
 >

 See below for a proposal for changelog.

 > > > '''bind10_src.py.in'''
 > > >
 > >
 > > Actually I prefer doing it here, so we can show that it works.
 >
 > Okay, then for the next step we should at least remove start_xfrin/out
 > and update bob.spec and isc.bind10.special_component accordingly.
 >
 > I think that changes regarding this should also be noted in the
 > changelog.
 >

 Of course, done, should now be started as dispensable normal
 components.

 > '''factory.cc'''
 >
 > > > - Don't we need a test for getDataSourceLibFile?
 > >
 > > did we decide on a good approach for testing
 > > anonymous-namespace-functions? We can of course make it a function in
 >
 > I don't even remember whether we have ever discussed this, but in
 > general I don't think we can directly test things in an unnamed (aka
 > "anonymous") namespace.  In this case I can think of two
 > possibilities:
 >
 > - define it in something like isc::datasrc::detail (with an explicit
 >   note that it's only for tests) and test the function directly.
 > - test the behavior indirectly, e.g., by passing a non existent path
 >   to DataSourceClientContainer and check the resulting what() string
 >   on exception.
 >

 I personally am not fond of changing or adding interfaces just for
 testing (in general, of course there are always exceptions), so adding
 a specialized namespace for this would not be my favourite option. And
 neither would another way we could go; introduce a member variable
 that stores the file that has been loaded.

 But checking the error message is a good idea, added a test. Do you
 know if there are dlopen() implementations that form different errors?
 (if so i'll have to modify the test a bit to first discover that error,
 or do some form of string.startswith check, but the error a direct
 EXPECT_EQ() gives is much nicer.

 > '''datasrc/tests/Makefile.am'''
 >
 > > yes, sorry, forgot about this in my hurry to get you to take a look at
 > > it :p Moved the if check and removed the old part.
 >
 > Okay, then I have some additional comments:
 >
 > - is this about "the libraries" or "modules"?
 > {{{
 > # For the factory unit tests, we need to specify that we want
 > # the libraries from the build tree, ...
 > }}}

 Ack changed (into backend libraries, as per the call just now)

 > - also, maybe we should copy the reason why we limit this to
 >   !USE_STATIC_LINK from the now-removed comment:
 > {{{
 > # This test uses dynamically loadable module.  It will cause various
 > # troubles with static link such as "missing" symbols in the static
 object
 > # for the module.  As a workaround we disable this particualr test
 > # in this case.
 > }}}
 >

 ok, readded

 > Changes I've not explicitly commented on above are okay to me.

 Thanks.

 Finally, here's the changelog proposal (it's a pretty long one for a
 relatively small change :p)

 [build?] jelte

 The DataSourceClient class that dynamically loads datasource backend
 libraries no longer provides just a .so file name to its call to
 dlopen(), but passes it an absolute path. This means that it is no
 longer an system implementation detail that depends on
 [DY]LD_LIBRARY_PATH which file is chosen, should there be multiple
 options (for instance, when test-running a new build while a different
 version is installed).
 These loadable modules are also no longer installed in the default
 library path, but in a subdirectory of the libexec directory of the
 target ($prefix/libexec/[version]/backends).
 This also removes the need to handle b10-xfin and b10-xfrout as
 'special' hardcoded components, and they are now started as regular
 components as dictated by the configuration of the boss process.

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