BIND 10 #3234: Kea4/6: reconfigure increases subnet_id
BIND 10 Development
do-not-reply at isc.org
Thu Jan 9 11:54:17 UTC 2014
#3234: Kea4/6: reconfigure increases subnet_id
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: defect | Status:
Priority: high | reviewing
Component: dhcpconf | Milestone: DHCP-
Keywords: | Kea1.0-alpha
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* owner: marcin => tomek
Comment:
Replying to [comment:7 tomek]:
> Replying to [comment:6 marcin]:
> > Reviewed commit 09cfa792bffe8ffa9106c915334ef539577fb62d
Reviewed commit 31e416087685a6dadc3047fdbb0927bbf60095aa
> >
> > General comment. The static subnet_id is a nice trick to keep having
globally unique IDs accross all Subnet objects. However, I foresee some
problems with that approach when it comes to keeping the configuration
consistent with the lease database!
> Agree. This is not an ultimate solution, just the remedy for the most
obvious issue. Without this fix, every time anything was changed in the
config, server kept forgetting every lease (because of mis-matched subnet-
id).
>
> A follow up work is needed. I created a ticket #3281 for that.
>
>
Ok.
> > I have in mind the case, where there are multiple subnets configured,
each having distinct subnet id. When adding a new subnet, the
configuration manager will put it on the very end of the list, and I
presume it will be processed as the last getting the highest subnet id.
Other Subnets will get their usual subnet ids. However, I am concerned
that when you remove the subnet in the middle (say second subnet id out of
3), the IDs for remaining two will be renumbered, such that the subnet id
with the number of 3 will now get subnet id = 2. If there are any leases
in the database for the subnet id 3, they will not match with the subnet
they belong to.
> In general case, it will break if you remove any subnet that is not the
last. We also need to cover another case - where you remove one subnet and
add different one.
Agreed.
>
> This also brings in a question whether we want to get rid of all leases
from the database when we remove the subnet configuration. There are pros
and cons for keeping and removing it. We may also consider a feature for
gentle removal of a subnet. The subnet is marked for deletion, but the
server keeps it until the least active lease is expired or released.
Server does not extend existing leases.
>
> > Nevertheless, this problem is a little more complex, because by
removing all subnets from !CfgMgr on reconfiguration we actually loose all
information about the matching between subnets held in !CfgMgr and in
bind10 configuration. I don't see the easy solution for that, other than
manually assign subnet id with the bindctl and check for invalid or
duplicate IDs.
> True. I'm afraid that ultimately we'll have to implement some sort of
logic that will do comparison between old subnets and new subnets and then
deal with the leases that belong to subnets being removed.
Yes.
>
> > '''!ChangeLog'''
> > Please consider adding a space between comma sign and !''b10-dhcp6!''
like this:
> > {{{
> > b10-dhcp4, b10-dhcp6: ...
> > }}}
> Updadated.
Ok.
>
> > '''src/bin/dhcp4/tests/config_parser_unittest.cc'''
> > '''src/bin/dhcp6/tests/config_parser_unittest.cc'''
> >
> > Apparently, there are no tests that check removal of the existing
subnet or addition of the new subnet if there are existing subnets. In the
case of removal, there is a problem of subnet id rennumbering which I
described above.
> Added new test. It has 2 cases. The first one removes the last subnet.
We can support that now and it passes. The second case (had 4 subnets,
removing 2nd) is implemented, but currently fails. It is ifdefed with the
@todo comment referencing #3281.
Just after this test there is another todo comment:
{{{
/// @todo: implement subnet removal test as part of #3281
}}}
Is it still required given that you implemented the test and ifdefed it?
>
> > '''src/lib/dhcpsrv/subnet.h'''
> > Subnet: not strictly related to this ticket, but still...
> > {{{
> > /// ...... Pool4 and Pool6 should be used instead.
> > }}}
> >
> > The comment seems irrelevant to the class. Perhaps it should be
Subnet4 and Subnet6?
> Good catch.
I saw it fixed now. OK.
>
> > Also, I was going to say that this constructor generates unique subnet
id by itself and that it modifies the global value which affects id of all
Subnet objects created in the future. That should be mentioned in this
constructor or maybe it is even more important to put it in the Subnet4
and Subnet6 constructor documentation.
> It is described in Subnet constructor, because this is where the subnet-
id is assigned. Also
> added brief mention in Subnet4 and Subnet6 constructor docs.
>
> > getNextID: There should be much more text about the intended use of
this function. In particular this function modifies the value of the
subnet id, contrary to its name, which suggests that it returns the next
available value rather than modify it. The caller should be urged in the
documentation that the function modifies the subnet id value for all
Subnet objects.
> Added extra text in Subnet constructor, in getNextID and renamed it to
generateNextID. I think the new names is more appropriate as it is now
obvious that the thing is generated, not merely returned.
ok. It is much clearer now.
>
> Also added a text in BIND10 Guide that explains the current
reconfiguration limitations.
That text is fine. Good idea to put this into the guide.
I don't need to see this ticket again. Whether you keep the todo mentioned
above or not, you're ok to merge this in.
--
Ticket URL: <https://bind10.isc.org/ticket/3234#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list