Opened 7 years ago

Closed 5 years ago

#3070 closed enhancement (complete)

Kea should support rapid-commit option for DHCPv6

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Kea0.9.2-beta
Component: dhcp6 Version: git
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

See RFC3315, section 22.14.

The option format is easy. The tricky part is to respond to SOLICIT with REPLY, not with ADVERTISE.

Subtickets

Change History (10)

comment:1 Changed 5 years ago by marcin

  • Milestone changed from DHCP Outstanding Tasks to Kea-proposed
  • Version set to git

I am moving this to the "proposed" queue for Kea team to discuss. We've received reports that this feature is desired. At the same time not terribly complicated.

comment:2 Changed 5 years ago by marcin

In order to support it, we need:

  • Configuration parameter to say whether Rapid Commit is accepted or not
  • Logic in the processSolicit which reacts to the presence of the Rapid Commit option and changes the message type to Reply, and the fake_allocation flag in the processing complex to false.
  • Bunch of unit tests to test it
  • Update documentation.

My estimate for this work, including the review, is 3 days.

comment:3 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.2

comment:4 Changed 5 years ago by marcin

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

comment:5 Changed 5 years ago by marcin

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

This ticket is ready for review.

ChangeLog entry:

XXX.	[func]		marcin
	DHCPv6 server now supports Rapid Commit option.
	(Trac 3070, git abcd)

comment:6 Changed 5 years ago by sar

  • Owner changed from UnAssigned to sar

comment:7 follow-up: Changed 5 years ago by sar

  • Owner changed from sar to marcin

General

Should there be a global option to allow rapid-commit to be enabled for all subnets? This would probably also lead to a desire for the option to be disabled on a per subnet basis.

I made a couple of typo level changes, please pull them and check them out.

doc/guide/dhcp6-srv.xml

I would change

This setting is solely effective for a subnet for which the 

to be

This setting only affects the subnet for which the

src/bin/dhcp6/dhcp6_messages.mes

Shouldn't the message be one earlier in the file to be in alphabetical order?
DHCP6_RAPID_COMMIT before DHCP6_RELEASE_MISSING_CLIENTID

src/bin/dhcp6/dhcp6_srv.cc

Please update the comments for setting fake_allocation in assignIA_NA and assignIA_PD to better describe what is happening with SOLICITs and RAPID-COMMIT.

src/bin/dhcp6/tests/config_parser_unittest.cc

It's probably redundant but it would be good to add a second instance().clear() at the end of testRapidCommit().

In subnetRapidCommit() it would be nice to be consistent with the placement of the comment describing the test and the SCOPED_TRACE statement.

src/bin/dhcp6/tests/sarr_unittest.cc

There are four combinations of rapid-commit flags client include/not-include and server enable/disable. The current testing covers include & enable and include & disable should we also check that not-include works properly when the server has the option enabled or disabled? (Or is that tested elsewhere?)

I haven't figured out if there is any way for one subnet to "interfere" with another but we do mention it in the documentation (that enabling rapid-commit on one subnet doesn't affect other subnets). Is it worthwhile testing that? Have two subnets one enabled one not and have different clients try to get a lease from both?

src/lib/dhcpsrv/subnet.h

Should there be a description of the default value of rapid_commit_ somewhere?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by marcin

  • Owner changed from marcin to sar

Replying to sar:

General

Should there be a global option to allow rapid-commit to be enabled for all subnets? This would probably also lead to a desire for the option to be disabled on a per subnet basis.

I am not convinced. It seems to me that Rapid Commit is not widely used feature. Using this feature means that there should be only one server present in the particular network to avoid situations when multiple servers commit lease to the same client. Therefore, the rapid-commit should be enabled carefully, rather than be enabled for all subnets all at once.

I think I'd wait for users to ask for this being global parameter and it will not be terribly complicated to make it global.

I made a couple of typo level changes, please pull them and check them out.

You might have forgotten to push your changes.

doc/guide/dhcp6-srv.xml

I would change

This setting is solely effective for a subnet for which the 

to be

This setting only affects the subnet for which the

Changed.

src/bin/dhcp6/dhcp6_messages.mes

Shouldn't the message be one earlier in the file to be in alphabetical order?
DHCP6_RAPID_COMMIT before DHCP6_RELEASE_MISSING_CLIENTID

