Opened 3 years ago

Closed 3 years ago

#5134 closed enhancement (complete)

Control Agent: Implement configuration storage and SimpleParser

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.2
Component: agent Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Mozilla Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 2
Total Hours: 31 Internal?: no

Description

Ticket #5076 implemented bison parser that sanity checks the input config file and produces Element tree structure. Now we need to convert this structure to usable configuration stored in CltrAgentConfigContext?. This part should be done using SimpleParser.

This ticket is a follow-up to 5076.

Subtickets

Change History (15)

comment:1 Changed 3 years ago by tomek

  • Component changed from Unclassified to agent

comment:2 Changed 3 years ago by fdupont

Work on it can be started IMHO...

comment:3 Changed 3 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.2
  • Owner set to tomek
  • Status changed from new to assigned

comment:4 follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 20
  • Total Hours changed from 0 to 20

The ticket is now ready for review.

Proposed ChangeLog:

12XX.	[func]		tomek
	Control agent is now able to parse the input configuration and
	store it in its configuration syntax structure. Upcoming tickets
	will take advantage of that information.
	(Trac #5134, git tbd)

Couple comments:

  1. I have extended DCfgMgr and DController classes. The code that is still there (and still being used by D2) was fine before we didn't have SimpleParser. I hope (but not advocate in any way) that eventually D2 will migrate to this new methods and we could remove the intermediate storage completely.
  1. Now that I learned a bit more about the architecture in libprocess, I find many of its advantages. One day we should think how we could migrate DHCPv4 and DHCPv6 to it. IMHO that's a goal that doable, just not in one step.
  1. One of the features that will be needed soon is configuration verification (or test-mode if you prefer). This is something that was recently added to DHCPv4 and DHCPv6 and I tried to implement the configuration handling in CA to make it as easy to implement as possible. The major chunk of work is already in, but there are couple pieces missing. However, I didn't want to push them through this ticket as this is out of scope for that particular ticket.
  1. The configuration is able to process hooks configuration and even load hook libraries. I have added one dummy lib that is used in unit-tests. There's broad area of extra checks that could be done, but I decided that the scope of this ticket was to parse configuration, not implement whatever the configuration conveys. As such, I made an arbitrary stop here. A separate ticket should probably cover actual proper library loads/unloads, define hook points etc.
  1. [added later] There's nice piece of code coming in 5110 that will allow using dedicated parser (agent::ParserContext? in this case) to parse input config into Element tree. Those changes are done in base DCfgMgr and DProcess classes. Once both this and #5110 are merged, we can update the code to use bison parser (the production code on this branch still uses legacy Element::fromJSON, the unit-tests are using bison parser already).
Last edited 3 years ago by tomek (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 3 years ago by fdupont

Replying to tomek:

The ticket is now ready for review.

=> Note this is not a review but additional information about comments

  1. One of the features that will be needed soon is configuration verification (or test-mode if you prefer). This is something that was recently added to DHCPv4 and DHCPv6 and I tried to implement the configuration handling in CA to make it as easy to implement as possible. The major chunk of work is already in, but there are couple pieces missing. However, I didn't want to push them through this ticket as this is out of scope for that particular ticket.

=> #3770 was merged with #5152 followup ticket which explicitly includes this point 3.

  1. The configuration is able to process hooks configuration and even load hook libraries. I have added one dummy lib that is used in unit-tests. There's broad area of extra checks that could be done, but I decided that the scope of this ticket was to parse configuration, not implement whatever the configuration conveys. As such, I made an arbitrary stop here. A separate ticket should probably cover actual proper library loads/unloads, define hook points etc.

=> the current hook stuff is not unparsable (configuration state is not stored at the right place). There is a ticket (#5145) about this so please consider any way to conciliate both this point and this ticket.

  1. [added later] There's nice piece of code coming in 5110 that will allow using dedicated parser (agent::ParserContext? in this case) to parse input config into Element tree. Those changes are done in base DCfgMgr and DProcess classes. Once both this and #5110 are merged, we can update the code to use bison parser (the production code on this branch still uses legacy Element::fromJSON, the unit-tests are using bison parser already).

=> there are some parser cleanup tickets waiting for #5110 too.

comment:6 Changed 3 years ago by tomek

  • Owner tomek deleted
  • Status changed from assigned to reviewing

comment:7 Changed 3 years ago by tomek

  • Owner set to Unassigned

Note: after #5110 was merged, I have rebased this work. Please review trac5134_rebase. If needed, this code can now benefit from #5110 work.

comment:8 Changed 3 years ago by tomek

  • Sub-Project changed from DHCP to Mozilla

comment:9 Changed 3 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:10 follow-up: Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 20 to 4
  • Owner changed from marcin to tomek
  • Total Hours changed from 20 to 24

I reviewed 0852eacf87796827ccc96c063a420d880c28abf1.

I had some issues with building the code on FreeBSD11 due to disallowed conversion from null to uint32_t. This is unrelated to your changes, but there was an occasion to fix it so I pushed the fix.

Another issue I found was a core dump when running new unit tests for the Control Agent. The problem is that hooks library still uses static initializations which may cause static deinitialization fiasco. I fixed it an pushed to the branch. However, I guess there are probably still some other cases like that which are not resolved. This may need its own ticket.

src/bin/agent/ctrl_agent_cfg_mgr.cc
getConfigSummary: if there are no sockets configured it is a special case, but it shouldn't be treated as "weird". In theory, the CA can handle some commands on its own and doesn't need to route them to any other server.

src/bin/agent/ctrl_agent_cfg_mgr.h
The clone() function appears to create a new instance of the conext but this instance will be dependent on the original instance in that the ctrl_sockets array members will not be copied but only their pointers. Is this intended?

Because {get,set}ControlSocketInfo? are longer than one line, wouldn't it be better to put their implementation in the cc file?

getControlSocketInfo description should be more clear as to what is returned when a socket for a given server type hasn't been specified. To me it looks like null is returned but the description doesn't state that.

I suggest that {set,get}Host and {set, get}Port are renamed to {set,get}HttpHost and {set,get}HttpPort to make it clear that they set values of "http-host" and "http-port" parameters. CA handles HTTP connection and connections with the servers, thus the parameter names are prefixed with "http-" to prevent confusion.

Why can't we simply remove createConfigParser method?

src/bin/agent/simple_parser.cc
This file includes some headers which probably aren't required. I am specifically thinking about DhcpConfigError and boost/foreach.hpp

setAllDefaults contains the following:

if (d2) {
   cnt += SimpleParser::setDefaults(d6, SOCKET_DEFAULTS);
}

The "d2" is probably a typo or copy-paste error?

I am also wondering why unit tests didn't catch that?

src/bin/agent/simple_parser.h
There seem to be copy-paste errors in the AgentSimpleParser description, as it talks about DHCPv4 specific parser.

I am guessing that we may get a lot of cppcheck warning about objects not being passed by reference but by value.

src/bin/agent/tests/Makefile.am
While I don't object to the rename of ctrl_agent_unittest to run_unittests, I believe that such changes should rather be done in separate ticket after all other tickets (related to CA in this case) have been merged. Usually, such changes cause annoying conflicts between tickets being implemented at the same time and make it easy to miss some unit tests when conflicts are being resolved.

As you renamed the binary, please make sure that the .gitignore is properly updated too.

src/bin/agent/tests/basic_library.cc
You probably don't need to include <fstream>

src/bin/agent/tests/test_libraries.h.in
Instead of creating this header file, couldn't you simply pass the define from Makefile.am using -D compiler switch?

Also, if keeping this file anyway, the .gitignore should be updated.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
As the Hooks libraries parser has been moved to another library, wouldn't it make sense to move hooks specific unit tests from dhcpsrv to hooks?

src/lib/hooks/Makefile.am
src/lib/hooks/hooks_parser.cc
Implementation of HooksLibrariesParser should be enclosed in namespace isc::hooks.

src/lib/process/d_cfg_mgr.cc
src/lib/process/d_cfg_mgr.h
We now have parseConfig and simpleParseConfig methods which essentially do similar things using old and new style. I am guessing that we keep both because D2 is still using old style config? Are we going to migrate D2 to simpleParseConfig anytime soon? Note that now we have certain log messages, e.g. DCTL_CONFIG_START etc., used in both functions which is against the guideline for having only one occurrence of a certain log message n the code. If we're going to remove parseConfig function as part of the 1.2 effort, I think it is ok to keep 2 occurrences of the same log message for now. Otherwise, we should rename one or another.

Also, simpleParseConfig appears to have no unit tests.

comment:11 Changed 3 years ago by marcin

BTW, I also applied several fixes in the commentary, removed some typos and corrected copyright dates. But, not sure if I caught all cases of copyright dates not updated.

comment:12 in reply to: ↑ 10 ; follow-ups: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 4 to 5
  • Owner changed from tomek to marcin
  • Total Hours changed from 24 to 29

Replying to marcin:

I reviewed 0852eacf87796827ccc96c063a420d880c28abf1.

I had some issues with building the code on FreeBSD11 due to disallowed conversion from null to uint32_t. This is unrelated to your changes, but there was an occasion to fix it so I pushed the fix.

Another issue I found was a core dump when running new unit tests for the Control Agent. The problem is that hooks library still uses static initializations which may cause static deinitialization fiasco. I fixed it an pushed to the branch. However, I guess there are probably still some other cases like that which are not resolved. This may need its own ticket.

Thanks for fixing those issues.

src/bin/agent/ctrl_agent_cfg_mgr.cc
getConfigSummary: if there are no sockets configured it is a special case, but it shouldn't be treated as "weird". In theory, the CA can handle some commands on its own and doesn't need to route them to any other server.

Updated the comment.

src/bin/agent/ctrl_agent_cfg_mgr.h
The clone() function appears to create a new instance of the conext but this instance will be dependent on the original instance in that the ctrl_sockets array members will not be copied but only their pointers. Is this intended?

Added explicit copy constructor. It still copies the smart pointers only, but it has a comment that explains what's going on. Surprisingly, there's no method to easily copy Element tree. The easiest way would be to convert it to text and then create new instance based on that text. This seems a bit of overkill, so I decided to stick with the pointer copying. Also added new unit-test that ensures the copied context has correct values.

If you really insist on copying the values (why here specifically? we have tons of other places in the code where smart pointers are used to reference the same objects from multiple places), I will open a new ticket that will cover making a copy of Element tree and returning smart pointer to the copy.

Because {get,set}ControlSocketInfo? are longer than one line, wouldn't it be better to put their implementation in the cc file?

Moved as suggested.

getControlSocketInfo description should be more clear as to what is returned when a socket for a given server type hasn't been specified. To me it looks like null is returned but the description doesn't state that.

Description updated.

I suggest that {set,get}Host and {set, get}Port are renamed to {set,get}HttpHost and {set,get}HttpPort to make it clear that they set values of "http-host" and "http-port" parameters. CA handles HTTP connection and connections with the servers, thus the parameter names are prefixed with "http-" to prevent confusion.

You seem to prefer both long names and short line length limits. This tends to lead to frequent code wraps. But I admit it's a matter of preference, so updated as suggested.

Why can't we simply remove createConfigParser method?

Remove it and see for yourself :)

Hint: the createConfigParser is pure virtual method in base class. It's also being used in D2 *in this version of the code*. There is 5110 ticket that was merged that changed those areas of the code. So to not make the merge any more difficult, it's better to leave this as is. Once both this and 5110 are merged, DCfgMgr class restructuring can be done and this method would be removed.

src/bin/agent/simple_parser.cc
This file includes some headers which probably aren't required. I am specifically thinking about DhcpConfigError and boost/foreach.hpp

setAllDefaults contains the following:

if (d2) {
   cnt += SimpleParser::setDefaults(d6, SOCKET_DEFAULTS);
}

The "d2" is probably a typo or copy-paste error?

Yes. Good catch.

I am also wondering why unit tests didn't catch that?

Perhaps because unit-tests don't test every possible parameter combination? This particular piece of code is tested by 2 unit tests: AgentParserTest?.configParse1Socket and AgentParserTest?.configParse3Sockets. Added two more unit-tests and fixed the bug.

src/bin/agent/simple_parser.h
There seem to be copy-paste errors in the AgentSimpleParser description, as it talks about DHCPv4 specific parser.

Oops. Corrected.

I am guessing that we may get a lot of cppcheck warning about objects not being passed by reference but by value.

Not anymore :)

src/bin/agent/tests/Makefile.am
While I don't object to the rename of ctrl_agent_unittest to run_unittests, I believe that such changes should rather be done in separate ticket after all other tickets (related to CA in this case) have been merged. Usually, such changes cause annoying conflicts between tickets being implemented at the same time and make it easy to miss some unit tests when conflicts are being resolved.

As you renamed the binary, please make sure that the .gitignore is properly updated too.

Updated.

src/bin/agent/tests/basic_library.cc
You probably don't need to include <fstream>

Removed.

src/bin/agent/tests/test_libraries.h.in
Instead of creating this header file, couldn't you simply pass the define from Makefile.am using -D compiler switch?

For just one library you would be right, but you need to ask a question whether you think that's the only test library we will ever have for control agent. If in doubt, take a look at src/lib/hooks/test_libraries.h.in. :)

Also, if keeping this file anyway, the .gitignore should be updated.

Updated.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
As the Hooks libraries parser has been moved to another library, wouldn't it make sense to move hooks specific unit tests from dhcpsrv to hooks?

Given infinite amount of time - yes. Unfortunately, the unit-tests in src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc are covering tons of different things and happen to be very DHCP specific. Extracting certain subset of tests may involve a lot of work for minor improvement in unit-test readability.

src/lib/hooks/Makefile.am
src/lib/hooks/hooks_parser.cc
Implementation of HooksLibrariesParser should be enclosed in namespace isc::hooks.

Updated as suggested.

src/lib/process/d_cfg_mgr.cc
src/lib/process/d_cfg_mgr.h
We now have parseConfig and simpleParseConfig methods which essentially do similar things using old and new style. I am guessing that we keep both because D2 is still using old style config? Are we going to migrate D2 to simpleParseConfig anytime soon? Note that now we have certain log messages, e.g. DCTL_CONFIG_START etc., used in both functions which is against the guideline for having only one occurrence of a certain log message n the code. If we're going to remove parseConfig function as part of the 1.2 effort, I think it is ok to keep 2 occurrences of the same log message for now. Otherwise, we should rename one or another.

That's a question to Thomas. My perception is that simpleParseConfig is... well, simpler, better prepared for configuration checking and rollback, and does not use any intermediate storage. But I don't want to push this approach on Thomas.

Also, simpleParseConfig appears to have no unit tests.

This function manages the whole configuration of the whole process. So at this level it's not unit anymore, because the unit is the whole process. It is tested, just not exactly from unit-tests for process. I did an experiment and updated simpleParseConfig to always return failed answer. As a result, number of tests in src/bin/agent failed.

But I suppose that may not be sufficient for you. So implemented some stub code and added
DStubCfgMgrTest.simpleParseConfig with extra commentary explaining why more thorough tests will not be implemented. Hope that's sufficient.

Ok, that's it. Let me know if this addresses all the comments.

comment:13 in reply to: ↑ 12 Changed 3 years ago by fdupont

Replying to tomek:

src/bin/agent/ctrl_agent_cfg_mgr.h

If you really insist on copying the values (why here specifically? we have tons of other places in the code where smart pointers are used to reference the same objects from multiple places), I will open a new ticket that will cover making a copy of Element tree and returning smart pointer to the copy.

=> IMHO you should do a deep copy but there is no need for a new ticket as this function is needed too for get-config aka #1205 / #5114 so I have already created the ticket #5158, written the (easy) code and it will be anyway available soon. So I suggest to note to update the agent code when isc.::data::copy(ConstElementPtr from) will be merged.

comment:14 in reply to: ↑ 12 Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 5 to 2
  • Owner changed from marcin to tomek
  • Total Hours changed from 29 to 31

Replying to tomek:

Replying to marcin:

I reviewed 0852eacf87796827ccc96c063a420d880c28abf1.

I had some issues with building the code on FreeBSD11 due to disallowed conversion from null to uint32_t. This is unrelated to your changes, but there was an occasion to fix it so I pushed the fix.

Another issue I found was a core dump when running new unit tests for the Control Agent. The problem is that hooks library still uses static initializations which may cause static deinitialization fiasco. I fixed it an pushed to the branch. However, I guess there are probably still some other cases like that which are not resolved. This may need its own ticket.

Thanks for fixing those issues.

src/bin/agent/ctrl_agent_cfg_mgr.cc
getConfigSummary: if there are no sockets configured it is a special case, but it shouldn't be treated as "weird". In theory, the CA can handle some commands on its own and doesn't need to route them to any other server.

Updated the comment.

src/bin/agent/ctrl_agent_cfg_mgr.h
The clone() function appears to create a new instance of the conext but this instance will be dependent on the original instance in that the ctrl_sockets array members will not be copied but only their pointers. Is this intended?

Added explicit copy constructor. It still copies the smart pointers only, but it has a comment that explains what's going on. Surprisingly, there's no method to easily copy Element tree. The easiest way would be to convert it to text and then create new instance based on that text. This seems a bit of overkill, so I decided to stick with the pointer copying. Also added new unit-test that ensures the copied context has correct values.

If you really insist on copying the values (why here specifically? we have tons of other places in the code where smart pointers are used to reference the same objects from multiple places), I will open a new ticket that will cover making a copy of Element tree and returning smart pointer to the copy.

I think it is fine to make copy constructor private if we expect to use clone() rather than copy constructor. I am also ok with copying pointers rather than objects if the particular use case doesn't require independence of the source and copy. However, I'd still suggest to put a doxygen comment in the description of clone() stating that objects aren't independent:

    /// Note that the returned instance shares some information with the
    /// original and thus modification of one of those objects may affect
    /// the other.

To move copy constructor to private scope you could also use a C++11 feature like this:

private:
    CtrlAgentCfgContext(const CtrlAgentCfgContext& orig) = default;

and in that case you woudn't need to provide your own implementation of the copy constructor, as this is always error prone. However, I think we have a practice to test for C++11 features support in configure.ac. Thus, if you don't want to do it here, it is ok to leave as is.


Because {get,set}ControlSocketInfo? are longer than one line, wouldn't it be better to put their implementation in the cc file?

Moved as suggested.

getControlSocketInfo description should be more clear as to what is returned when a socket for a given server type hasn't been specified. To me it looks like null is returned but the description doesn't state that.

Description updated.

I suggest that {set,get}Host and {set, get}Port are renamed to {set,get}HttpHost and {set,get}HttpPort to make it clear that they set values of "http-host" and "http-port" parameters. CA handles HTTP connection and connections with the servers, thus the parameter names are prefixed with "http-" to prevent confusion.

You seem to prefer both long names and short line length limits. This tends to lead to frequent code wraps. But I admit it's a matter of preference, so updated as suggested.

I just thought it is right thing to do, not that I love long names. :-)

