Opened 6 years ago

Closed 2 years ago

Last modified 2 years ago

#3389 closed enhancement (complete)

Kea4 and Kea6 servers should re-detect interfaces when new configuration comes in

Reported by: marcin Owned by: fdupont
Priority: medium Milestone: Kea1.3 beta
Component: dhcp Version:
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: 2
Total Hours: 2 Internal?: no

Description

Currently, the interface detection is performed on server startup. When the server is reconfigured it is possible for the user to specify on which interfaces the server will listen, and on which it will not listen. This should come along with the interface detection if the new interface has been configured later than the server's startup.

Subtickets

Change History (36)

comment:1 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

comment:2 Changed 3 years ago by tomek

  • Milestone changed from Outstanding Tasks to Kea1.2

comment:3 Changed 3 years ago by fdupont

In theory it should be enough to call on the interface manager singleton clearIfaces() followed by detectIfaces()...

comment:4 Changed 3 years ago by fdupont

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

IMHO the simplest (so a priori the best) solution is to move the call to dectectIfaces() outside the constructor. BTW this function can throw so the current constructor too, and the test derived class clears the interface list in its constructor so at least in half cases this call is more than useless...

comment:5 Changed 3 years ago by fdupont

I found a defect in [ "*" ] -> [ ] transition: sockets must be closed in (re)config before parsing. I create a new child ticket to fix it as it is trivial (when new unit tests are not taken into account).

comment:6 Changed 3 years ago by fdupont

More thinking: IMHO a new interface-config knob is needed. I propose "rescan" with a boolean value and false by default (it can be an expensive operation). Of course the value from the new config should be used so the code should go in IfacesConfigParser::parse before parseInterfacesList is called.

comment:7 Changed 3 years ago by fdupont

Can be done as #5019 was merged. IMHO the only question is about the new parameter name: rescan vs re-detect (I go on the second).

comment:8 Changed 3 years ago by fdupont

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

I have no easy idea about an unit-test checking this... if has someone a proposal?

Ready for review.

comment:9 Changed 3 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:10 follow-up: Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

The changes look ok, but I am unsure about the re-detect parameter. I think the default should be true not false. I don't think needing to turn a re-detection knob on is intuitive and we want it to just "work" for most people most of time. On the other hand, someone who is looking for that sort of performance edge will certainly scour the documentation looking for just such knobs. I think most folks would wonder why it didn't see the new vlan they just created. In reality, how much of a time saver is it?

I don't have any easy unit test suggestions for you. At least nothing that wouldn't be a lot of work and I don't see the ROI on that.

comment:11 in reply to: ↑ 10 Changed 3 years ago by fdupont

Replying to tmark:

The changes look ok, but I am unsure about the re-detect parameter. I think the default should be true not false. I don't think needing to turn a re-detection knob on is intuitive and we want it to just "work" for most people most of time. On the other hand, someone who is looking for that sort of performance edge will certainly scour the documentation looking for just such knobs. I think most folks would wonder why it didn't see the new vlan they just created. In reality, how much of a time saver is it?

=> it is arguable. In fact it depends on what a reconfiguration should really do. Anyway as I have to adapt the code to toElement and config-test I can change the default...

I don't have any easy unit test suggestions for you. At least nothing that wouldn't be a lot of work and I don't see the ROI on that.

=> so we'll ask the customers who requested this to verify it does what they expect.

comment:12 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tmark

I updated the code, regenerate flex/bison and get_config, added some doc, etc.
So IMHO it needs another not trivial round of review (and BTW a ChangeLog entry).

comment:13 Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

Francis, did you forget to push changes? The most recent entry in git log is dates Jan 27th:

commit d821d820ef4159885f2dad1635147653f4ff1a49
Author: Francis Dupont <fdupont@isc.org>
Date:   Fri Jan 27 15:29:05 2017 +0100

    [3389] Added interface re-detection in iface parser

The default appears to still be false, I don't see any documentation other than the sample JSON files. There also doesn't appear to be any SimpleDefaults? mechanism for setting the default.

comment:14 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tmark

I believe you were looking at the wrong branch: the current branch is trac3389a (BTW I pushed the REBASED file on trac3389: it was supposed to avoid this problem but until it is pushed it helps nobody).

comment:15 follow-up: Changed 3 years ago by tmark

  • Owner changed from tmark to fdupont

It all builds and unit tests pass for me. It is sort of shame the fake interface mgr code didn't stub detectIfaces(), it would have made all the steps you had to do to not redetect in unit tests less messy.

src/bin/dhcp4/json_.cc:configureDhcp4Server()

The block of code at line 485 implements a hard-coded default rather than
it being done through SimpleDefaults?. This defies the intent to define all
defaultable values in one place.

src/bin/dhcp4/tests/dhcp4_test_util.h:patchIfacesReDetect()

Perhaps you should rename this to disableIfacesRedetect() since this method
can only set the value to false. Either that or change the comment where
the method is used so it explains what "patching" it means:

    // Patch the re-detect flag
    patchIfacesReDetect(json);

