#5583 closed defect (fixed)

Fix issues with missing logging placeholder in the HA unit tests

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.4
Component: high-availability 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

One running HA unit tests may see a lot of those:

2018-04-10 15:41:53.681 INFO  [kea.ha_hooks/62473] HA_LOCAL_DHCP_DISABLE  @@Missing placeholder %1 for 'server3'@@ @@Missing placeholder %2 for 'WAITING'@@

I investigated this problem and it turned out that this issue is caused by the fact that we mix unit tests that load HA library and those that don't and simply test classes that also perform logging.

Prior to loading the library to he code includes the ha messages along with MessageInitializer:

const isc::log::MessageInitializer initializer(values);

which, in conjunction with initLogger() stores log messages in the global dictionary.

Then, some tests load and unload the HA library which creates another instance of MessageInitializer and destroys it upon unload. The new instance warns about duplicated log messages but then drops those duplicates. When the destructor is called during unload, it searches for the relevant messages by their ids and finds them in the global dictionary (registered prior to loading the library) and removes them. The effect is that HA specific log messages do not exist in the dictionary throughout the rest of the tests causing the error message pasted above.

Subtickets

Change History (5)

comment:1 Changed 22 months ago by marcin

  • Milestone changed from Kea-proposed to Kea1.4

Moving to 1.4 per Kea call on April 12th, 2018.

comment:2 Changed 22 months ago by marcin

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

comment:3 Changed 22 months ago by marcin

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

This ticket is stacked on the #5467, because #5467 converts one of the culprit tests so as it doesn't need to load the hook library. With this ticket I removed the second test which caused this problem. I tried to find some way to convert this test so as it doesn't cause the problem, but it turned out to be harder than I had expected. There are two major issues....

The first issue is that load() function uses LibraryHandle to access configuration. The unit test has no way of setting the configuration, other than via loading the lib. I tried to split the load() function into two function, of which one takes a ConstElementPtr as a parameter, rather than LibraryHandle. That works, but.....

the second issue is that the load() function registers command handlers via the LibraryHandle. What you can get is the product of HooksManager::preCalloutsLibraryHandlePtr() which is not the "real" handle that can be used for such purpose.

I just decided to remove the test, counting on the system testing to do the right thing. If we really want to test load() functions we need some generic and clever way of doing it without a harm to logging system. I think this is beyond the scope of this or any other HA ticket.

Proposed ChangeLog entry:

XX.	[bug]		marcin
	Removed HA hook library unit test which caused logging issues
	on debug level.
	(Trac #5583, git cafe)

comment:4 Changed 22 months ago by fdupont

  • Owner changed from UnAssigned to marcin

It fixes the problem. Unfortunately it removes some tests but I can't see a critical reason to create a libloadtests directory for the HA hook as for legal logging or RADIUS so according to time constraints I shan't insist.
Please merge.

comment:5 Changed 22 months ago by marcin

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

Merged with commit cd053c6b0e9783a796c7a2cdbe93c6d2dd3efbe5

Note: See TracTickets for help on using tickets.