Why can't we simply remove createConfigParser method?

Remove it and see for yourself :)

Hint: the createConfigParser is pure virtual method in base class. It's also being used in D2 *in this version of the code*. There is 5110 ticket that was merged that changed those areas of the code. So to not make the merge any more difficult, it's better to leave this as is. Once both this and 5110 are merged, DCfgMgr class restructuring can be done and this method would be removed.

src/bin/agent/simple_parser.cc
This file includes some headers which probably aren't required. I am specifically thinking about DhcpConfigError and boost/foreach.hpp

setAllDefaults contains the following:

if (d2) {
   cnt += SimpleParser::setDefaults(d6, SOCKET_DEFAULTS);
}

The "d2" is probably a typo or copy-paste error?

Yes. Good catch.

I am also wondering why unit tests didn't catch that?

Perhaps because unit-tests don't test every possible parameter combination? This particular piece of code is tested by 2 unit tests: AgentParserTest?.configParse1Socket and AgentParserTest?.configParse3Sockets. Added two more unit-tests and fixed the bug.

src/bin/agent/simple_parser.h
There seem to be copy-paste errors in the AgentSimpleParser description, as it talks about DHCPv4 specific parser.

Oops. Corrected.

I am guessing that we may get a lot of cppcheck warning about objects not being passed by reference but by value.

