Opened 5 years ago

Closed 5 years ago

#3699 closed enhancement (complete)

Code in AllocEngine, CfgHosts HostMgr should log more details

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

During #3563 review Stephen and I found that the code in AllocEngine?, CfgHosts? and HostMgr? are lacking proper debug messages. This should be expanded as it is very useful for debugging purposes.

Subtickets

Change History (6)

comment:1 Changed 5 years ago by hschempf

  • Priority changed from medium to high

comment:2 Changed 5 years ago by marcin

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

comment:3 Changed 5 years ago by marcin

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

I have added a bunch of log traces to the CfgHosts and HostMgr. I didn't update the Allocation Engine because it is tightly coupled with the Dhcpv4Srv and Dhcpv6Srv code which is awaiting similar update with the tickets 3806 and #3807. I prefer to update the allocation engine together with those tickets or after those tickets are implemented.

For the HR logging, I introduced a new logger: hosts_logger for the following reasons:

  • The functions of the HostMgr are called for each received packet. As a result, there may be many log messages produced. Note, that also DHCP servers and the allocation engine produce messages for each packet. The administrator may want to have a control over log levels for hosts vs allocation engine vs server to extract the bits of information he needs.
  • There is no overhead related to the use of the new logger, other than one additional logger instance hanging in memory.
  • Log messages include the logger name, which makes it easy to distinguish messages related to different types of operations. In this particular case, to distinguish messages related to host reservations.

Proposed ChangeLog entry:

XXX.	[func]		marcin
	libdhcpsrv: Added log traces to the host manager.
	(Trac #3699 git abcd)

comment:4 Changed 5 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:5 follow-up: Changed 5 years ago by tomek

  • Owner changed from tomek to marcin

I did review changes on trac3699 (commit a829519ad468c89a73c9f0c9af02d2630f766868). I have several minor comments:
src/lib/dhcpsrv/cfg_hosts.cc

Many log entries use formatting with each .arg() being in their own line.
Is there any reason for that? I don't think it's specified in our coding
guidelines. We should either update the guidelines or update the code to
use regular notation. I don't have any strong preference here, but updating
the code changed would probably be easier in the short term.

Some of the code is overly wrapped in my opinion. The coding guidelines
say that we should keep to 80 columns boundary in general, but it's ok
to move beyond it up to 100 columns.

For example the code in line 167:

    for (HostContainerIndex1::iterator host = r.first; host != r.second;
         ++host) {

could be written as:

    for (HostContainerIndex1::iterator host = r.first; host != r.second; ++host) {

which is only 2 columns above 80 limit.

CfgHosts::getAllInternal(): " Log how many hosts found." It would read better
if the text said: Log how many hosts were found or similar.

CfgHosts::getHostInternal, around line 411 logs host details if a match was
found. Do you think it would be useful to log something when no match
was found?

CfgHosts::getAllInternal6(const IOAddress& address, Storage& storage):
Does it make sense to log the number of hosts found (HOSTS_CFG_GET_ALL_ADDRESS6_COUNT)?
The addresses are supposed to be unique, so this code is expected to find
0 or 1 hosts. Or am I missing something?

hosts_messages.mes
HOSTS_CFG_GET_ALL_ADDRESS6_COUNT description mentions IPv4, should be IPv6.

HOSTS_CFG_GET_ALL_ADDRESS6_HOST addres => address

In serveral descriptions you use the following explanation "This debug message is
issued when retrieving reservations...". It's a bit ambiguous as it is no
clear, whether the code is about to look for those reservations or already found
them and retrieving them now. Please reword this, if you agree. Perhaps
"This debug message is issed when the code is trying to retrieve reservations...".

HOSTS_CFG_GET_ONE_SUBNET_ID_HWADDR_DUID_HOST is missing description.


The code compiles on Mac OS 10.9.5 and all unit-tests pass (compilation with
--enable-logger-checks). The changes are sufficiently minor, so I don't need to
see this ticket again.

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

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

Replying to tomek:

I did review changes on trac3699 (commit a829519ad468c89a73c9f0c9af02d2630f766868). I have several minor comments:
src/lib/dhcpsrv/cfg_hosts.cc

Many log entries use formatting with each .arg() being in their own line.
Is there any reason for that? I don't think it's specified in our coding
guidelines. We should either update the guidelines or update the code to
use regular notation. I don't have any strong preference here, but updating
the code changed would probably be easier in the short term.

I don't really think that the coding guidelines should go into that level of details. I wrap the arguments to log messages because it makes it clearer in my opinion. Same as wrapping some function arguments even if they don't hit 80 lines.

Some of the code is overly wrapped in my opinion. The coding guidelines
say that we should keep to 80 columns boundary in general, but it's ok
to move beyond it up to 100 columns.

I was under impression that we want 80 columns and go beyond this only if it is really necessary. Most of the developers are used to 80 columns and most of the time 80 columns is enough.

For example the code in line 167:

    for (HostContainerIndex1::iterator host = r.first; host != r.second;
         ++host) {

could be written as:

    for (HostContainerIndex1::iterator host = r.first; host != r.second; ++host) {

which is only 2 columns above 80 limit.

CfgHosts::getAllInternal(): " Log how many hosts found." It would read better
if the text said: Log how many hosts were found or similar.

It was an omission. Added it now.

CfgHosts::getHostInternal, around line 411 logs host details if a match was
found. Do you think it would be useful to log something when no match
was found?

Omission again. Good catch.

CfgHosts::getAllInternal6(const IOAddress& address, Storage& storage):
Does it make sense to log the number of hosts found (HOSTS_CFG_GET_ALL_ADDRESS6_COUNT)?
The addresses are supposed to be unique, so this code is expected to find
0 or 1 hosts. Or am I missing something?

But, if we introduce the bug logging it may allow for better tracibility.

hosts_messages.mes
HOSTS_CFG_GET_ALL_ADDRESS6_COUNT description mentions IPv4, should be IPv6.

Fixed.

HOSTS_CFG_GET_ALL_ADDRESS6_HOST addres => address

Fixed.

In serveral descriptions you use the following explanation "This debug message is
issued when retrieving reservations...". It's a bit ambiguous as it is no
clear, whether the code is about to look for those reservations or already found
them and retrieving them now. Please reword this, if you agree. Perhaps
"This debug message is issed when the code is trying to retrieve reservations...".

Hope it is less ambiguous now.

HOSTS_CFG_GET_ONE_SUBNET_ID_HWADDR_DUID_HOST is missing description.

Added one.


The code compiles on Mac OS 10.9.5 and all unit-tests pass (compilation with
--enable-logger-checks). The changes are sufficiently minor, so I don't need to
see this ticket again.

Thanks for testing and review.

Merged with 75b75c89db88eb1a81e76f5550f2a5b3155ce42d.

Note: See TracTickets for help on using tickets.