Opened 7 years ago

Closed 7 years ago

#2995 closed enhancement (fixed)

Add selected few (3-4) hooks into DHCPv6 server

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130717
Component: dhcp6 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is a sort of pilot ticket. Its goal is to implement 3-4 selected hook points to get practical understanding of the hook mechanism. Depending on how it goes, we will revisit (tweak) our hook design or will go ahead with implementation of the other hooks (see ticket #2983)

Subtickets

Attachments (3)

hooks_manager.cc (3.1 KB) - added by tomek 7 years ago.
HooksManager? from branch 2980.
hooks_manager.h (5.8 KB) - added by tomek 7 years ago.
Header from trac2980.
loadlib_changes.diff (1.4 KB) - added by stephen 7 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

Changed 7 years ago by tomek

HooksManager? from branch 2980.

Changed 7 years ago by tomek

Header from trac2980.

comment:2 Changed 7 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

The code is now ready for review. Please keep in mind that the code contains a merge from trac2980 from over a week ago. To compile this code, also hooks_manager.cc|h is needed. I did not check those files into trac2995 branch to avoid merge conflicts.

There are 4 callouts implements: pkt6_receive, subnet6_select, lease6_select, pkt6_send. That's a bare minimum that allows users to start developing their callouts.

The Hooks API for DHCPv6 document has been added to Developer's guide. Its older version on wiki (http://bind10.isc.org/wiki/DhcpHooks) will be removed as soon as the developer's guide becomes available somewhere on the web. (currently we have devel guide here: http://bind10.isc.org/docs/developers/cpp/index.html, but it is regenerated after every release).

This code must not be merged before #2980.

comment:3 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:4 follow-ups: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit commit c6b3ce6d16f92b80db77487900fb57a7a67b3203

This is a bit long, mainly because it includes some discussion of the hooks system related to changes here.

Note that some changes have been made and should be pulled before development continues.

doc/devel/mainpage.dox
src/bin/dhcp6/dhcp6_hooks.dox
I've edited these files and altered the format a bit. In particular:

  • The Doxygen tags convention appears to be the same as for method names. I've renamed the tag "hooks-dhcp6" to "hooksDhcp6". (I note that the Logging pages don't follow that convention, but an update of them is scheduled.)
  • The description for each hooks has been split, with the setting of the "skip" flag put in a separate section. (This may need to change if we alter the skip flag - see later in this comment.)
  • I've altered the reference to the general Hooks API document in anticipating of the merging of #2982.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
You may want to add the removal of the hook points to the Dhcpv4SrvTest constructor as well, in anticipation of the time hooks are added to DHCPv4.

src/bin/dhcp6/dhcp6_messages.mes
DHCP6_HOOK_SUBNET6_SELECT_SKIP: the general idea is that when the callout instructs the server to skip the next stage of processing, it is because the callout has already done that. So in the case of the skipping of subnet selection, it would be because the callout has already selected the subnet. (How/why I've no idea - but it would be logical for the selected subnet to be returned through the argument list somehow. And how the server translates what is returned into a pointer to the appropriate data structure is another issue.)

DHCP6_HOOK_PACKET_RCVD_SKIP: As noted above, the "skip" flag was intended to instruct the server to skip the processing associated with the hook· In some cases - such as here - there is no associated processing: instead, it makes sense to allow the callout to drop the packet. I suggest that the last two sentences of the explanation be replaced with "For this particular hook, the setting of the flag by a callout instructs the server to drop the packet." (But see question below.)

DHCP6_HOOK_PACKET_SEND_SKIP: suggest a similar change to the wording as for DHCP6_HOOK_PACKET_RCVD_SKIP - replace the last two sentences with something like "For this particular hook, the setting of the flag by a callout instructs the server to drop the response." (But see below.)


Question about hooks system: the "skip" flag was so-named because of anticpated use. What I hadn't considered was the possibility of dropping the packet (hence the overloaded use of the skip flag by the server in this situation). On the assumption that it seem illogical to a hook author that setting the "skip" flag causes the dropping of the packet, I think this should be changed. Although we could require that the action be returned to the server in the argument list, an easily-accessible flag or value appears to be more convenient (and faster). Would it be better to replace {get,set}SkipFlag() with something like {get,set}ReturnAction() (where the "return action" value is an "int")? Appropriate values for the return action indicator would be defined on a per-hook basis, although adopting the convention that a value of 0 means that the server should continue normal processing will simplify things.

So for the above hook points, a return value of 0 means continue and a non-zero value is interpreted as "drop the packet" and the send and receive points, and "skip the associated processing" at the subnet selection point. (Some hook points may have more than two possible return actions (e.g. drop, skip, or continues) which is why it is suggested that the the "return action" be an integer value rather than a boolean flag.)


(Incidentally, if we go for this option, the error codes above would be better named xxx_DROP instead of xxx_SKIP.)

src/bin/dhcp6/dhcp6_srv.h
Methods getServerHooks/getHooksManager. These won't be needed after #2980 is merged. (See end of this comment.)

The methods getCalloutHandle(), packetProcess{Start,End} should be documented.

src/bin/dhcp6/dhcp6_srv.cc
With the changes listed at the end of this comment, the include of server_hooks.h can be removed.

Dhcpv6Srv constructor: unless Dhcpv6Srv is a singleton created before the configuration file is read and hook libraries loaded, hook registration should not be done here. For a suggested way of registering hooks when the program is loaded, see the "Guide to Hooks for the BIND 10 Component Developer" (in the .dox file) in #2980.

Dhcpv6Srv constructor: the changes in #2980 should mean that there is no need to call HooksManager::loadLibraries(). If no libraries are loaded when something is called, the framework will set itself up with an empty set of libraries (as you have done here).

Dhcpv6Srv destructor: "todo" comment about deregistering a hook. I've been thinking this through and "deregister()" is not needed. In this case there is only one Dhcpv6Srv object which is in existence all the time the server is running, so there is no need to deregister them.


In our Jabber conversation a couple of days ago, we discussed hooks in an object such as the allocation engine, where we might have multiple allocation engines using different algorithms, and choose one based on a configuration option. However, even then deregisterHooks is not needed. There are several reasons for this:

  • Although the allocation engine object is create and destroyed when the configuration is changed, the file is loaded when the program is started. Therefore the hooks can be registered then.

For the the case where we have an alternate allocation engine available, then one of the following applies:

  • Either or both the main engine and alternate engine contain a hook that is not in the other engine. In that case, the hook should be uniquely named and can be declared at start-up. If the particular engine is not loaded, any callouts attached to that hook will not be called.
  • Both engines contain a common hook called at the same (logical) point in each of them. In this case, since the engines fit in the same place in the server, they will be derived from a common base class. The hook call can be encapsulated in a method in the common base class and the hook point registered when that the file containing the base class is loaded.
  • Both engines contain a hook with the same name but at different logical points in the code (and possibly with different arguments). This causes problems - when libraries load they identify hooks by name. What happens if the hooks library for the main engine is loaded when the alternate engine is running (or vice-versa)? The wrong callout will be called at that hooks point. We should should not really have any code with conflicting hook names.

In addition to the above, there is the issue concerning the interaction of the reconfiguration of the allocation engine with the reloading of the hooks libraries. Suppose a library containing a callout for a hook that is only in the alternate engine is loaded when the main engine is the current engine. The code for that hook won't be registered. If we now switch to the alternate engine, the hook won't be registered unless we reload the library. This could be done, bit it is messy - every time we make a switch of the allocation engine we need to reload all hooks libraries. (And it potentially applies to every case where we have alternate engines, e.g. suppose we add hooks to the database backend code?)

In summary, hook names should be registered on start-up. They are either unique to a piece of code - in which case there is no problem. Or they are common to bits of code derived from a common ancestor - in which the code should call the hook through a method in the common base class. There is no need to deregister the names.


Dhcpv6Srv::run(): "Callouts decided to skip the next processing step". The comment is incorrect: as indicated the comment (above) for DHCP6_HOOK_SUBNET6_SELECT_SKIP, callouts can skip a processing step when it makes sense to do so. In other cases - such as here - "skip" makes no sense, but "drop" does.

Dhcpv6Srv::run(): "No need to clear arguments". This is true, but the "deleteAllArguments()" call should be negligible overhead if no arguments are stored. It might be more defensive to clear them regardless. (On the other hand, callouts should only reference documented arguments. If they access something undocumented - such as an argument from a previous call that has not been deleted - we can't be held responsible if a changes to BIND 10 causes their code to fail.)

Dhcpv6Srv::run(): Would the names of the arguments to the callouts be more descriptive if named "query" and "response" rather than "pkt6"?

Dhcp6Srv general: "CalloutHandlePtr" has been defined and can be used instead of boost::shared_ptr<CalloutHandle>.

Dhcp6Srv::selectSubnet(): this has the same comment as the "run()" method - "This is the first callout so not need to clear any arguments". Both statements can't be true.

Dhcp6Srv::assignIA_NA(): there is a call to get the CalloutHandle here although the return value is not used anywhere.

Dhcpv6Srv::getCalloutHandle(): just a comment here that this scheme is only valid providing a packet is fully processed (from being received to the response being sent (or the query dropped)). This is the case for the server, but comments in the method should make the restrictions clear.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
This file is now 2,500 lines long - at some time we should break it down into smaller files.

"Hooks" test: serverHooks::getIndex() will throw a NoSuchHook exception if the hook is not registered, so these tests should really be EXPECT_NO_THROW tests.

HooksDhcpv6SrvTest class: methods should be documented, including arguments.

HooksDhcpv6SrvTest class: once #2980 is merged, there will be no need to set the library index in the constructor.

Note: once #2980 is merged, we should think of creating a shared library containing a variant of a HooksDhcpv6SrvTest object and repeat the hooks tests here using that library.

src/lib/dhcpsrv/alloc_engine.{cc,h}
See above for discussion of hook registration questions.

See below for future changes regarding getHooksManager() etc.

AllocEngine::reuseExpiredLease(): there is no need to reset the "skip" flag - that is automatically set "false" in HooksManager::callCallouts() before the first callout is called.

AllocEngine::reuseExpiredLease(): (comment before callCallouts()) is this the first callout as well?

AllocEngine::reuseExpiredLease(): the callout called is for lease6_select, yet this method is described as reusing an expired lease (and the comment before the callouts talks about lease6_ia being added). Reusing an expired lease seems to be an event that merits its own distinct callout.

AllocEngine::createLease6(): the callout called in this method has the same hook index as that in reuseExpiredLease() (as well as the same comments, including "this is the first callout". I suspect a cut-and-paste error somewhere, probably in reuseExpiredLease().

src/lib/dhcpsrv/cfgmgr.{cc,h}
getSubnets6(): for getter and setter methods that get/return a member of the class (and that are not virtual), putting the single line of the code in the header allows the compiler to inline the call and save the overhead of a call/return.

src/lib/dhcpsrv/dhcpsrv_log.h
DHCPSRV_HOOKS should be DHCPSRV_DBG_HOOKS to be consistent with other debug levels.

src/lib/dhcpsrv/dhcpsrv_messages.mes
DHCPSRV_HOOK_LEASE_IA_ADD_SKIP:
s/callout assigned on lease6_ia.../callout installed on the lease6_ia.../
s/Server will not.../The server will not.../

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
General: The EXPECT_EQ methods should have the expected value as the first argument and the value under test as the second (Google test messages are optimised for this order).

See elsewhere about deleting hook names. (Note that tests need to register them even if they have been registered in the file holding the code being tested: other tests may have deleted those names.)

various: the calls to loadLibraries will not be required after #2980 is merged.

Updates required after #2980 is merged

After #2980 is merged (assuming it gets through the review unscathed), the following changes should to be made:

1. getHooksManager() calls are no longer needed. All HooksManager instance methods have been wrapped in static methods that call the instance manager with the singleton instance. For example, you can replace:

HooksManager::getHooksManager().loadLibraries(...);

with

HooksManager::loadLibraries(...);

etc.

2. getServerHooks() calls are no longer needed except in unit tests. The main code should only be registering hooks - only the test code should be clearing the hooks list of hooks. A HooksManager wrapper around ServerHooks::registerHook() has been written so that:

ServerHooks::getServerHooks().registerHook(...);

can be replaced by a call to the static method:

HooksManager::registerHook(...);

In this way, the main code only need access the HooksManager and CalloutHandle classes (and possibly the LibraryHandle class if the server registers its own callouts.)

3. To clear the list of hooks, the unit test code will still need to call

ServerHooks::getServerHooks().reset();

This should be done in the test class constructor. (Putting a call in the destructor as well does no harm.)

4. Within the unit tests, register a callout that is defined in the test file with:

HooksManager::preCalloutsLibraryHandle().registerCallout(...)

(There is also a postCalloutsLibraryHandle() method, but unless any shared libraries are loaded, the pre- and post- methods do effectively the same thing.)

5. We should write some tests that involve loading a shared library while the test is running.

comment:5 in reply to: ↑ 4 Changed 7 years ago by tomek

Replying to stephen:

Note that some changes have been made and should be pulled before development continues.

Thank you.

doc/devel/mainpage.dox
src/bin/dhcp6/dhcp6_hooks.dox
I've edited these files and altered the format a bit. In particular:

  • The Doxygen tags convention appears to be the same as for method names. I've renamed the tag "hooks-dhcp6" to "hooksDhcp6". (I note that the Logging pages don't follow that convention, but an update of them is scheduled.)
  • The description for each hooks has been split, with the setting of the "skip" flag put in a separate section. (This may need to change if we alter the skip flag - see later in this comment.)

Thank you. It looks more clear now. I also reverted the order - it is now again in the order hook points will be called in packet processing. Also corrected skip flag meaning between receive and skip. The difference is that skip flag in send hook will not send out the response packet, but the server will have this updated state (e.g. allocated a lease).

  • I've altered the reference to the general Hooks API document in anticipating of the merging of #2982.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc
You may want to add the removal of the hook points to the Dhcpv4SrvTest constructor as well, in anticipation of the time hooks are added to DHCPv4.

No. It is added to destructor already. If I added it to contructor, it would break down the tests once Dhcpv4Srv gets hooks support. Dhcpv4SrvTest constructor is called after Dhcpv4Srv constructor, so adding reset() to the latter would effectively get rid of all hook points. That's why it has to be added to destructor. BTW Dhcpv4Srv class uses AllocEngine? that registers lease6_select hook. Obviously it won't be used, but the registration takes place nevertheless. That's why reset() in Dhcpv4SrvTest is required now.

src/bin/dhcp6/dhcp6_messages.mes
DHCP6_HOOK_SUBNET6_SELECT_SKIP: the general idea is that when the callout instructs the server to skip the next stage of processing, it is because the callout has already done that. So in the case of the skipping of subnet selection, it would be because the callout has already selected the subnet. (How/why I've no idea - but it would be logical for the selected subnet to be returned through the argument list somehow. And how the server translates what is returned into a pointer to the appropriate data structure is another issue.)

I understand the general idea, but I tried to offer as much flexibility as possible. The callout could use some extra information (e.g. options added by relays, like subscriber-id or docsis3.0 options) to pick a different subnet. Obviously, a callout can set subnet6 to Subnet6Ptr(), effectively not selecting a subnet. But I thought that using skip flag for that purpose would be cleaner and more explicit way of not selecting a subnet at all.

DHCP6_HOOK_PACKET_RCVD_SKIP: As noted above, the "skip" flag was intended to instruct the server to skip the processing associated with the hook· In some cases - such as here - there is no associated processing: instead, it makes sense to allow the callout to drop the packet. I suggest that the last two sentences of the explanation be replaced with "For this particular hook, the setting of the flag by a callout instructs the server to drop the packet." (But see question below.)

But how can you tell a server to skip receiving a packet that it passed to you? There will be another callout for that - buffer6_received that is called after read from sockets. It will also have skip flag that will instruct the server to drop the packet.

DHCP6_HOOK_PACKET_SEND_SKIP: suggest a similar change to the wording as for DHCP6_HOOK_PACKET_RCVD_SKIP - replace the last two sentences with something like "For this particular hook, the setting of the flag by a callout instructs the server to drop the response." (But see below.)

There's a difference between RCVD_SKIP (server not processed anything) and SEND_SKIP (server processed the request packet and possibly allocated a lease, just the change was not communicated to the client). I used
your proposed sentence, but kept the existing explanation as well (with a small tweak).


Question about hooks system: the "skip" flag was so-named because of anticpated use. What I hadn't considered was the possibility of dropping the packet (hence the overloaded use of the skip flag by the server in this situation). On the assumption that it seem illogical to a hook author that setting the "skip" flag causes the dropping of the packet, I think this should be changed. Although we could require that the action be returned to the server in the argument list, an easily-accessible flag or value appears to be more convenient (and faster). Would it be better to replace {get,set}SkipFlag() with something like {get,set}ReturnAction() (where the "return action" value is an "int")? Appropriate values for the return action indicator would be defined on a per-hook basis, although adopting the convention that a value of 0 means that the server should continue normal processing will simplify things.

So for the above hook points, a return value of 0 means continue and a non-zero value is interpreted as "drop the packet" and the send and receive points, and "skip the associated processing" at the subnet selection point. (Some hook points may have more than two possible return actions (e.g. drop, skip, or continues) which is why it is suggested that the the "return action" be an integer value rather than a boolean flag.)

There is no clear consensus on this one. You seem to prefer int returnAction and I prefer bool skip. I asked Marcin and Tom, and Tom has mixed feelings. For the sake of not holding back the development, I propose to keep the code as it is and revisit this topic later after we get some comments from external users. I have updated #3037 with this idea.

src/bin/dhcp6/dhcp6_srv.h
Methods getServerHooks/getHooksManager. These won't be needed after #2980 is merged. (See end of this comment.)

The methods getCalloutHandle(), packetProcess{Start,End} should be documented.

No. They should be removed as they are not used and not implemented. They are a leftover from my initial idea that I forgot to remove. They are gone now.

Review will continue on Monday.

comment:6 in reply to: ↑ 4 Changed 7 years ago by tomek

Replying to stephen:

Reviewed commit commit c6b3ce6d16f92b80db77487900fb57a7a67b3203

Continuing after 5

src/bin/dhcp6/dhcp6_srv.cc
With the changes listed at the end of this comment, the include of server_hooks.h can be removed.

Changes applied, include removed.

Dhcpv6Srv constructor: unless Dhcpv6Srv is a singleton created before the configuration file is read and hook libraries loaded, hook registration should not be done here. For a suggested way of registering hooks when the program is loaded, see the "Guide to Hooks for the BIND 10 Component Developer" (in the .dox file) in #2980.

Dhcpv6Srv is not a singleton, but there is never more than once instance at any given time. It is created before configuration file is read and libraries are loaded. Anyway, the registration now follows the Guide approach.

Dhcpv6Srv constructor: the changes in #2980 should mean that there is no need to call HooksManager::loadLibraries(). If no libraries are loaded when something is called, the framework will set itself up with an empty set of libraries (as you have done here).

Hmm, I've commented it out and now 3 tests fail:
[ FAILED ] HooksDhcpv6SrvTest.simple_pkt6_send
[ FAILED ] HooksDhcpv6SrvTest.valueChange_pkt6_send
[ FAILED ] HooksDhcpv6SrvTest.deleteServerId_pkt6_send
I do not understand why that is happening. I left it as it is now.

Dhcpv6Srv destructor: "todo" comment about deregistering a hook. I've been thinking this through and "deregister()" is not needed. In this case there is only one Dhcpv6Srv object which is in existence all the time the server is running, so there is no need to deregister them.

Ok, todo removed.


In our Jabber conversation a couple of days ago, we discussed hooks in an object such as the allocation engine, where we might have multiple allocation engines using different algorithms, and choose one based on a configuration option. However, even then deregisterHooks is not needed. There are several reasons for this:

  • Although the allocation engine object is create and destroyed when the configuration is changed, the file is loaded when the program is started. Therefore the hooks can be registered then.

For the the case where we have an alternate allocation engine available, then one of the following applies:
...

Thanks for the thorough explanation. Consider myself convinced.

In summary, hook names should be registered on start-up. They are either unique to a piece of code - in which case there is no problem. Or they are common to bits of code derived from a common ancestor - in which the code should call the hook through a method in the common base class. There is no need to deregister the names.

That makes sense. Hook registration in AllocEngine? now follows the Guide.

Changes checked in (commit-it 682cac0da7b318a81746d93424a638faa867ee7c)

The review response will resume tomorrow.

comment:7 in reply to: ↑ 4 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit commit c6b3ce6d16f92b80db77487900fb57a7a67b3203

Review continues. See comment:5 and comment:6 for previous parts.

Dhcpv6Srv::run(): "Callouts decided to skip the next processing step". The comment is incorrect: as indicated the comment (above) for DHCP6_HOOK_SUBNET6_SELECT_SKIP, callouts can skip a processing step when it makes sense to do so. In other cases - such as here - "skip" makes no sense, but "drop" does.

I decided to keep the "skip" flag for now. If we decide later to implement int returnAction, we will do so at a later date.

Dhcpv6Srv::run(): "No need to clear arguments". This is true, but the "deleteAllArguments()" call should be negligible overhead if no arguments are stored. It might be more defensive to clear them regardless. (On the other hand, callouts should only reference documented arguments. If they access something undocumented - such as an argument from a previous call that has not been deleted - we can't be held responsible if a changes to BIND 10 causes their code to fail.)

deleteAllArguments() is now called. Also the comment about being first callout removed. We will soon implement buffer6_receive, which will be called before pkt6_receive, so the comment would be incorrect, but we would probably forget to update it.

Dhcpv6Srv::run(): Would the names of the arguments to the callouts be more descriptive if named "query" and "response" rather than "pkt6"?

That way you can't reuse the same method for both pkt6_send and pkt6_receive. Anyway, I've changed the code as you suggested.

Dhcp6Srv general: "CalloutHandlePtr" has been defined and can be used instead of boost::shared_ptr<CalloutHandle>.

I hope I spotted all the instances. Let me know if I missed anything.

Dhcp6Srv::selectSubnet(): this has the same comment as the "run()" method - "This is the first callout so not need to clear any arguments". Both statements can't be true.

Copy and paste error. Fixed.

Dhcp6Srv::assignIA_NA(): there is a call to get the CalloutHandle here although the return value is not used anywhere.

Look closer. :) It is passed to allocateAddress6(). This is the last place where we have access to packet. Allocation engine operates on addresses, subnets and leases it knows nothing about packets.

Dhcpv6Srv::getCalloutHandle(): just a comment here that this scheme is only valid providing a packet is fully processed (from being received to the response being sent (or the query dropped)). This is the case for the server, but comments in the method should make the restrictions clear.

Explanatory comment added.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
This file is now 2,500 lines long - at some time we should break it down into smaller files.

Yes, but not within this ticket. It is already too complex - 2 large merges from another branch. Please create a separate refactoring ticket if you want to pursue this matter further.

"Hooks" test: serverHooks::getIndex() will throw a NoSuchHook exception if the hook is not registered, so these tests should really be EXPECT_NO_THROW tests.

It doesn't make much difference as exception thrown will fail the test anyway. But I modified the code as you suggested.

HooksDhcpv6SrvTest class: methods should be documented, including arguments.

It is now documented.

HooksDhcpv6SrvTest class: once #2980 is merged, there will be no need to set the library index in the constructor.

I can't find specific code that sets library index. I must have removed it when tweaking code for the second merge.

Note: once #2980 is merged, we should think of creating a shared library containing a variant of a HooksDhcpv6SrvTest object and repeat the hooks tests here using that library.

We may, but I thought that will be part of the tests that QA is supposed to be doing.

src/lib/dhcpsrv/alloc_engine.{cc,h}
See above for discussion of hook registration questions.

Done.

AllocEngine::reuseExpiredLease(): there is no need to reset the "skip" flag - that is automatically set "false" in HooksManager::callCallouts() before the first callout is called.

Removed.

AllocEngine::reuseExpiredLease(): (comment before callCallouts()) is this the first callout as well?

Another copy and paste error. Fixed.

AllocEngine::reuseExpiredLease(): the callout called is for lease6_select, yet this method is described as reusing an expired lease (and the comment before the callouts talks about lease6_ia being added). Reusing an expired lease seems to be an event that merits its own distinct callout.

No. I consider the fact that we are reusing expired lease a very internal things. For all intents and purposes, we are allocating a lease for client. The fact that we reuse a lease is our internal optimization and honestly speaking a workaround for lack of housekeeping process.

If you really think that we should expose that information, then we can perhaps add a flag that states whether this is a new or reused lease.

AllocEngine::createLease6(): the callout called in this method has the same hook index as that in reuseExpiredLease() (as well as the same comments, including "this is the first callout". I suspect a cut-and-paste error somewhere, probably in reuseExpiredLease().

No. That is by design. These are two points where server allocates leases. It is very internal matter. For users it is sufficient to know that this lease going to be used. The previous history is irrelevant.

src/lib/dhcpsrv/cfgmgr.{cc,h}
getSubnets6(): for getter and setter methods that get/return a member of the class (and that are not virtual), putting the single line of the code in the header allows the compiler to inline the call and save the overhead of a call/return.

Done. Added explicit inline.


src/lib/dhcpsrv/dhcpsrv_log.h
DHCPSRV_HOOKS should be DHCPSRV_DBG_HOOKS to be consistent with other debug levels.

Done.

src/lib/dhcpsrv/dhcpsrv_messages.mes
DHCPSRV_HOOK_LEASE_IA_ADD_SKIP:
s/callout assigned on lease6_ia.../callout installed on the lease6_ia.../
s/Server will not.../The server will not.../

Done.


src/lib/dhcpsrv/tests/alloc_engine_unittest.cc
General: The EXPECT_EQ methods should have the expected value as the first argument and the value under test as the second (Google test messages are optimised for this order).

Done, except one case where false being first causes the code to not compile.

various: the calls to loadLibraries will not be required after #2980 is merged.

Ok. We will clean this up in the next ticket.

Ok, the code is back with you. Thanks for thorough review.

comment:8 Changed 7 years ago by jwright

  • Summary changed from Add selected few (3-4) hoots into DHCPv6 server to Add selected few (3-4) hooks into DHCPv6 server

Changed 7 years ago by stephen

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

  • Owner changed from stephen to tomek

Reviewed commit 72b06693a049e925d240971d92dd8427d3fa8f73

I've made some editorial changes to the dhcp6_hooks.dox file - please pull before doing any modifications.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

You may want to add the removal of the hook points to the Dhcpv4SrvTest constructor as well, in anticipation of the time hooks are added to DHCPv4.

No. It is added to destructor already. If I added it to contructor, it would break down the tests once Dhcpv4Srv gets hooks support. Dhcpv4SrvTest constructor is called after Dhcpv4Srv constructor, so adding reset() to the latter would effectively get rid of all hook points.

Is it? The Dhcpv4SrvTest is a class used by Gtest and instantiated for the test. Unless there is a static declaration of Dhcpv4Srv somewhere, the test class will be instantiated before the object being tested. However, if the hooks points are statically registered (as they are), there is no need to delete them in the test so the whole point is moot.

Question about hooks system: the "skip" flag was so-named because of anticpated use. What I hadn't considered was the possibility of dropping the packet (hence the overloaded use of the skip flag by the server in this situation). On the assumption that it seem illogical to a hook author that setting the "skip" flag causes the dropping of the packet

For the sake of not holding back the development, I propose to keep the code as it is and revisit this topic later after we get some comments from external users.

The code has been left as-is. In the latest version of the user guide (#2982), I've described that setting the flag causes the server to skip normal processing (so including the possibility of dropping the packet).

src/bin/dhcp6/dhcp6_hooks.dox
subnet6_select: the documentation mentions that the Subnet6Selection is provided as a "const reference". As the information is copied across to the CallHandle object when setArgument is called and copied from it when getArgument is called, I think that what is happening is that the callout is getting a copy of the Subnet6Collection, not a reference to it. If you want to pass an object without copying to the callout, and do not want the callout to modify it, you need to pass a "pointer to const".

Note: In editing this, I added links to the Pkt6 class. On checking them, I noticed that the class does not have a header (hence the Doxygen HTML entry does not have a class description). I also noticed that Pkt4 does not have a class header. Can you create a ticket to add ones.

src/bin/dhcp6/dhcp6_srv.cc
There is no need to call "setSkip(false)" before calling the callouts - callCallouts does that.

(Although I've just noticed that if there are no callouts attached to a hook and callCallouts is called, the flag is not cleared - I've raised #3050 for that. This bug does not affect the code here, because the server checks that callouts are present first.)

Dhcpv6Srv constructor: the changes in #2980 should mean that there is no need to call HooksManager::loadLibraries(). If no libraries are loaded when something is called, the framework will set itself up with an empty set of libraries (as you have done here).

Hmm, I've commented it out and now 3 tests fail:
...

The reason the tests were failing is that the the callouts registered by earlier tests were not cleared. The call in the Dhcpv6Srv constructor cleared them when the object was instantiated at the start of a test. I've attached a patch file to the ticket that will clear registered hooks at the end of each test.

As an aside, there is the comment:

/// @todo call loadLibraries() when handling configuration changes

Where is the best place to do this, as I am aiming to do it as part of #2981? The issue with memory allocated within a callout means that it can only be done where all structures associated with the previous packet have been destroyed. This appears to be either:

  • In Dhcp6Srv::run(), at the head of the run() loop. (The loading of the libraries will only take place if the configuration has changed, not on every packet).
  • In configureDhcp6Server(). (This assumes that configureDhcp6Server can only be called from the main select/asio loop. In other words, it assumes that Dhcp6Srv::run is at the point where it is waiting for a new packet, so everything associated with the previous packet has been destroyed.)

getCalloutHandle(): we may want something that will allow the deletion of the stored pointer. If the libraries are reloaded, the way the hooks code is written will mean that the old libraries will not be unloaded until the stored CalloutHandle is destroyed. If there is a problem, the deferred deletion may make it difficult to debug.

src/lib/dhcpsrv/alloc_engine.cc
reuseExpiredLease(): with the changes to the hooks manager:

HooksManager::getHooksManager()::callCallouts(...)

can be replaced with

HooksManager::callCallouts(...)

src/lib/dhcpsrv/cfgmgr.h
White space is incorrect for getSubnets6().

comment:10 in reply to: ↑ 9 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit 72b06693a049e925d240971d92dd8427d3fa8f73

I've made some editorial changes to the dhcp6_hooks.dox file - please pull before doing any modifications.

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

You may want to add the removal of the hook points to the Dhcpv4SrvTest constructor as well, in anticipation of the time hooks are added to DHCPv4.

No. It is added to destructor already. If I added it to contructor, it would break down the tests once Dhcpv4Srv gets hooks support. Dhcpv4SrvTest constructor is called after Dhcpv4Srv constructor, so adding reset() to the latter would effectively get rid of all hook points.

Is it? The Dhcpv4SrvTest is a class used by Gtest and instantiated for the test. Unless there is a static declaration of Dhcpv4Srv somewhere, the test class will be instantiated before the object being tested. However, if the hooks points are statically registered (as they are), there is no need to delete them in the test so the whole point is moot.

Sorry for the confusion. Here's the explanation what had happened. At first I intended to keep the hooks as they were before - register them in Dhcpv4Srv constructor (or to be more precise - in AllocEngine? constructor that is called from Dhcpv4Srv constructor) and call reset() in Dhcpv4SrvTest destructor (so once the next test instantiates the Dhcpv4Srv class it could be able to register properly). Then while I was working through your comments I got convinced by your arguments and rewrote hook registration to work as recommended by the hooks guide. Finally, I forgot to update this comment.

Anyway, I understand the difference between Dhcp4SrvTest and Dhcpv4Srv classes. The ideal C++ approach would be to undo in destructor whatever was done in consutructor. However, since there was no unregisterHook() method, I couldn't call reset() in Dhcpv4Srv destructor as that may have impacted other components. In any case, that is no longer relevant, as the registration is done statically just once and there is no need to use reset() anymore.

Question about hooks system: the "skip" flag was so-named because of anticpated use. What I hadn't considered was the possibility of dropping the packet (hence the overloaded use of the skip flag by the server in this situation). On the assumption that it seem illogical to a hook author that setting the "skip" flag causes the dropping of the packet

For the sake of not holding back the development, I propose to keep the code as it is and revisit this topic later after we get some comments from external users.

The code has been left as-is. In the latest version of the user guide (#2982), I've described that setting the flag causes the server to skip normal processing (so including the possibility of dropping the packet).

Great! Thank you.

I think at the end, this is up to the definition of 'skip' flag. Forget any documentation for a moment. What do you think "skip this incoming packet" could mean to the server? Anyway, we'll probably get back to this once we get some external reviewers.

src/bin/dhcp6/dhcp6_hooks.dox
subnet6_select: the documentation mentions that the Subnet6Selection is provided as a "const reference". As the information is copied across to the CallHandle object when setArgument is called and copied from it when getArgument is called, I think that what is happening is that the callout is getting a copy of the Subnet6Collection, not a reference to it. If you want to pass an object without copying to the callout, and do not want the callout to modify it, you need to pass a "pointer to const".

I've updated it to use const Subnet6Selection*. I hope I haven't messed up const pointer to variable object and variable pointer to const object this time.

Note: In editing this, I added links to the Pkt6 class. On checking them, I noticed that the class does not have a header (hence the Doxygen HTML entry does not have a class description). I also noticed that Pkt4 does not have a class header. Can you create a ticket to add ones.

Sure. Created ticket #3053.

src/bin/dhcp6/dhcp6_srv.cc
There is no need to call "setSkip(false)" before calling the callouts - callCallouts does that.

(Although I've just noticed that if there are no callouts attached to a hook and callCallouts is called, the flag is not cleared - I've raised #3050 for that. This bug does not affect the code here, because the server checks that callouts are present first.)

Removed.

Dhcpv6Srv constructor: the changes in #2980 should mean that there is no need to call HooksManager::loadLibraries(). If no libraries are loaded when something is called, the framework will set itself up with an empty set of libraries (as you have done here).

Hmm, I've commented it out and now 3 tests fail:
...

The reason the tests were failing is that the the callouts registered by earlier tests were not cleared. The call in the Dhcpv6Srv constructor cleared them when the object was instantiated at the start of a test. I've attached a patch file to the ticket that will clear registered hooks at the end of each test.

Thank you. Patch applied.

As an aside, there is the comment:

/// @todo call loadLibraries() when handling configuration changes

Where is the best place to do this, as I am aiming to do it as part of #2981? The issue with memory allocated within a callout means that it can only be done where all structures associated with the previous packet have been destroyed. This appears to be either:

  • In Dhcp6Srv::run(), at the head of the run() loop. (The loading of the libraries will only take place if the configuration has changed, not on every packet).

No, that's not the right place. Loading libraries is part of configuration (a fancy one, I admit) and it should be done in the same place as every other configuration change.

  • In configureDhcp6Server(). (This assumes that configureDhcp6Server can only be called from the main select/asio loop. In other words, it assumes that Dhcp6Srv::run is at the point where it is waiting for a new packet, so everything associated with the previous packet has been destroyed.)

That is the place to load libraries.

getCalloutHandle(): we may want something that will allow the deletion of the stored pointer. If the libraries are reloaded, the way the hooks code is written will mean that the old libraries will not be unloaded until the stored CalloutHandle is destroyed. If there is a problem, the deferred deletion may make it difficult to debug.

Part of the #2981 should be to extend getCalloutHandle to release existing CalloutHandle? when called with NULL parameter. You could then call it before unloading libraries.

src/lib/dhcpsrv/alloc_engine.cc
reuseExpiredLease(): with the changes to the hooks manager:

HooksManager::getHooksManager()::callCallouts(...)

can be replaced with

HooksManager::callCallouts(...)

Done (also in dhcp6_srv.cc)

src/lib/dhcpsrv/cfgmgr.h
White space is incorrect for getSubnets6().

Fixed.

Also added the following changes:

  • removed Dhcp6Srv::getServerHooks() and getHooksManager() declarations.
  • updated sendPacket comment (reception->transmission)
  • documented getCalloutHandle()

I think that addresses all issues. Unless there are new issues spotted, I think this is finally ready to merge.

comment:11 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit d6de376f97313ba40fef989e4a437d184fdf70cc

Unless there are new issues spotted, I think this is finally ready to merge.

With the exception of a ChangeLog entry, it is. I suggest something like:

Added initial set of hooks to the DHCPv4 server.

comment:12 Changed 7 years ago by tomek

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

Merged. Updated ChangeLog? and libb10-hooks dependencies in src/bin/d2 and src/lib/dhcpsrv.

Note: See TracTickets for help on using tickets.