Not anymore :)

src/bin/agent/tests/Makefile.am
While I don't object to the rename of ctrl_agent_unittest to run_unittests, I believe that such changes should rather be done in separate ticket after all other tickets (related to CA in this case) have been merged. Usually, such changes cause annoying conflicts between tickets being implemented at the same time and make it easy to miss some unit tests when conflicts are being resolved.

As you renamed the binary, please make sure that the .gitignore is properly updated too.

Updated.

src/bin/agent/tests/basic_library.cc
You probably don't need to include <fstream>

Removed.

src/bin/agent/tests/test_libraries.h.in
Instead of creating this header file, couldn't you simply pass the define from Makefile.am using -D compiler switch?

For just one library you would be right, but you need to ask a question whether you think that's the only test library we will ever have for control agent. If in doubt, take a look at src/lib/hooks/test_libraries.h.in. :)

Also, if keeping this file anyway, the .gitignore should be updated.

Updated.

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
As the Hooks libraries parser has been moved to another library, wouldn't it make sense to move hooks specific unit tests from dhcpsrv to hooks?

Given infinite amount of time - yes. Unfortunately, the unit-tests in src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc are covering tons of different things and happen to be very DHCP specific. Extracting certain subset of tests may involve a lot of work for minor improvement in unit-test readability.