It was unintentional. I intended to put it in the alphabetical order but it seems I may need to go back to primary school to learn to do it right ;-)

src/bin/dhcp6/dhcp6_srv.cc

Please update the comments for setting fake_allocation in assignIA_NA and assignIA_PD to better describe what is happening with SOLICITs and RAPID-COMMIT.

Updated.

src/bin/dhcp6/tests/config_parser_unittest.cc

It's probably redundant but it would be good to add a second instance().clear() at the end of testRapidCommit().

Added.

In subnetRapidCommit() it would be nice to be consistent with the placement of the comment describing the test and the SCOPED_TRACE statement.

Updated.

src/bin/dhcp6/tests/sarr_unittest.cc

There are four combinations of rapid-commit flags client include/not-include and server enable/disable. The current testing covers include & enable and include & disable should we also check that not-include works properly when the server has the option enabled or disabled? (Or is that tested elsewhere?)

I added another test case to cover the situation when the server is configured to use Rapid Commit, but the client doesn't supply the Rapid Commit option. The forth case is covered in many other cases (unrelated to Rapid Commit).

I haven't figured out if there is any way for one subnet to "interfere" with another but we do mention it in the documentation (that enabling rapid-commit on one subnet doesn't affect other subnets). Is it worthwhile testing that? Have two subnets one enabled one not and have different clients try to get a lease from both?

I modified the configuration used in the sarr_unittest.cc to have two subnets - one with rapid-commit set to true, another one with rapid-commit set to false. There are two distinict tests, where client is attached to each of those subnets and requests Rapid Commit. In one case it goes through 2-way exchange, on other case server returns Advertise. I think it covers what you're asking for. I don't think there is a need to create another test where two clients connected to different subnet ask simultaneously for Rapid Commit.

src/lib/dhcpsrv/subnet.h

Should there be a description of the default value of rapid_commit_ somewhere?

Included the default in the comment describing the rapid_commit_.

comment:9 in reply to: ↑ 8 Changed 5 years ago by sar

  • Owner changed from sar to marcin

The changes look fine with one minor item below. I don't think I need to review the changes for that item and the ChangeLog? above looks fine.

Replying to marcin:

Replying to sar:

General

Should there be a global option to allow rapid-commit to be enabled for all subnets? This would probably also lead to a desire for the option to be disabled on a per subnet basis.

I am not convinced. It seems to me that Rapid Commit is not widely used feature. Using this feature means that there should be only one server present in the particular network to avoid situations when multiple servers commit lease to the same client. Therefore, the rapid-commit should be enabled carefully, rather than be enabled for all subnets all at once.

I think I'd wait for users to ask for this being global parameter and it will not be terribly complicated to make it global.

We can do that.

I made a couple of typo level changes, please pull them and check them out.

You might have forgotten to push your changes.

Yes I did forget to push them, I have now tried to merge your changes and push mine back. Hopefully it all worked properly.

<snip>

src/bin/dhcp6/tests/sarr_unittest.cc

There are four combinations of rapid-commit flags client include/not-include and server enable/disable. The current testing covers include & enable and include & disable should we also check that not-include works properly when the server has the option enabled or disabled? (Or is that tested elsewhere?)

I added another test case to cover the situation when the server is configured to use Rapid Commit, but the client doesn't supply the Rapid Commit option. The forth case is covered in many other cases (unrelated to Rapid Commit).

I haven't figured out if there is any way for one subnet to "interfere" with another but we do mention it in the documentation (that enabling rapid-commit on one subnet doesn't affect other subnets). Is it worthwhile testing that? Have two subnets one enabled one not and have different clients try to get a lease from both?

I modified the configuration used in the sarr_unittest.cc to have two subnets - one with rapid-commit set to true, another one with rapid-commit set to false. There are two distinict tests, where client is attached to each of those subnets and requests Rapid Commit. In one case it goes through 2-way exchange, on other case server returns Advertise. I think it covers what you're asking for. I don't think there is a need to create another test where two clients connected to different subnet ask simultaneously for Rapid Commit.

The tests look fine. There is still a configuration block for configs[2] which should either be described, possibly as "for future use" or probably better to simply delete it if it isn't being used. I didn't see any use of it but maybe I missed one.

<snip>

comment:10 Changed 5 years ago by marcin

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

Merged with commit a6b6156aaa95ab74c69a537e90483f82e9fbe4a2

Note: See TracTickets for help on using tickets.