The same comments apply to dhpv6.

comment:16 in reply to: ↑ 15 Changed 3 years ago by fdupont

Replying to tmark:

It all builds and unit tests pass for me. It is sort of shame the fake interface mgr code didn't stub detectIfaces(), it would have made all the steps you had to do to not redetect in unit tests less messy.

=> i more than agree!

src/bin/dhcp4/json_.cc:configureDhcp4Server()

The block of code at line 485 implements a hard-coded default rather than
it being done through SimpleDefaults?. This defies the intent to define all
defaultable values in one place.

=> the interface parser is optional and has no default so there is no clean way to do it.

src/bin/dhcp4/tests/dhcp4_test_util.h:patchIfacesReDetect()

Perhaps you should rename this to disableIfacesRedetect() since this method
can only set the value to false. Either that or change the comment where
the method is used so it explains what "patching" it means:

    // Patch the re-detect flag
    patchIfacesReDetect(json);

=> I am in favor to change the name

The same comments apply to dhpv6.

=> of course, I ported changes from dhcpv4...

Note as it changes the syntax so required flex/bison regen I shan't merge it soon so this will likely miss the code freeze.

comment:17 Changed 3 years ago by tomek

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

Code freeze for 1.2-beta. Moving all remaining open tickets to 1.2-final.

comment:18 follow-up: Changed 3 years ago by tomek

Is this ready for merge or are there any outstanding problems here?

comment:19 in reply to: ↑ 18 Changed 3 years ago by fdupont

Replying to tomek:

Is this ready for merge or are there any outstanding problems here?

=> no fully ready and as it was a new feature and it missed the 1.2 code freeze deadline I stalled it.

comment:20 Changed 3 years ago by fdupont

  • Milestone changed from Kea1.2-final to Kea1.3

Moved to 1.3 according to the 2017-04-13 Kea planning.

comment:21 Changed 3 years ago by fdupont

  • Owner changed from fdupont to nobody

Changed the name as recommended. Ready for final review.

comment:22 Changed 3 years ago by fdupont

  • Owner changed from nobody to UnAssigned

comment:23 Changed 2 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:24 follow-up: Changed 2 years ago by tomek

  • Owner changed from tomek to fdupont

I have reviewed the changes currently (a1d86e2c7ea0195edf59e218861d2e909a12b9ba) and have couple comments.

The unit-tests failed for me. The reason is that ParserTest?.file tried to load advanced.json and failed, because parser was not regenerated. I regenerated it and test has passed. Make sure you regenerate it after merging.

I agree with the suggestion Thomas made that the default should be true. Please update the code. There are couple things to consider here:

  1. the default should be written down somewhere in src/bin/dhcpX/simple_parserX.cc.
  1. the default should be true (redetect by default).
  1. Documentation is missing. The user's guide has not been updated.
  1. I don't see any code that stores this parameter, so config-get will miss it. If you decide to keep the parameter, this has to be implemented.

Personally, I don't think it's that much useful to disable the redetection. If I were the one doing this ticket, I would simply call redetect every time and udpate docs that this behavior was improved in 1.3. But I won't insist if you want to keep the parameter. However, if you decide to keep it there's more code to be written - config-get must store and return it. This should also be unit-tested.

Sadly, I must agree that there's not easy way to test the actual interface detection. You could develop a shell test that loads config, then creates some sort of tunnel, then reconfigures kea, but the time invested in this would be huge, the tests would probably require root access and wouldn't be really portable. In return, they would check that a single call to redetectIfaces() is really called. Given all of the above, I think it's ok to not have unit-tests for the actual interfaces being redetected.

I don't think the ChangeLog? entry is appropriate. Users don't know what iface parser is and how its change affects them. It would be better to say:

12XX.	[func]		fdupont
	Kea now re-detects network interfaces every time configuration is
	changed.
	(Trac #3389, git tbd)

comment:25 in reply to: ↑ 24 Changed 2 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

I agree with the suggestion Thomas made that the default should be true. Please update the code.

=> this shows you did not review the right branch (i.e., trac3389a).

comment:26 Changed 2 years ago by fdupont

I am taking back to ticket and perhaps I'll rebase it again. Note the previous message stuck in my browser and is in fact a week old...

comment:27 Changed 2 years ago by fdupont

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

comment:28 Changed 2 years ago by fdupont

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

Most concerns about trac3389 were addressed by trac3389a so I propose that in parallel I check there is no missing thing and you re-review it (the trac3389a branch, BTW the trac3389 was marked as been rebased).

comment:29 Changed 2 years ago by fdupont

About the main argument of comment:24: the way many unit tests are written requires to disable re-detection so there must be a way to disable it even most customers don't need it or simply don't care.

comment:30 follow-up: Changed 2 years ago by tomek

  • Owner changed from tomek to fdupont

Ok, hope I reviewed the right branch this time :) Comments for trac3389a follow:

src/bin/dhcp4/json_config_parser.cc

The default you implemented here is misplaced. It's ok to
have the part that forces redetect to false if check_only is
set.

However, the default of setting it to true must be set together
with all the other defaults we have, i.e. in simple_parser4.cc
This was one of the major things when we moved to simple parser,
so the defaults are not spread out everywhere, but concentrated in
one file.

/src/bin/dhcp6/json_config_parser.cc
The same comment as for v4.

doc/examples/keaX/advanced.json
Your change used tabs, the rest of the file uses spaces. I converted
them. Please pull.

dhcp6-srv.xml
The text you added is good, but we need an example. I added one.
Please pull and review.


The changelog should mention 're-detect' parameter. How about this one:

12XX.	[func]		fdupont
	Kea now re-detects network interfaces every time configuration is
	changed. 're-detect' parameter added to restore old behavior, if
        needed.
	(Trac #3389, git tbd)

comment:31 in reply to: ↑ 30 ; follow-up: Changed 2 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

Ok, hope I reviewed the right branch this time :) Comments for trac3389a follow:

src/bin/dhcp4/json_config_parser.cc

The default you implemented here is misplaced. It's ok to
have the part that forces redetect to false if check_only is
set.

However, the default of setting it to true must be set together
with all the other defaults we have, i.e. in simple_parser4.cc

=> in fact it is not true that all defaults are here, e.g.
D2ClientConfigParser::setAllDefaults is in src/lib/dhcpsrv/parsers.

This was one of the major things when we moved to simple parser,
so the defaults are not spread out everywhere, but concentrated in
one file.

/src/bin/dhcp6/json_config_parser.cc
The same comment as for v4.

=> I believe we have 4 solutions:

  • move the whole thing to simple_parser[46].cc adding a check_only argument to the setAllDefaults class methods
  • only move the default to simple_parser[46].cc setAllDefaults class methods (I believe it is what you'd like)
  • move all or only the default to a new setAllDefaults in src/lib/dhcpsrv/ifaces_config_parser.*

The idea for the last is the default is not DHCP version dependent
but IMHO the duplication between DHCPv4 and DHCPv6 is not
enough to not make for instance the second solution better

doc/examples/keaX/advanced.json
dhcp6-srv.xml

=> I pulled and reviewed them.

comment:32 in reply to: ↑ 31 ; follow-up: Changed 2 years ago by tomek

  • Owner changed from tomek to fdupont

Replying to fdupont:

Replying to tomek:

Ok, hope I reviewed the right branch this time :) Comments for trac3389a follow:

src/bin/dhcp4/json_config_parser.cc

The default you implemented here is misplaced. It's ok to
have the part that forces redetect to false if check_only is
set.

However, the default of setting it to true must be set together
with all the other defaults we have, i.e. in simple_parser4.cc

=> in fact it is not true that all defaults are here, e.g.
D2ClientConfigParser::setAllDefaults is in src/lib/dhcpsrv/parsers.

There are some exceptions, true, but the goal is to have all defaults in one
place eventually. Users don't care about C++ inheritance or code redundancy. They want
to have one place to look to answer questions like "i haven't specified X, what's the default for it"?

/src/bin/dhcp6/json_config_parser.cc
The same comment as for v4.

=> I believe we have 4 solutions:

  • move the whole thing to simple_parser[46].cc adding a check_only argument to the setAllDefaults class methods
  • only move the default to simple_parser[46].cc setAllDefaults class methods (I believe it is what you'd like)

That is indeed my preference.

  • move all or only the default to a new setAllDefaults in src/lib/dhcpsrv/ifaces_config_parser.*

No, we should avoid that. Yes, there's a little bit of duplication, but we get nice clarity in mind. also, we can point non-programmers to this file. THe idea is that you don't need to understand C++ to read a simple table of default values.

comment:33 in reply to: ↑ 32 Changed 2 years ago by fdupont

  • Owner changed from fdupont to tomek

Replying to tomek:

  • only move the default to simple_parser[46].cc setAllDefaults class methods (I believe it is what you'd like)

That is indeed my preference.

=> done. BTW I didn't do the same for the dhcp-socket-type parameter because the use of its default is logged.

Ready for a final review.

I didn't put hours because most of the work was done for 1.2. If you need the 1.3 (only) count put one hour for each of us.

comment:34 Changed 2 years ago by tomek

  • Add Hours to Ticket changed from 0 to 1
  • Owner changed from tomek to fdupont
  • Total Hours changed from 0 to 1

The changes are good, please merge.

Yes, please fill the the hours you spent on this in 1.3. I already put mine.

comment:35 Changed 2 years ago by fdupont

  • Add Hours to Ticket changed from 1 to 2
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 1 to 2

Merged. Closing.

comment:36 Changed 2 years ago by vicky

  • Milestone changed from Kea1.3 to Kea1.3 beta

Milestone renamed

Note: See TracTickets for help on using tickets.