BIND 10 #2379: add python wrapper of datasrc::ZoneLoader

[BIND 10 Development]

#2379: add python wrapper of datasrc::ZoneLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20121218
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  10            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei
 * totalhours:  0 => 10


Comment:

 Replying to [comment:6 jinmei]:
 > '''pydnspp.cc'''
 >
 > I'd like to make it a bit more safer at this opportunity using
 > `PyObjectContainer` and `installToModule()`.  See, e.g.
 > src/lib/python/isc/util/cio/socketsession_python.cc
 >

 Done.

 While I was at it, I noticed the type/class initialization code was quite
 inconsistent (different cast types, return value of addToModule not always
 checked, etc), so I also added an initClass() function to pydnspp_common,
 and used that for all types in pydnspp.cc

 If that approach looks ok, we should probably add it to the general python
 util class as well.

 > '''client_python.h'''
 >
 > - I'd like to see some documentation for
 `PyDataSourceClient_ToDataSourceClient()`.
 >

 added

 > '''client_python.cc'''
 >
 > - the what() message of the exception seems incorrect (it's not `Name`):
 > {{{#!cpp
 >     if (client_obj == NULL) {
 >         isc_throw(PyCPPWrapperException,
 >                   "obj argument NULL in Name PyObject conversion");
 >     }
 > }}}

 fixed

 > - not a big deal in this context, but I suspect technically `client`
 >   cannot be a const pointer, because we are effectively going to allow
 >   this object to be modified.  So, this matches the reality better:
 > {{{#!cpp
 >     s_DataSourceClient* client =
 static_cast<s_DataSourceClient*>(client_obj);
 > }}}
 >

 fixed

 > '''zone_loader_python.cc'''
 >
 > - `ZoneLoader_init`: it doesn't seem to be exception safe regarding
 >   the reference to po_target_client.  I suspect it should be protected
 >   using `PyObjectContainer`.  Also writing in that case (that checks
 >   the refcount of the target client) is a good idea.

 Done, though the incref/set could probably just go at the end in this
 specific scenario. But I may be misunderstanding the use of the container
 here :)

 Also added a check that should trigger too many DECREFS. But have not
 really thought of a good way to test reference leaks.

 Not sure what that last line means.

 > - With this code, the error string may not always be helpful:
 >   It would say "TypeError: must be str, not int", but it may not
 >   necessarily have to be str (can be another client).  So in such
 >   cases I'd add our own error string, saying some error in parameters
 >   and what are acceptable.

 fixed

 > - is it okay not to gain a reference to source_client?

 No, added

 > - `ZoneLoader_loadIncremental`: maybe a matter of taste, but we don't
 >   need `complete`.
 >

 removed

 > '''zone_loader_test.py'''
 >
 > - some resources don't seem to be cleaned up cleanly in case a test
 >   fails.  for example, if we do this in test_bad_file
 > {{{#!python
 >         self.check_zone_soa(ORIG_SOA_TXT)
 >         loader = isc.datasrc.ZoneLoader(self.client, self.test_name,
 >                                         'no such file')
 >         loader.load()
 >         ...
 > }}}
 >   some other tests also fail with "database is locked".

 Hmz, initially I thought this may be a refcounting problem (and it looks
 like it is, in a way, but not what I was expecting).

 Turns out, when there is an exception, local variables aren't actually
 completely destroyed until all tests finish (exception context gets a
 reference too). Normally, this isn't much of a problem, but in the case of
 the loader, we can only have 1 instance at a time (since it locks the db).

 But it certainly messes up normal C++-style RAII usage...

 There are several workarounds;

 - put the loader as a member in the test class, and set it to None in
 tearDown(); (tbh, i was surprised that that worked)

 - try/catch every single test that creates one (then set loader to None
 and re-raise), or never put the loader in a local variable (but this is
 not always possible for extensive tests)

 - destroy the internal cppobj for any exception (...) and disallow further
 operations (so the db gets unlocked). Perhaps even in the c++ class itself
 (with the current API i don't think there's ever a case where you want to
 or even can continue to use the loader instance after a failure, but
 perhaps that may change). This would probably be safest in any
 environment, but it'd also be quite unexpected behaviour.

 - make a much smarter loader class :p (that can share db access). meh.

 None of these seem nice though... I think I dislike option 1 the least.
 Therefore I'll do that but as the last commit, so if you have a better
 idea, I can revert it.

 > - not looking into the code, but does this case "no such zone in
 >   source"?
 > {{{#!python
 >     def test_no_such_zone_in_source(self):
 >         source_client = isc.datasrc.DataSourceClient('sqlite3',
 >
 DB_SOURCE_CLIENT_CONFIG)
 >         self.assertRaises(isc.datasrc.Error, isc.datasrc.ZoneLoader,
 >                           self.client, isc.dns.Name("unknownzone"),
 >                           source_client)
 > }}}
 >   maybe it happens to be so, but to be sure it's better to use an
 >   existing zone for the destination but not in the source (or do we do
 >   this?  If so, please clarify that in the comment).

 Ack, it actually didn't. Updated test to do so, and to make sure it does.

 > - the tests seem to be pretty different from the C++ counterpart.
 >   does the Python version have the same level of coverage?
 >

 Not entirely, and IMO it shouldn't have to; the important thing here is
 that we test the wrapper code, not necessarily the underlying c++ classes
 (they have their own tests); there is a lot of overlap (they are
 essentially doing the same thing), but I don't think we need to repeat
 every single test. Of course I could be missing some here.

 > '''zone_loader_inc.cc'''
 >
 > The same comment as that for #2542 applies.  I'm attaching the C++
 > file that the auto-generation tool produces for reference (if we want
 > to use it we still normally need to edit it a bit).

 Updated it a bit and replaced the original.

 Two comments if you're cleaning up your script:
 - 'void' tends to be None in python (as a return value for methods)
 - the \note for loadIncremental was omitted

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