Opened 8 years ago

Closed 7 years ago

#2204 closed task (complete)

revise auth DataSourceConfiguratorGeneric::reconfigure()

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20121023
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: 6 Add Hours to Ticket: 0
Total Hours: 7.69 Internal?: no

Description (last modified by jinmei)

A subtask of #2201, depend on #2203.

This change will allow us to exclude the whole reconfigure() from a
critical section protected by a lock in a later stage of this work.

What to do are (there are many, but each should be pretty straightforward):

  • change type of AuthSrvImpl::client_lists_ to a shared_ptr of map
  • don't reuse the existing client list: always create a new one and builds it with the new config
  • pass reconfigure() an empty map<RRClass, ConfigurableClientList(PTr)> and have reconfigure() fill in it with the entire new data source client lists
  • introduce AuthSrv::setDataSourceClientLists(). It takes a shared pointer to the new map created by reconfigure() and swaps it with the current value of AuthSrvImpl::client_lists_, and returns the old one.
  • Deprecate the current AuthSrv::setClientList() interface (we don't need it any more)
  • revise the command handler (introduced in #2203) so it calls reconfigure() with the new config and then call setDataSourceClientLists() with the result. For now, the old map returned by setDataSourceClientLists() is just dropped at this point (but the swap behavior will become important in #2210)

Subtickets

Change History (19)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jelte

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

comment:4 Changed 8 years ago by jelte

  • Milestone Sprint-20120918 deleted

comment:5 Changed 8 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 8 years ago by muks

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

comment:7 Changed 7 years ago by jinmei

trac2204 is ready for review.

The branch is mostly a straightforward implementation of what is
described in the ticket. A few notes:

  • The first commit is to merge dependency (#2203). It should be ignored for review.
  • The most essential change is d664fca and 5d61dba, especially the former. See the commit log of it. The rest of the changes are minor cleanups or straightforward adjustments.
  • The last commit (86a4bae) is not strictly necessary, but the amount of diff is relatively big. So, it may be easier to first review the branch excluding this commit, and then review this commit separately.

No plan to add changelog.

comment:8 Changed 7 years ago by jinmei

  • Owner set to UnAssigned
  • Status changed from new to reviewing

comment:9 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:10 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I noticed the new approach is simpler. However, it drops one place for future
improvement. I planned that when there's an update, we would look through the
old list and reuse the clients with the same configuration, instead of creating
them again. I don't think this was implemented, but with your new approach, it
doesn't seem to be easily possible. If we want to provide the shared-memory
approach soon, I'm willing to trade the thing for simpler code, but we should
at least consider if it is OK.

Also, I thought that when there's an update of configuration and the top-level
item is unchanged, it is not contained in the update. That is, if we added
another item into data_sources besides classes and the new one got changed, we
would get an update not containing any classes element. This seems not to be
taken into account in the handler in main.cc.

This comment seems to be wrong, strictly speaking, the return value is not
really ignored, it is stored into a variable (which is then not used). It's not
that'd require full formalism, but it is confusing ‒ I was trying to see which
return value is ignored.

By ignoring the return value we let the
old lists be destroyed.  Lock will be released immediately after the
swap.

This comment looks like it misses the third form, should it be „returns“? Or
„will return“?

/// This will hook into the data_sources module configuration and it return
/// a new set (in the form of a shared pointer to map) of data source client
/// lists corresponding to the configuration.

The tests still speak about rollback, but currently, there's no rollback in the
code. It might be misleading, so I think the names of tests and comments should
be updated:

// Check that we can rollback an addition if something else fails
// Check that we can rollback a deletion if something else fails
// Check that we can roll back configuration change if something
// fails later on.

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

Replying to vorner:

Thanks for the review.

I noticed the new approach is simpler. However, it drops one place for future
improvement. I planned that when there's an update, we would look through the
old list and reuse the clients with the same configuration, instead of creating
them again. I don't think this was implemented, but with your new approach, it
doesn't seem to be easily possible. If we want to provide the shared-memory
approach soon, I'm willing to trade the thing for simpler code, but we should
at least consider if it is OK.

Yeah, I was actually aware of that, and I admit I should mention it.
At least for our middle term goals (like for the next beta release) I
don't see any benefit to keep complicated interface with possible
future extensions. But if you want to leave some consideration notes
on this point, I suggest adding the following to the detailed
description for configureDataSourceGeneric():

/// \note In future we may want to make the reconfiguration more efficient
/// by only creating newly configured data and just moving the rest from
/// the running configuration if they are used in the new configuration
/// without any parameter change.  We could probably do it by passing
/// the old lists in addition to the new config, but further details are
/// still to be defined yet.  It will surely require changes in the
/// data source library, too.  So, right now, we don't introduce the
/// possibility in the function interface.  If and when we decide to introduce
/// the optimization, we'll extend the interface.

I personally don't think the current approach makes this goal
particularly more difficult, though (I'm not saying it's easy - it was
already uneasy task, and I don't think the current change does not
make it even harder). I can think of some possibility of doing it by
extending the current interface, although there can be unseen pitfalls
in details.

Also, I thought that when there's an update of configuration and the top-level
item is unchanged, it is not contained in the update. That is, if we added
another item into data_sources besides classes and the new one got changed, we
would get an update not containing any classes element. This seems not to be
taken into account in the handler in main.cc.

Sorry, I don't understand this. Could you some specific example?

This comment seems to be wrong, strictly speaking, the return value is not
really ignored, it is stored into a variable (which is then not used). It's not
that'd require full formalism, but it is confusing ‒ I was trying to see which
return value is ignored.

By ignoring the return value we let the
old lists be destroyed.  Lock will be released immediately after the
swap.

Ah, sorry, it didn't make sense. I guess it was an intermediate
version when the return value was indeed "ignored". I've updated the
comments so it will match the code.

This comment looks like it misses the third form, should it be „returns“? Or
„will return“?

/// This will hook into the data_sources module configuration and it return
/// a new set (in the form of a shared pointer to map) of data source client
/// lists corresponding to the configuration.

That should have been "will return". Fixed.

The tests still speak about rollback, but currently, there's no rollback in the
code. It might be misleading, so I think the names of tests and comments should
be updated:

Good catch, I think we only have to change the wording of test name
and comments. I've updated it that way.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Yeah, I was actually aware of that, and I admit I should mention it.
At least for our middle term goals (like for the next beta release) I
don't see any benefit to keep complicated interface with possible
future extensions. But if you want to leave some consideration notes
on this point, I suggest adding the following to the detailed
description for configureDataSourceGeneric():

/// \note In future we may want to make the reconfiguration more efficient
/// by only creating newly configured data and just moving the rest from
/// the running configuration if they are used in the new configuration
/// without any parameter change.  We could probably do it by passing
/// the old lists in addition to the new config, but further details are
/// still to be defined yet.  It will surely require changes in the
/// data source library, too.  So, right now, we don't introduce the
/// possibility in the function interface.  If and when we decide to introduce
/// the optimization, we'll extend the interface.

OK, let's add the note and leave it this way. Though I had a reasonably easy
approach to do the reuse.

Also, I thought that when there's an update of configuration and the top-level
item is unchanged, it is not contained in the update. That is, if we added
another item into data_sources besides classes and the new one got changed, we
would get an update not containing any classes element. This seems not to be
taken into account in the handler in main.cc.

Sorry, I don't understand this. Could you some specific example?

OK, let's say we have the data_sources configuration, but because of some
discussion, besides of classes, we add another item called „zones“:

"data_sources": {
	"classes": {
		"IN": { something },
		"CH": { something else }
	},
	"zones": {
		whatever
	}
}

Now we do this in bindctl:

config set data_sources/zones "some other thing"
config commit

The data_sources/classes didn't change. And the config manager AFAIK sends only
something like partial diffs, only the items that were changed ‒ so the update
contains "zones" but does not contain "classes".

So, the update that does not contain the "classes" item should be ignored. In
the current state, it would either erase all the data sources or crash (I
didn't really look closely which would happen), which is both bad.

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

Replying to vorner:

Note: I made one additional editorial cleanup at 86e4008.

/// \note In future we may want to make the reconfiguration more efficient
/// by only creating newly configured data and just moving the rest from
/// the running configuration if they are used in the new configuration
/// without any parameter change.  We could probably do it by passing
/// the old lists in addition to the new config, but further details are
/// still to be defined yet.  It will surely require changes in the
/// data source library, too.  So, right now, we don't introduce the
/// possibility in the function interface.  If and when we decide to introduce
/// the optimization, we'll extend the interface.

OK, let's add the note and leave it this way. Though I had a reasonably easy
approach to do the reuse.

I've added the above comment. Although I don't think the current
version is particularly more difficult of supporting such incremental
update, or even in some sense it's now easier due to the simpler form,
but as long as it's not a blocking issue I won't fight about that.

Also, I thought that when there's an update of configuration and the top-level
item is unchanged, it is not contained in the update. That is, if we added
another item into data_sources besides classes and the new one got changed, we
would get an update not containing any classes element. This seems not to be
taken into account in the handler in main.cc.

Sorry, I don't understand this. Could you some specific example?

OK, let's say we have the data_sources configuration, but because of some
discussion, besides of classes, we add another item called „zones“:

[...]

The data_sources/classes didn't change. And the config manager AFAIK sends only
something like partial diffs, only the items that were changed ‒ so the update
contains "zones" but does not contain "classes".

Okay, I understood what you meant, but I still don't see how that
applies to this implementation. If the updated config doesn't contain
any info about "classes", just nothing will happen (and the server
will keep running with the current lists) due to this:

void
datasrcConfigHandler(AuthSrv* server, bool* first_time,
                     ModuleCCSession* config_session, const std::string&,
                     isc::data::ConstElementPtr config,
                     const isc::config::ConfigData&)
{
    assert(server != NULL);
    if (config->contains("classes")) {
...
            lists = configureDataSource(config->get("classes"));
    }

Or do I still misunderstand you?

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:16 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 2.19

Ups, my apologies. I overlooked this condition and I was looking at the
internal one, with first_time. This indeed looks correct.

Please merge.

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

Replying to vorner:

Ups, my apologies. I overlooked this condition and I was looking at the
internal one, with first_time. This indeed looks correct.

Please merge.

Okay, thanks. Merge done, closing.

comment:18 in reply to: ↑ 17 Changed 7 years ago by jinmei

Replying to jinmei:

Replying to vorner:

Ups, my apologies. I overlooked this condition and I was looking at the
internal one, with first_time. This indeed looks correct.

Please merge.

Okay, thanks. Merge done, closing.

Oh, btw, I just realized I forgot pushing the comment updates
regarding future extension with an additional comment cleanup.
The former was already agreed and I believe the latter was trivial,
so I'll merge and close the ticket anyway, but if you have time
please check it.

comment:19 Changed 7 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 2.19 to 7.69
Note: See TracTickets for help on using tickets.