Opened 5 years ago

Closed 4 years ago

#3795 closed enhancement (complete)

Collect packet statistics in the DHCPv6 code

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

Once #3793 is implemented, we need to extend DHCPv6 server code to collect per packet type statistics.

Subtickets

Change History (6)

comment:1 Changed 5 years ago by tomek

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

comment:2 Changed 4 years ago by tomek

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

The code is now ready for review.

Proposed ChangeLog? entry:

9XX.	[func]		tomek
	16 new statistics added in DHCPv6 server: generic packet counters,
	per packet type counters, parsing failure and packet drops.
	(Trac #3795, git tbd)

Note: this is a v6 equivalent of #3794.

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

  • Owner changed from UnAssigned to tomek

General

I fixed some typo level items, please do a pull.

doc/guide/dhcp4-srv.xml

The name of the statistic subnet[id].assigned-addresses in the stats table is in italics

doc/guide/dhcp6-srv.xml

While I don't think it matters to the output and don't think it needs to be addressed in this ticket can we pick one style for indentation in <para> </para> blocks? In the section "Starting and Stoping the DHCPv6 Server" we have three different indent styles 0, 6, and 2 characters from the beginning of the <para> tag.

The text for pkt6-release-received states that it "is expected to grow every time a device is shutdown in the network". I worry that will provide an incorrect expectation that most clients will do a release when they probably won't. Changing this isn't required.

I've changed pkt6-advertiser-sent (with an r) to pkt6-advertise-sent as that seems to be the correct name.

Another minor item - I would move the blocks pkt6-receive-drop and pkt6-parse-failed to be at the top of the table just after the pkt-received to try and group the labels.

src/bin/dhcp6/dhcp6_srv.cc

Any reason not to do "using namespace isc::stats;" ?

As a general thing (which isn't required for this ticket, maybe another ticket for more stats work?) I would argue for the different drops to have their own counters so the admin can easily see why packets are being dropped. I would also suggest (again in future work) to add at least a drop counter for callouts.

Should there be counters for: 1) packets we tried to send but failed for some reason, 2) packets for which we failed when trying to create the on wire representation (call rsp->pack() and catching an error). For the second it would seem that we should at least count the received dropped counter.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

The test for receiveReplyStat is actually checking pkt6-advertise-received (probably copy and paste error)

pkt6-receive-drop can be incremented in 5 places but the test only checks for one - if the parse fails. Should there be more tests to check if the packet was dropped for other reasons?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by tomek

  • Owner changed from tomek to sar

Replying to sar:

General

I fixed some typo level items, please do a pull.

Thanks.

doc/guide/dhcp4-srv.xml

The name of the statistic subnet[id].assigned-addresses in the stats table is in italics.

Not anymore.

doc/guide/dhcp6-srv.xml

While I don't think it matters to the output and don't think it needs to be addressed in this ticket can we pick one style for indentation in <para> </para> blocks? In the section "Starting and Stoping the DHCPv6 Server" we have three different indent styles 0, 6, and 2 characters from the beginning of the <para> tag.

There's a ticket #3872 (planned for 0.9.2-final). I added a remark there about indentation.

The text for pkt6-release-received states that it "is expected to grow every time a device is shutdown in the network". I worry that will provide an incorrect expectation that most clients will do a release when they probably won't. Changing this isn't required.

Updated the text. Hopefully it's better now.

I've changed pkt6-advertiser-sent (with an r) to pkt6-advertise-sent as that seems to be the correct name.

Thanks.

Another minor item - I would move the blocks pkt6-receive-drop and pkt6-parse-failed to be at the top of the table just after the pkt-received to try and group the labels.

Moved. Initially I thought that the list should be roughly from most to least frequently appearing. But I have no strong opinion on that, so updated as suggested.

src/bin/dhcp6/dhcp6_srv.cc

Any reason not to do "using namespace isc::stats;" ?

Not really. Added using namespace.

As a general thing (which isn't required for this ticket, maybe another ticket for more stats work?) I would argue for the different drops to have their own counters so the admin can easily see why packets are being dropped. I would also suggest (again in future work) to add at least a drop counter for callouts.

Created ticket #3912 and #3914 for that, referencing your comment here.

Should there be counters for: 1) packets we tried to send but failed for some reason, 2) packets for which we failed when trying to create the on wire representation (call rsp->pack() and catching an error). For the second it would seem that we should at least count the received dropped counter.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

The test for receiveReplyStat is actually checking pkt6-advertise-received (probably copy and paste error)

Oops. Fixed.

pkt6-receive-drop can be incremented in 5 places but the test only checks for one - if the parse fails. Should there be more tests to check if the packet was dropped for other reasons?

Now 4 cases out of 5 are tested. The last case handles the server throwing general exception. I do not know how to cause the server to throw generic exception in packet handling code. If I knew, I would fix it.


Ticket is back with you. Thanks for helping with the review.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by sar

  • Owner changed from sar to tomek

Aside from the comments below the changes look fine. I don't think I need to re-review this ticket.

Replying to tomek:

Replying to sar:

<snip>

doc/guide/dhcp6-srv.xml

pkt6-receive-drop can be incremented in 5 places but the test only checks for one - if the parse fails. Should there be more tests to check if the packet was dropped for other reasons?

Now 4 cases out of 5 are tested. The last case handles the server throwing general exception. I do not know how to cause the server to throw generic exception in packet handling code. If I knew, I would fix it.

The new tests pkt6ReceivedDropStat1(), pkt6ReceiveDropStat2() and pkt6ReceiveDropStat3() include the comment "Ok, let's check the statistics. None should be present." but there should be a receive-drop present.

The description for stat2 and stat3 appear to be a bit off. The one for stat2 should be moved to stat3 (multiple client ids). A new one for stat2 should be created (unicast packets instead of multicast).

The test in stat3 needs to be modified to remove the code to pretend its a unicast packet

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

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

Replying to sar:

Aside from the comments below the changes look fine. I don't think I need to re-review this ticket.

Thanks for the review.

doc/guide/dhcp6-srv.xml

pkt6-receive-drop can be incremented in 5 places but the test only checks for one - if the parse fails. Should there be more tests to check if the packet was dropped for other reasons?

Now 4 cases out of 5 are tested. The last case handles the server throwing general exception. I do not know how to cause the server to throw generic exception in packet handling code. If I knew, I would fix it.

The new tests pkt6ReceivedDropStat1(), pkt6ReceiveDropStat2() and pkt6ReceiveDropStat3() include the comment "Ok, let's check the statistics. None should be present." but there should be a receive-drop present.

The description for stat2 and stat3 appear to be a bit off. The one for stat2 should be moved to stat3 (multiple client ids). A new one for stat2 should be created (unicast packets instead of multicast).

Comments updated.

The test in stat3 needs to be modified to remove the code to pretend its a unicast packet

stat3 test now properly sends solicit with two server-ids, which causes the exception that triggers statistic update.

Code merged and pushed to master. Closing ticket.

Note: See TracTickets for help on using tickets.