support "load zone" command in the configurator thread

[jinmei]

(We probably don't have time to do this for the September release)

A subtask of #2201, depend on #2202, #2203, #2205, #2209

This task extends the newly introduced "data source configurator"
thread (#2205) to support background zone reloading.

It will be based on the current content of LoadZoneCommand::exec(),
but works as follows:

• first call ConfigurableClientList::getCacheZoneUpdater() introduced in #2209
• then call updater->load()
• then acquire the lock introduced in #2202
• then call updater->install()
• then release the lock
• then call updater->cleanup().

Note that updater operations can throw.

[jinmei]

I've pushed some initial work for this task at branch trac2212.

It added the "LODAZONE" command for the loader thread, and ported
the current command handler code in a mostly straightforward way.
corresponding tests in command_unittest were also ported.

When #2209 is done, we'll update the doLoadZone() internal
as described in this ticket based on (snapshot of) #2209, and
adjust tests.

I'm going to release this ticket now. Please feel free to take it
over if #2209 is completed soon.

[jinmei]

This task is ready for review at the trac2212-2 branch (note: not
"trac2212", which has also been pushed).

I first merged a snapshot of trac2209 (03499b1), which should be
ignored.

The main changes (in datasrc_clients_mgr.h) should be pretty
straightforward. For tests, I mainly ported current tests using
the old interface in command_unittest. Porting them is mostly
straightforward, but see commit logs starting with "ported command
test".

There's one thing I may have to update when trac2209 is merged:

    case datasrc::ConfigurableClientList::ZONE_RELOADED: // XXX misleading name
        assert(writerpair.second);
        return (writerpair.second);

IMO this result code is misleading, and I pointed it out in my review
for trac2209. If we agree on changing this, we'll need to update this
code too. But this should be a trivial adjustment and I believe can
be done on merge of this branch.

[jelte]

there is a compilation issue in getZoneWriter() (datasrc_clients_mgr.h); some compilers still need a return statement;

I suggest adding something like

    // all cases above should either return or throw, but some compilers
    // still need a return statement
    return (boost::shared_ptr<datasrc::memory::ZoneWriter>());

at the end of getZoneWriter()

auth_messages.mes:

I have several small fixes for the COMMAND_ERROR log message, I'd propose making it this text:

The separate thread for maintaining data source clients failed to complete a
command given by the main thread.  In most cases this is some kind of
configuration or temporary error such as an attempt to load a non-existent
zone or a temporary DB connection failure.  So the event is just logged and
the thread keeps running.  In some rare cases, however, this may indicate an
internal bug and it may be better to restart the entire program, so the log
message should be carefully examined.

The death tests recently got an additional check (so that valgrind doesn't fail on them, at least if valgrind.h is available), so any code that does EXPECT_DEATH should include <util/unittests/check_valgrind.h>, and put

    if (!isc::util::unittests::runningOnValgrind()) {
    }

around the EXPECT_DEATH(_IF_SUPPORTED) macros
(as an example, see buffer_unittest.c)

(note that we are considering removing this again and provide an environment option to simply skip all death tests, as this is really the only place valgrind can't handle, but until that is decided this is the best we have for valgrind not to fail on death tests)

Oh, and FYI, maybe not entirely related to this branch (but it is about new code here), I plan to make a slight change in doLoadZone(); and put the 'default to IN if class not provided' code there, in trac2213; it is easier (both in code and in cycles) to do that check here, instead of in the code sending the thread command, as that code is dealing with a const element, and would need to recreate the 'entire' args variable (granted, it isn't that big). Other checks will go in the code i'll add in 2213, but it can only check, not modify data (without reconstructing 'everything').

[jinmei]

Thanks for the review.

Replying to jelte:

there is a compilation issue in getZoneWriter() (datasrc_clients_mgr.h); some compilers still need a return statement;

I suggest adding something like

    // all cases above should either return or throw, but some compilers
    // still need a return statement
    return (boost::shared_ptr<datasrc::memory::ZoneWriter>());

at the end of getZoneWriter()

Ah, okay, added.

auth_messages.mes:

I have several small fixes for the COMMAND_ERROR log message, I'd propose making it this text:

Thanks, adopted.

The death tests recently got an additional check (so that valgrind doesn't fail on them, at least if valgrind.h is available), so any code that does EXPECT_DEATH should include <util/unittests/check_valgrind.h>, and put

    if (!isc::util::unittests::runningOnValgrind()) {
    }

around the EXPECT_DEATH(_IF_SUPPORTED) macros
(as an example, see buffer_unittest.c)

Okay, done.

Oh, and FYI, maybe not entirely related to this branch (but it is about new code here), I plan to make a slight change in doLoadZone(); and put the 'default to IN if class not provided' code there, in trac2213; it is easier (both in code and in cycles) to do that check here, instead of in the code sending the thread command, as that code is dealing with a const element, and would need to recreate the 'entire' args variable (granted, it isn't that big). Other checks will go in the code i'll add in 2213, but it can only check, not modify data (without reconstructing 'everything').

Hmm, I'm okay with leaving the default handling to the builder per se,
but I'd like to avoid hardcoding the value. The default is given in
the spec, and the implementation should retrieve the default from it.
But to do so, it needs to be able to access ModuleCCSession, and
at least right now the builder doesn't have the access (and I'd like
to keep it that way so that the builder is as independent as
possible). I know it requires the reconstructing the message, but in
this case it's a simple name, class pair, so I think it's not a big
overhead for the sender.

[jinmei]

Replying to jinmei:

There's one thing I may have to update when trac2209 is merged:

    case datasrc::ConfigurableClientList::ZONE_RELOADED: // XXX misleading name
        assert(writerpair.second);
        return (writerpair.second);

IMO this result code is misleading, and I pointed it out in my review
for trac2209. If we agree on changing this, we'll need to update this
code too. But this should be a trivial adjustment and I believe can
be done on merge of this branch.

Based on the latest status of #2209, I'll change this part as follows:

-    case datasrc::ConfigurableClientList::ZONE_RELOADED: // XXX misleading name
+    case datasrc::ConfigurableClientList::ZONE_SUCCESS:

(But I won't merge #2209 in this branch just for this, in order to
keep it easier to track. I'll just make this change on final merge -
if I forget it, the compiler will soon notice that).

[jelte]

Changes look fine, and this branch is good for merge, I'll see about the default issue in 2213

[jinmei]

Replying to jelte:

Changes look fine, and this branch is good for merge, I'll see about the default issue in 2213

Okay, merge done, closing the ticket.