Opened 8 years ago

Closed 7 years ago

#2213 closed task (complete)

revise LoadZoneCommand::exec() of b10-auth to use the configurator thread

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20121106
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: background zone loading
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 2 Internal?: no

Description

A subtask of #2201, depends on #2205, #2212

This task revise the exec() method so it now just creates a "reload
zone" command for the thread, and wakes up the thread with it.

At this point, we can (should) deprecate ConfigurableClientList::reload().

When this task completes, zone reloading should now be possible
without disrupting service.

Subtickets

Change History (18)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120918

comment:2 Changed 7 years ago by jelte

  • Milestone Sprint-20120918 deleted

comment:3 Changed 7 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 7 years ago by jinmei

  • Milestone set to Sprint-20121023

(moving the ticket to the 20121023 sprint as it's specified in the meeting minutes
but actually not in the queue).

comment:5 Changed 7 years ago by jinmei

This ticket should be able to be started based on the latest snapshot
of trac2212 (see #2212).

What should be done is:

  • write new tests for the next bullet
  • add loadzone() method to DataSrcClientsMgr. It will be port of the initial half of LoadZoneCommand::exec(), including validation of args (it doesn't have to check the existence of the client list), and send the LOADZONE command with the argument of the pair of class and origin (note that the builder expects a complete pair of class and zone. So the manager needs to pass the default class if it's omitted)
  • remove now-unnecessary tests in command_unittest. These are loadZone, loadZoneSQLite3, loadBrokenZone, loadUnreadableZone, loadZoneWithoutDataSrc, and loadZoneInvalidParams, and helper functions used by these tests.

comment:6 Changed 7 years ago by jelte

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

comment:7 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

(piece of comment taken from #2212, about where the code should fill in the IN value for the optional class argument, if not provided)

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.

Yes, builder shouldn't have access to that data, and it could be placed in auth_srv.cc, or even main.cc right now.

I've been looking into this, however, the command specification is pretty well-enclosed in the API (apart from validateCommand() it doesn't offer much atm). I thought about extending it to allow for getDefaultValue() similar to the existing one, since that one is really for config only. But doing so would really be a separate task in itself.

And while the value would not be hard-coded, the name (and what defaults to fill in) would still be, so I am going to propose a different change, in a new ticket, and make it hardcoded here for now. In short, I want the validation itself to get an option to fill in defaults (so that both for config and commands you'll always know you should have this kind of data available, both for mandatory data and optional data that has defaults). IMHO this is a step that will be needed as part of a bigger refactor anyway, and a relatively easy one that gives us much for relatively little (though the diff would probably be pretty big, as it needs to either factor out and copy a lot of api, or change a lot of consts to non-consts).

Anyway, this branch is ready for review :)

Additional notes:

  • command.cc needs something to call to process the loadzone command; rather than expose the entire builder I chose to make a very simple loadZone() method in AuthSrv? (that simply forwards it)
  • updated a few lettuce tests for the now new log message that is printed when reloading

comment:8 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:9 follow-up: Changed 7 years ago by jinmei

Some important points first:

Since this version doesn't validate name or class beyond they are
string, it causes unexpected termination of builder due to bogus name
or class value (such as "example..com", or "INN"). At least this
should be fixed some way.

One way is to tighten the validation in the builder, but I personally
think it's better to catch it at the manager side because we can then
give the user (if it comes from a user via bindctl etc) an immediate
error report.

As for hardcoding as an intermediate workaround, I'm okay with it, but
I'd suggest leaving some comments about this point.

And, a relatively minor points:

  • maybe we should generalize LoadZoneCommandError so we can use it for all commands (in case we want to throw an error from them).
  • why do we need AuthSrv::loadZone? I guess LoadZoneCommand can do server.getDataSrcClientsMgr().loadZone().

BTW, I think we need a changelog entry (for the entire feature),
either as a separate one or by updating changelog no. 495.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

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

  • Owner changed from jelte to jinmei

Replying to jinmei:

One way is to tighten the validation in the builder, but I personally
think it's better to catch it at the manager side because we can then
give the user (if it comes from a user via bindctl etc) an immediate
error report.

ah of course, the old errors don't bubble up anymore.

I've added basic checking for the origin and class strings. There are however more scenarios that aren't caught; for starters if the zone (or class) itself isn't known in the configuration.

We could add support for that (by factoring out the first part of the real threaded handler, that looks up the zone in the map), but it would mean doing that twice too (however, this may be relatively cheap anyway).

Shall we do that too? (it should make error more reporting complete, up to the actual reading of data at least)

As for hardcoding as an intermediate workaround, I'm okay with it, but
I'd suggest leaving some comments about this point.

added

And, a relatively minor points:

  • maybe we should generalize LoadZoneCommandError so we can use it for all commands (in case we want to throw an error from them).

I made it CommandError?, and added a bit of doxy that it is supposed to be thrown if initial checks fail (i.e. before any actual command is sent)

  • why do we need AuthSrv::loadZone? I guess LoadZoneCommand can do server.getDataSrcClientsMgr().loadZone().

ah doh, no we don't. Originally I didn't want to expose the whole clients mgr, and I had missed that this had been done anyway :p

Removed.

BTW, I think we need a changelog entry (for the entire feature),
either as a separate one or by updating changelog no. 495.

hmm yes.

[func] er, many devs :)
The b10-auth 'loadzone' command now uses the internal thread introduced in 495 to (re)load a zone in the background, so that query processing isn't blocked while loading a zone.
(Trac #2213, git xxx)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by jinmei

Replying to jelte:

One way is to tighten the validation in the builder, but I personally
think it's better to catch it at the manager side because we can then
give the user (if it comes from a user via bindctl etc) an immediate
error report.

ah of course, the old errors don't bubble up anymore.

I've added basic checking for the origin and class strings. There are however more scenarios that aren't caught; for starters if the zone (or class) itself isn't known in the configuration.

We could add support for that (by factoring out the first part of the real threaded handler, that looks up the zone in the map), but it would mean doing that twice too (however, this may be relatively cheap anyway).

Shall we do that too? (it should make error more reporting complete, up to the actual reading of data at least)

I have a mixed opinion about this. In terms of user-friendliness it's
better to do double check; mistyping a zone name wouldn't be uncommon,
and the overhead of the double check shouldn't be a big concern in
this case. On the other hand, redundant behavior is generally not
good, and, at least in theory, the check at the manager isn't 100%
reliable anyway; the builder may be updating the data source clients
at the time the manager receives the reload command, and the specified
zone may disappear when the builder finally receives the command from
the manager (although this scenario is more unlikely than a user
mistyping the zone name).

So, I'd leave it to you. But if you decide to add the check at the
manager, I suggest combining the check code some way rather than
copying the same logic from the builder to the manager.

[func] er, many devs :)
The b10-auth 'loadzone' command now uses the internal thread introduced in 495 to (re)load a zone in the background, so that query processing isn't blocked while loading a zone.
(Trac #2213, git xxx)

This looks good.

On the branch itself:

datasrc_clients_mgr.h

  • you can skip explicitly adding else...if here:
            } else {
                // Also check if it really is a valid name
                try {
                    dns::Name(args->get("origin")->stringValue());
                } catch (const isc::Exception& exc) {
                    isc_throw(CommandError, "bad origin: " << exc.what());
                }
            }
    
    which would make the code a bit more concise. Same for the 'class' part. But that's minor and may be matter of preference. I'd leave it to you.
  • this doesn't parse:
        // but atm the config/command API does not allow that to be done
        // easily.
    
    maybe atm = "at the moment"? If so it parses, although I think it's first time to me to see this acronym.

