Opened 8 months ago

Closed 7 months ago

#5186 closed defect (fixed)

config-test command result depends on config currently used by kea

Reported by: wlodekwencel Owned by: fdupont
Priority: medium Milestone: Kea1.2-final
Component: agent 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

Firstly I start Kea with config file:

{
    "Dhcp6":
    {
        "renew-timer":1000,
        "rebind-timer":2000,
        "preferred-lifetime":3000,
        "valid-lifetime":4000,
        "interfaces-config":
        {
            "interfaces":["eth2"]
        }
        ,
        "subnet6":[
        {
            "subnet":"3000::/64",
            "interface":"eth2",
            "pools":[
            {
                "pool":"3000::1-3000::f"
            }
            ]
        }
        ],
        "lease-database":
        {
            "type":"memfile"
        }
        ,
        "control-socket":
        {
            "socket-type":"unix",
            "socket-name":"/home/test/installed/git/var/kea/control_socket"
        }
    }
    ,
    "Logging":
    {
        "loggers":[
        {
            "name":"kea-dhcp6",
            "output_options":[
            {
                "output":"/home/test/installed/git/var/kea/kea.log"
            }
            ],
            "debuglevel":99,
            "severity":"DEBUG"
        }
        ]
    } 
}

Then I'm using control channel command "config-test" to check different config, and no matter if it's correct or not result is always the same:

{
  "result": 1,
  "text": "Error during command processing: unable to unregister timer flush-reclaimed-leases while worker thread is running"
}

But if I will add to current config:

        "expired-leases-processing":
        {
            "flush-reclaimed-timer-wait-time":0,
            "hold-reclaimed-time":0,
            "max-reclaim-leases":100,
            "max-reclaim-time":0,
            "reclaim-timer-wait-time":0,
            "unwarned-reclaim-cycles":5
        }

"config-test" command start to work properly. So kea during testing proposed config is using current config file. This process should be separated, and should not interact with current working config file.

Subtickets

Change History (8)

comment:1 Changed 8 months ago by fdupont

Interesting... IMHO the best fix (but not the easiest one0 is to avoid a config sub-routine to play with timers when called in test mode. Note the reconfig mode stops timers so works well...
Thanks to have found this, now I don't know if it can be fixed before the freeze... Of course the -t command line is still a working alternative.

comment:2 Changed 8 months ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.2-final

Per Kea team meeting Mar 30, accept 1.2-final

comment:3 Changed 7 months ago by hschempf

  • Priority changed from high to medium

comment:4 Changed 7 months ago by fdupont

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

comment:5 Changed 7 months ago by fdupont

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

I made the call to unregisterTimers() conditional and it seems this trivial fix is enough to solve the problem...
BTW resetSubnetID() (class method so global) is not undone: this will break commands which manage only subnets (vs the whole config).

Ready for review
(the hard thing is to test so I am explaining what I did:

  • take a simple but working config with a control-socket section
  • launch a kea server with this config in its terminal window
  • put { "command": "config-reload' } in a file
  • do socat UNIX:<control-socket-path> - < <file>
  • it should work and the kea server should log a lot of thing
  • edit the file to change to { "command": "config-test", "arguments": <insert-config> }
  • unpatched server fails and stops
  • patched server succeed and continue

)

comment:6 Changed 7 months ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 follow-up: Changed 7 months ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commit a62c62639f9d4e4da340e350bb52475130388a03

  1. The file src/lib/dhcp/tests/libdhcp++_unittest.cc would not compile on Ubuntu 16.04 (g++ version 5.4.0) : the static_assert at line 1689 is missing the second argument. (That error generated a warning on OSX (clang-700.1.81) but the file compiled successfully.) The fix has been corrected in the master branch, so I used master's copy of the file in tests. However, when merging, please check that file to ensure that the correct version is used.
  1. The changes seem OK. I ran the tests described above for the DHCPv6 server. Without the patch, my server continued working, but I got the error described in the ticket description and a COMMAND_PROCESS_ERROR2 error in the log file. With the patch, a success message was returned to socat and no error was recorded in the log file.

The change can be merged (but as noted above, check that the correct copy of libdhcp++_unittest.cc ends up in master).

This probably needs a ChangeLog entry. Something like:

Do not unregister timers when running the config-test command.

... should be OK.

comment:8 in reply to: ↑ 7 Changed 7 months ago by fdupont

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

Replying to stephen:

Reviewed commit a62c62639f9d4e4da340e350bb52475130388a03

  1. The file src/lib/dhcp/tests/libdhcp++_unittest.cc would not compile on Ubuntu 16.04 (g++ version 5.4.0) : the static_assert at line 1689 is missing the second argument.

=> yes, C++17 does not require this second argument so some compilers accept the code (including the one I used), some stop.

(That error generated a warning on OSX (clang-700.1.81) but the file compiled successfully.) The fix has been corrected in the master branch, so I used master's copy of the file in tests. However, when merging, please check that file to ensure that the correct version is used.

=> 1. does not belongs to this ticket which BTW does not change an unit test file.

  1. The changes seem OK. I ran the tests described above for the DHCPv6 server. Without the patch, my server continued working, but I got the error described in the ticket description and a COMMAND_PROCESS_ERROR2 error in the log file. With the patch, a success message was returned to socat and no error was recorded in the log file.

=> this looks like a working fix...

The change can be merged (but as noted above, check that the correct copy of libdhcp++_unittest.cc ends up in master).

This probably needs a ChangeLog entry. Something like:

Do not unregister timers when running the config-test command.

... should be OK.

=> adopted!

Merged. Closing.

Note: See TracTickets for help on using tickets.