I am not going to insist on that, but making too many exceptions like this will almost certainly result in a mess within the code structure, e.g. to find a test for a class, grep the entire code.

src/lib/hooks/Makefile.am
src/lib/hooks/hooks_parser.cc
Implementation of HooksLibrariesParser should be enclosed in namespace isc::hooks.

Updated as suggested.

src/lib/process/d_cfg_mgr.cc
src/lib/process/d_cfg_mgr.h
We now have parseConfig and simpleParseConfig methods which essentially do similar things using old and new style. I am guessing that we keep both because D2 is still using old style config? Are we going to migrate D2 to simpleParseConfig anytime soon? Note that now we have certain log messages, e.g. DCTL_CONFIG_START etc., used in both functions which is against the guideline for having only one occurrence of a certain log message n the code. If we're going to remove parseConfig function as part of the 1.2 effort, I think it is ok to keep 2 occurrences of the same log message for now. Otherwise, we should rename one or another.

That's a question to Thomas. My perception is that simpleParseConfig is... well, simpler, better prepared for configuration checking and rollback, and does not use any intermediate storage. But I don't want to push this approach on Thomas.

We should all have the same approach. We should discuss with Thomas as soon as possible.

Also, simpleParseConfig appears to have no unit tests.

This function manages the whole configuration of the whole process. So at this level it's not unit anymore, because the unit is the whole process. It is tested, just not exactly from unit-tests for process. I did an experiment and updated simpleParseConfig to always return failed answer. As a result, number of tests in src/bin/agent failed.

But I suppose that may not be sufficient for you. So implemented some stub code and added
DStubCfgMgrTest.simpleParseConfig with extra commentary explaining why more thorough tests will not be implemented. Hope that's sufficient.

Ok, that's it. Let me know if this addresses all the comments.

Ok. So it seems we have three things left. One is to add a deep clone of the context or simply put a commentary in the code stating that it is not a deep copy. Secondly, we should plan for addressing duplicated configuration methods and avoid the duplication of log messages. Probably having a ticket in 1.2 to address it would be fine. Lastly, we should get rid of the hard coded HTTP server address and port and use the configured values in CA.

comment:15 Changed 3 years ago by tomek

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

Out of those 3 things:

  • Added a commentary to the code that it's not a deep copy.
  • Getting rid of the hard coded server address proven to be tricky, so it's now extracted to separate ticket: #5165.
  • We as a team need to discuss which of those two configuration methods we prefer. Created #5166 for that.

The code is now on master. The work is not complete, but we made a significant step in the right direction. Closing.

Note: See TracTickets for help on using tickets.