Opened 3 years ago

Closed 21 months ago

#3696 closed defect (fixed)

Missing lease-database entry confuses Kea

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Kea1.1
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: 6
Total Hours: 6 Internal?: no

Description (last modified by marcin)

I tried to use the following config file to run Kea6 in stateless-only mode:

"Dhcp6": {
    "interfaces": [ "ethX" ],
    "option-data": [ {
        "name": "dns-servers",
        "data": "2001:db8::1, 2001:db8::2"
    } ]
 }

Note missing entry for lease-database. The config was accepted, but
upon start-up, it started printing out error messages very rapidly:

# ./kea-dhcp6 -c stateless.json 
2015-01-28 15:29:56.068 INFO  [kea-dhcp6.dhcp6/15706] DHCP6_STARTING Kea DHCPv6 server version 0.9-git starting
2015-01-28 15:29:56.069 INFO  [kea-dhcp6.dhcpsrv/15706] DHCPSRV_CFGMGR_ADD_IFACE listening on interface eth0
2015-01-28 15:29:56.069 INFO  [kea-dhcp6.dhcp6/15706] DHCP6_CONFIG_COMPLETE DHCPv6 server has completed configuration: no IPv6 subnets!; DDNS: disabled
2015-01-28 15:29:56.069 ERROR [kea-dhcp6.dhcp6/15706] DHCP6_PACKET_RECEIVE_FAIL error on attempt to receive packet: no current lease manager is available
2015-01-28 15:29:56.069 WARN  [kea-dhcp6.dhcp6/15706] DHCP6_LEASE_DATABASE_TIMERS_EXEC_FAIL failed to execute timer-based functions for lease database: no current lease manager is available
2015-01-28 15:29:56.069 ERROR [kea-dhcp6.dhcp6/15706] DHCP6_PACKET_RECEIVE_FAIL error on attempt to receive packet: no current lease manager is available
2015-01-28 15:29:56.069 WARN  [kea-dhcp6.dhcp6/15706] DHCP6_LEASE_DATABASE_TIMERS_EXEC_FAIL failed to execute timer-based functions for lease database: no current lease manager is available
2015-01-28 15:29:56.069 ERROR [kea-dhcp6.dhcp6/15706] DHCP6_PACKET_RECEIVE_FAIL error on attempt to receive packet: no current lease manager is available
2015-01-28 15:29:56.069 WARN  [kea-dhcp6.dhcp6/15706] DHCP6_LEASE_DATABASE_TIMERS_EXEC_FAIL failed to execute timer-based functions for lease database: no current lease manager is available

I see two options here:

  1. if there's no lease-database entry, just assume that memfile should be used.
  2. extend the code to be able to run without LeaseMgr? instance.

While the 2. is more ideologically correct, 1. seems much more pragmatic.

Note: While I haven't checked, I expect the same issue to come up in v4.

Subtickets

Change History (11)

comment:1 Changed 3 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.0

comment:2 Changed 2 years ago by tomek

Also see #3892 (closed as duplicate).

comment:3 Changed 2 years ago by stephen

  • Milestone changed from Kea1.0 to DHCP Outstanding Tasks

Per the Kea planning meeting in October, remove from 1.0.

comment:4 Changed 2 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

comment:5 Changed 23 months ago by marcin

  • Description modified (diff)
  • Milestone changed from Outstanding Tasks to Kea1.1
  • Owner set to marcin
  • Status changed from new to accepted

As part of the bug-fixing effort for 1.1 release, I am moving this ticket to 1.1 milestone.

comment:6 Changed 23 months ago by marcin

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

I modified the database access parser to store the access string in the configuration manager. If the access parser is not launched (due to lack of the lease-database parameter) the default backend is used.

Proposed ChangeLog entry:

XXXX.	[bug]		marcin
	When lease-database configuration parameter is not
	specified the default lease database backend (memfile)
	is used.
	(Trac #3696, git abcd)

comment:7 Changed 21 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:8 follow-up: Changed 21 months ago by tomek

  • Owner changed from tomek to marcin

I did review the code on trac3696 branch (up to and including commit a08a044bcfcaeb9489a6ce0a79b55cf1a091e710). Here are my comments:

srv_config.h
getCfgDbAccess() method comment: s/objec/object/

memfile_lease_mgr.cc
Memfile_LeaseMgr::getDescription() should mention that the leases
are written to disk. The text as is now suggests it's in-memory only.

cfg_db_access.h
setAppendedParameters - name of the method suggest plurality. The
comment should give an example of what the format should be and
how to separate multiple parameters. I guess its name=value separated
by spaces, right?

Why do we have separate strings for lease db and host db? Wouldn't
it be simpler to have two instances of the CfgDbAccess?? Keep in mind
that some time in the future we'll add subnet db to the mix.

I must say that I'm not very happy to see the createManagers() method
here. The description of CfgDbAccess? class says that its purpose is
to hold the configuration, not actually manage the managers. But I
won't insist on moving it somewhere else, if you think it's ok to
keep it here.

getAccessString() - that's definetely a creative way of determining
whether appended parameters should be added or not. But I like it :)

json_config_parser.cc
Should have copyright years updated.

There are no unit-tests that check how the code behaves during
reconfiguration. I was thinking about three scenarios:

  1. no leasemgr specified -> leasemgr specified
  2. leasemgr specified -> no lease mgr specified (I would do that test when MySQL is enabled to make sure that the MySQL->memfile switch really takes place)
  3. memfile specified -> mysql specified

