Opened 5 years ago

Closed 5 years ago

#3591 closed defect (fixed)

Kea creates lock file incorrectly

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea0.9.1beta
Component: logging 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 start-up Kea creates lock file incorrectly. There are several issues:

  1. By default it uses TOP_BUILDDIR (a relative path to the top level source direction). It should be somewhere in ${prefix}/var. (See src/lib/log/logger_unittest_support.cc)
  1. Production code should not use logging functions that were initially intended for testing. If they are upgraded for production use, they should be moved out of logger_unittest_support.cc
  1. The code ignores KEA_LOCKFILE_DIR_FROM_BUILD env. variable content and overwrites it. That variable should not be overwritten. Also, it should be more appropriately named. KEA_LOCKFILE_DIR looks like a reasonable name.
  1. Kea's usage of env. variables should be documented.

See the following thread for original bug report:
https://lists.isc.org/pipermail/kea-dev/2014-September/000074.html

Subtickets

Change History (9)

comment:1 Changed 5 years ago by tomek

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

comment:2 Changed 5 years ago by tomek

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

Code is ready for review.

Proposed ChangeLog? entry:

8xx.	[bug]		tomek
	Kea components now use KEA_LOCKFILE_DIR to store logging lockfile,
	if the variable is set. Lockfile can now be disabled completely
	by setting KEA_LOCKFILE_DIR to 'none'.
	(Trac #3591, git tbd)

comment:3 Changed 5 years ago by jreed

I saw the docs said default lockfile directory is under $prefix, but it shouldn't be @prefix@ but maybe @localstatedir@

comment:4 Changed 5 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-ups: Changed 5 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit fb1dec2fe851760670fbb2904492cf9bdc16387c

I've made a number of edits to doc/guide/logging.xml - please pull and review.

Makefile.am
When I did a "make install" I ended up with <prefix>/var/kea and <prefix>/var/run/kea. Do we use both directories?

src/bin/d2/tests/d2_process_tests.sh.in
Should the name of the "logger_vars" test be "dhcpv6.variables"?

src/bin/dhcp4/tests/dhcp4_process_tests.sh.in
Should the name of the "logger_vars" test be "dhcpv6.variables"?

src/bin/dhcp6/tests/dhcp6_process_tests.sh.in
Should the name of the "logger_vars" test be "dhcpv6.variables"?

src/lib/dhcpsrv/logging.cc
applyDefaultConfiguration(): You can get rid of the declaration and initialization of "spec". This holds the specification of a logger and nothing else. With the changes made to this method, it is now not used. Removing it means that applyDefaultConfiguration() is now nothing more than a call to setDefaultLoggingConfiguration() and so can be removed in turn; calls to it can be replaced by calls to setDefaultLoggingConfiguration().

src/lib/log/interprocess/interprocess_sync_file.cc
Copyright needs updating to 2014.

InterpocessSyncFile::doLock(): LOCKFILE_DIR is a macro that must be defined at compile time. However, there is nothing in the file that indicates this. If (for whatever reason) the file is compiled outside of the current set of makefiles, an error will result. I suggest a comment in the code as to what LOCKFILE_DIR is and how it needs to be set.

InterpocessSyncFile::doLock(): Remark: although not changed by this ticket, the "mode" arguments to umask() and open() are specified as octal constants: it would be better to use symbolic constants.

src/lib/log/interprocess/tests/run_unittests.cc
Copyright needs updating to 2014.

A note to the effect that TEST_DATA_TOPBUILDDIR is defined as a compile-time macro would be useful.

src/lib/log/logger_impl.cc
Copyright needs updating to 2014.

src/lib/log/logger_support.h
src/lib/log/logger_unittest_support.cc
Copyright needs updating to 2014. (Both files)

setDefaultLoggingOutput() is declared in logger_support.h but defined in logger_unittest_support.cc. Either this is a general function (in which case it should be defined in logger_support.cc) or a unit test function (in which case it should be declared in longer_unittest_support.h).

src/lib/log/logger_unittest_support.h
Copyright needs updating to 2014.

If we haven't done so already, we should create a ticket to rename the unit test "initLogger" to something else. (Mentioned as a "TODO" in this file.

src/lib/log/tests/logger_level_impl_unittest.cc
Copyright needs updating to 2014.

src/lib/log/tests/logger_support_unittest.cc
Copyright needs updating to 2014.

src/lib/util/threads/tests/run_unittests.cc
Copyright needs updating to 2014.

A note to the effect that TEST_DATA_TOPBUILDDIR is defined as a compile-time macro would be useful.

ChangeLog
Suggest slight change of wording:

Kea components now use the KEA_LOCKFILE_DIR environment
variable to specify the directory of the logging lockfile.
Locking can be disabled completely by setting the
variable to 'none'.

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

Replying to stephen:

When I did a "make install" I ended up with <prefix>/var/kea and <prefix>/var/run/kea. Do we use both directories?

We should not have var/kea (we should not have @localstatedir@/kea) but should use some common subdirectory under @localstatedir@ depending on the use.

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

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit fb1dec2fe851760670fbb2904492cf9bdc16387c

I've made a number of edits to doc/guide/logging.xml - please pull and review.

Thank you. They are fine.

Makefile.am
When I did a "make install" I ended up with <prefix>/var/kea and <prefix>/var/run/kea. Do we use both directories?

Yes. And according to Jeremy we need both. The other use case is the default location for memfile storage. I initially proposed to keep everything in one directory, and packagers can tweak it with ./configure --localstatedir. However, he said that the functionality of storing run time files and lockfiles are often separated. The idea to add /run/ to the path was Jeremy's idea.

src/bin/d2/tests/d2_process_tests.sh.in
Should the name of the "logger_vars" test be "dhcpv6.variables"?

Yes.

src/bin/dhcp4/tests/dhcp4_process_tests.sh.in
Should the name of the "logger_vars" test be "dhcpv6.variables"?

Yes.

src/bin/dhcp6/tests/dhcp6_process_tests.sh.in
Should the name of the "logger_vars" test be "dhcpv6.variables"?

No.

src/lib/dhcpsrv/logging.cc
applyDefaultConfiguration(): You can get rid of the declaration and initialization of "spec". This holds the specification of a logger and nothing else. With the changes made to this method, it is now not used. Removing it means that applyDefaultConfiguration() is now nothing more than a call to setDefaultLoggingConfiguration() and so can be removed in turn; calls to it can be replaced by calls to setDefaultLoggingConfiguration().

For some reason I thought that the initialization of the spec triggers initialization in log4cplus. It does not, so removed as you suggested.

src/lib/log/interprocess/interprocess_sync_file.cc
Copyright needs updating to 2014.

Updated. Also all other files you mentioned.

InterpocessSyncFile::doLock(): LOCKFILE_DIR is a macro that must be defined at compile time. However, there is nothing in the file that indicates this. If (for whatever reason) the file is compiled outside of the current set of makefiles, an error will result. I suggest a comment in the code as to what LOCKFILE_DIR is and how it needs to be set.

Added. I'm not sure how much value such a practice has. I have heard about only one case of removing our makefiles and coming up with alternate scheme. Dominik Zeromski, one of the students working on minimalistic server disliked our dependencies so much that he wrote his own cmake build scripts. That was in the 0.8 timeframe. He said he'll rebase his changes to 0.9.

InterpocessSyncFile::doLock(): Remark: although not changed by this ticket, the "mode" arguments to umask() and open() are specified as octal constants: it would be better to use symbolic constants.

Updated. Let's hope I did them correctly.

src/lib/log/interprocess/tests/run_unittests.cc
Copyright needs updating to 2014.

A note to the effect that TEST_DATA_TOPBUILDDIR is defined as a compile-time macro would be useful.

Added.

src/lib/log/logger_support.h
src/lib/log/logger_unittest_support.cc

setDefaultLoggingOutput() is declared in logger_support.h but defined in logger_unittest_support.cc. Either this is a general function (in which case it should be defined in logger_support.cc) or a unit test function (in which case it should be declared in longer_unittest_support.h).

Moved.

src/lib/log/logger_unittest_support.h
Copyright needs updating to 2014.

If we haven't done so already, we should create a ticket to rename the unit test "initLogger" to something else. (Mentioned as a "TODO" in this file.

Added #3605, but I'm not sure what milestone it will end up at.

src/lib/log/tests/logger_level_impl_unittest.cc
src/lib/log/tests/logger_support_unittest.cc
src/lib/util/threads/tests/run_unittests.cc
Copyright needs updating to 2014.

Updated.

ChangeLog
Suggest slight change of wording:

Kea components now use the KEA_LOCKFILE_DIR environment
variable to specify the directory of the logging lockfile.
Locking can be disabled completely by setting the
variable to 'none'.

Updated as suggested.

comment:8 Changed 5 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 6098e089b32fb948ccc777a75af30627922ad18e

All looks OK. One comment though:

src/lib/dhcpsrv/daemon.cc
As "configureLogger" was modified, the following additional modification - replacing

isc::data::ConstElementPtr loggers;
loggers = log_config->get("loggers");

by

isc::data::ConstElementPtr loggers = log_config->get("loggers");

... could also be done.

Ready to merge, I don't need to see it again.

comment:9 Changed 5 years ago by tomek

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

Merged, closing ticket. Thanks for your review.

During merge I had to slightly tweak the code:

  • LogConfigParser::applyConfiguration() was no longer used, so removed
  • SrvConfig::applyLoggingCfg() used to set KEA_LOCKFILE_DIR_FROM_BUILD, which is no longer used, so removed it
  • Daemon::loggerInit() now sets KEA_LOGGER_DESTINATION to stdout if it is not set, thus preserving current logging behavior.
Note: See TracTickets for help on using tickets.