Others

  • I made some trivial style fixes.
  • I suggest removing AUTH_LOAD_ZONE from the log message.

comment:13 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:14 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

We could add support for that (by factoring out the first part of the real threaded handler, that looks up the zone in the map), but it would mean doing that twice too (however, this may be relatively cheap anyway).

Shall we do that too? (it should make error more reporting complete, up to the actual reading of data at least)

I have a mixed opinion about this. In terms of user-friendliness it's
better to do double check; mistyping a zone name wouldn't be uncommon,
and the overhead of the double check shouldn't be a big concern in
this case. On the other hand, redundant behavior is generally not
good, and, at least in theory, the check at the manager isn't 100%
reliable anyway; the builder may be updating the data source clients
at the time the manager receives the reload command, and the specified
zone may disappear when the builder finally receives the command from
the manager (although this scenario is more unlikely than a user
mistyping the zone name).

So, I'd leave it to you. But if you decide to add the check at the
manager, I suggest combining the check code some way rather than
copying the same logic from the builder to the manager.

Yes we'd have to factor the check itself out a bit, and lock the map (which itself needs careful checking). I'll leave it for now, consider it a bit more, and if I still want it, I'll create a new ticket for just that.

  • you can skip explicitly adding else...if here:
            } else {
                // Also check if it really is a valid name
                try {
                    dns::Name(args->get("origin")->stringValue());
                } catch (const isc::Exception& exc) {
                    isc_throw(CommandError, "bad origin: " << exc.what());
                }
            }
    

done

  • this doesn't parse:
        // but atm the config/command API does not allow that to be done
        // easily.
    
    maybe atm = "at the moment"? If so it parses, although I think it's first time to me to see this acronym.

Yes, atm = "at the moment", I use it so often I don't even realize it anymore :) expanded the acronym.

Others

  • I suggest removing AUTH_LOAD_ZONE from the log message.

done

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

Replying to jelte:

The changes are okay. I just noticed one minor editorial point (a
stale comment) and fixed it (42298ce). With this change please merge.

And we are done:-)

comment:16 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:17 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 2

comment:18 Changed 7 years ago by jelte

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

Even though it triggered a new problem with log4cplus and threads (we found after merge), I consider this ticket itself closed.

Note: See TracTickets for help on using tickets.