I think it's essential to check 1. People may forget to add it and
then try to correct their mistake. 2 is useful, but I don't think it
would happen frequently. Nevertheless it would prove that the
reconfiguration logic is sane. 3 is optional. Changing lease storage
is so essential parameter that if it doesn't work, we would simply
recommend doing full restart. Nevertheless, we should understand
whether the code is working properly or not. It would also pave a way
for more advanced tests in the future (e.g. what to do when the lease
mgr creation fails due to DB access errors and how to recover from that).


I did test that the code builds and unit-tests pass (with -O0 due to
the gcc 5 issue) when built on Ubuntu 15.10 x64.

comment:9 in reply to: ↑ 8 Changed 21 months ago by marcin

  • Add Hours to Ticket changed from 0 to 6
  • Owner changed from marcin to tomek
  • Total Hours changed from 0 to 6

Replying to tomek:

I did review the code on trac3696 branch (up to and including commit a08a044bcfcaeb9489a6ce0a79b55cf1a091e710). Here are my comments:

srv_config.h
getCfgDbAccess() method comment: s/objec/object/

Corrected.

memfile_lease_mgr.cc
Memfile_LeaseMgr::getDescription() should mention that the leases
are written to disk. The text as is now suggests it's in-memory only.

Extended the description string a bit.

cfg_db_access.h
setAppendedParameters - name of the method suggest plurality. The
comment should give an example of what the format should be and
how to separate multiple parameters. I guess its name=value separated
by spaces, right?

Added an example to the description of this function.

Why do we have separate strings for lease db and host db? Wouldn't
it be simpler to have two instances of the CfgDbAccess?? Keep in mind
that some time in the future we'll add subnet db to the mix.

I didn't want to multiply the number of different objects there. You have a valid point but I wouldn't bother thinking about the subnet just yet. Hopefully we can improve the configuration parsing code before we get to the subnet.

I must say that I'm not very happy to see the createManagers() method
here. The description of CfgDbAccess? class says that its purpose is
to hold the configuration, not actually manage the managers. But I
won't insist on moving it somewhere else, if you think it's ok to
keep it here.

This is not the first place where we follow such practice. I think this is logical place to implement such function because it has full access to all the data required to create the instance of the manager.

getAccessString() - that's definetely a creative way of determining
whether appended parameters should be added or not. But I like it :)

json_config_parser.cc
Should have copyright years updated.

Updated.

There are no unit-tests that check how the code behaves during
reconfiguration. I was thinking about three scenarios:

  1. no leasemgr specified -> leasemgr specified
  2. leasemgr specified -> no lease mgr specified (I would do that test when MySQL is enabled to make sure that the MySQL->memfile switch really takes place)
  3. memfile specified -> mysql specified

I think it's essential to check 1. People may forget to add it and
then try to correct their mistake. 2 is useful, but I don't think it
would happen frequently. Nevertheless it would prove that the
reconfiguration logic is sane. 3 is optional. Changing lease storage
is so essential parameter that if it doesn't work, we would simply
recommend doing full restart. Nevertheless, we should understand
whether the code is working properly or not. It would also pave a way
for more advanced tests in the future (e.g. what to do when the lease
mgr creation fails due to DB access errors and how to recover from that).

Ok, I have added a bunch of unit tests as suggested. This wasn't a simple change though. Use of the MySQL backend requires that the schema is created for a test. This required some general purpose files to be moved to a common test library that the unit tests could access from where they are.


I did test that the code builds and unit-tests pass (with -O0 due to
the gcc 5 issue) when built on Ubuntu 15.10 x64.

Thanks for your review and giving it a try and please see how that works for you after my updates.

comment:10 Changed 21 months ago by tomek

  • Owner changed from tomek to marcin

I did review your changes.

A question about new class in kea_controller_unittest.cc. It's called
JSONFileBackendMySQLTest. Since we gave up the idea of configuration
backends, I think it doesn't make much sense to keep references to the
backend concept. While renaming JSONFileBackendTest to something more
appropriate is clearly outside of the scope of this ticket, I think it
makes sense to not introduce more references to the JSON backend
concept. I don't know, maybe ConfigMySQLEnabledTest would be a better
name? It's only a suggestion. If you want to stick with the name as
is, that's ok.

lea_controller_unittest.cc (applies to both in dhcp4 and dhcp6):

The first comment in testBackendReconfiguration is incorrect:

    // This is basic server configuration which excludes lease database
    // backend specification. The default Memfile backend should be
    // initialized in this case.
    writeFile(createConfiguration(backend_first));

The configuration may or may not exclude lease configuration. That
depends on the backend_first value. The test logic is valid, though.
So it's just a matter of rewording the comment.

The same is true for the following comment:

// New configuration modifies the lease database backend type to MYSQL.
    writeFile(createConfiguration(backend_second));

I did test the updated code. It compiles ok and unit-tests pass on Ubuntu 15.10 x64
(compiled with -O0 due to unrelated known gcc-5 issue).

When you correct the comments (and possibly rename the class if you decide so), the
code will be ready for merge.

comment:11 Changed 21 months ago by marcin

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

Comments fixed and the ticket merged to master 0be5e6eb32680a742ddcf427b8181f55c0c98115

Note: See TracTickets for help on using tickets.