BIND 10 #2178: Logging for Out-of-zone data

[BIND 10 Development]

#2178: Logging for Out-of-zone data
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jelte
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120904
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  3
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Responding to non-blocking points...

 Replying to [comment:7 jelte]:

 > > Things already addressed:
 > > - client_list_unittest.cc didn't compile for me because the result
 > >   class doesn't have operator<< (Does it work for a newer version of
 > >   gtest?)
 >
 > interesting, it certainly compiled here (but i had been wondering why
 the EQ(==) had been used in other places, since operator== obviously is
 defined); fyi, I have gtest 1.6.0

 Then it looks like a change in 1.6.  This issue has been a well known
 problem, and you can find many such compromise by grepping the
 keywords of 'EXPECT_TRUE' and '==' (I believe we leave comments about
 why it cannot be EXPECT_EQ in some of the cases).

 > > - I'd log the zone name and class in the unified form here:
 > > {{{#!cpp
 > >                     } catch (const isc::dns::MasterLoadError& mle) {
 > >                         LOG_ERROR(logger, DATASRC_MASTERLOAD_ERROR)
 > >                             .arg(mle.what());
 > > }}}
 >
 > I don't understand; the exception has zonename/class in it, or do you
 mean it should not be in the exception but in the log arguments?

 Ah, okay, I didn't chase it that deeper.  I'd still prefer including
 the zone info explicitly, because the current code depends on the
 specific what() format of `MasterLoadError` (that should be common to
 all 'throw' cases), and this class is far from the data source
 module.  But, as discussed, we'll revise the code around this more
 substantially pretty soon anyway, so I'm okay with keeping the current
 version.

 > > Bigger issues:
 > > - The current error handling in ConfigurableClientList::configure
 > >   seems quite ad hoc and policyless.  [...]
 >
 > Probably, but I do hope we can get something easier to edit than
 command/config in auth :)

 Hmm, I'm not sure how the auth config implementation is related in
 this context.  Out of curiosity, what kind of difficulty are you
 seeing?

 > > > - a potentially bigger problem that is shown by this ticket is that
 reconfigure() in the DataSourceConfigurator can throw, which it shouldn't,
 as it is used as a remote config callback; we should probably catch and
 log std::exception and maybe even (...) as well (in
 reconfigureInternal()).
 > >
 > > See above.  I agree we should revisit the error handling policy (and
 > > implementation).  But as for "it is used as a remote config callback",
 > > I guess it's part of another big problem of what we should do if
 > > remote config update fails in general.  Also, I think std or even
 > > unexpected type of exceptions should actually kill (maybe gracefully)
 > > the process just like in other cases.  We generally consider these
 > > an unrecoverable error, and rather than trying to deal with it, just
 > > let the process die and restart.
 >
 > well, there's two potential problems (which is why the set-remote-
 config-handler-call documentation specifies the handler should not throw);
 > - the handler cannot be trusted to immediately report errors back to the
 user since the module may not be running, and even if we had 'offline'
 config, we still wouldn't know which modules might use it
 > - more importantly, especially in this scenario, is that if the module
 is of kind 'needed', and there is an error in the config which kills the
 module, you can't start bind10 again and fix it; it will immediately exit
 during initialization.
 >
 > So there may be classes of errors that should do so; i certainly don't
 think zone-loading is one of them :)

 Content errors in a zone file shouldn't cause such disruption, but I
 don't think it causes std or other abnormal type of exceptions.

 Regarding the "don't throw" requirement in the remote config handler,
 while I've not fully thought about it, in my gut feeling it's not
 about throw or no throw, because it should be possible to catch
 exceptions in the caller side of the module CC if it were only about
 exception.  The real issue seems, as I mentioned in the previous
 comment (cited above), "what we should do if remote config update
 fails in general", whether or not it's reported in the form of
 exception.  Errors can happen (just like we see it in this topic), and
 even if we hide the corresponding exception, the problem of how we
 ensure the system-level consistency among different modules that
 share the config doesn't go away.  That's what I meant by "part of
 another big problem".

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