Opened 6 years ago

Closed 6 years ago

#3171 closed enhancement (complete)

PD: AllocationEngine support for Prefix Delegation, pt.2

Reported by: tomek Owned by: tomek
Priority: high Milestone: Sprint-DHCP-20131016
Component: dhcp6 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: 0
Total Hours: 0 Internal?: no

Description

Once #3146, #3147 and #3150 are done, AllocationEngine? can be extended to support PD.

This is a second part of the work. This first one (#3149) covered API changes/clean ups.

Subtickets

Change History (9)

comment:1 Changed 6 years ago by tomek

  • Owner set to UnAssigned
  • Status changed from new to reviewing

PD support in AllocEngine? implemented. I have updated documentation to explain how the PD allocation process works.

I did convert many v6 address allocation tests to cover both normal v6 addresses and prefixes. I did not do it for every single test out there. If you believe that it should be done, I think a separate ticket for refactoring remaining unit-tests would be the way to go.

Please review.

comment:2 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:3 follow-up: Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit e09660860573d1b76c65a200d3682d3592406b67

Please propose a ChangeLog entry for this ticket.

src/lib/dhcpsrv/alloc_engine.cc

  • increasePrefix:
    • Suggest that the error string ''Prefix operations are for IPv6 only'' is extended to include the string representation of the prefix passed to this function. It would be useful for the debugging purposes.
    • I was hoping that Mr Bean would explain what is going on between line 104 and 106 but apparently he doesn't know DHCP very well. So, what exactly is going on there? :D I had to spend a while writting the variables state on the sheet of paper to understand how this code works.
  • pickAddress: If the pool found, doesn't cast to the Pool6 type, you should rather throw an Unexpected exception, not InvalidParameter to indicate that there is something ''gravely wrong''. Also, the exception string could indclude the pool (iterator) in the textual format (for debugging purposes).
  • pickAddress: There is no need to have pool6 in brackets in this statement:
            next = increasePrefix(last, (pool6)->getLength());
    

src/lib/dhcpsrv/alloc_engine.h

  • pickAddress: Commentary should be exteded to indicate that not only does this function pick an IPv6 address but also IPv6 prefix which is not obvious.
  • Both increaseAddress and increasePrefix could be static instead of const as they don't modify the state of the class.
  • Sorry for the neat pick but both the brief of each increaseAddress and increasePrefix should start with the capital letter.
  • The prefix_len parameter in the increasePrefix could be declared const.
  • increasePrefix documentation: s/increase/increases
  • increaseAddress documentation: it would be useful to have an example of the ''increased'' prefix, just like you did for increasePrefix.
  • Since allocateLease6 returns the collection of leases (as per @return tag), shouldn't function name be allocateLeases6 instead?
  • createLease6: the prefix_len (just like other parameters) could be declared const. The same is true for reuseExpiredLease
  • createLease6: since prefix_len is for PD only it should be mentioned what value it should be assigned when creating a lease for IA_NA. Or, is it simply ignored? The same is the case for reuseExpiredLease

src/lib/dhcpsrv/libdhcpsrv.dox

  • s/At lease 3 allocators will be implemented/At least 3 allocators will be implemented/
  • s/The advantages of this approach are/The advantages of this approach are:/
  • I would suggest that the first paragraph of the new section: ''Prefix Delegation support in Allocation Engine'' is moved to a separate section. I would just leave the second section which is strictly about the PD support which this section should be about.
  • IMHO, the sentence: ''Prefixes are supported'' is ambiguous. Please consider saying: ''The Allocation Engine supports allocation of the IPv6 address prefixes''.

src/lib/dhcpsrv/subnet.cc

  • getPools: It may be worth to static_cast the type when returning exception string in the const variant too.

src/lib/dhcpsrv/subnet.h

  • inPool documentation: perhaps it would be better to say: type of pools to iterate over. Otherwise it sounds like you're passing multiple lease types, not just one. Also, it sounds odd that Lease::Type argument identifies pool type but there is nothing we can now do about it.
  • getPool: Apparently the addr parameter could be passed by const reference. Also, assuming that inRange can be made const, the getPool could be made const too.
  • getPools: brief should start with capital letter.
  • getPools (variable variant): IMHO, the non-const variant would be more appropriate.
  • The non-const variant of getPools is used internally by the Subnet class. In this case, there is no need to make it public. Especially, that it leads to the situation that the class members may be modified externally when returned via non-const variant of the function. However, if you make it private you will end-up having one overload of getPools private and another one public which will cause the compilation to fail for a different reason: compiler will complain that the getPools is private even though you want to access the public (const) variant. I believe, that the reasonable way to overcome this problem is to give the private function a different name: say getPoolsWritable or something like that and use it internally within Subnet class.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

  • NakedIterativeAllocator: This class appears to be the only one which doesn't have any commentary. Please consider adding some brief description of its purpose.
  • Is comment in line 149 still relevant? - ''this is IA_NA, not IA_PD''
  • checkPrefixIncrease doesn't have a comment, while other functions do have.
  • simpleAlloc6Test: you may have meant to add @brief at the beginning of the function description. You may have meant to start the description with the capital letter :D
  • allocWithUsedHintTest function appears to have no description.
  • IterativeAllocatorPrefixIncrease: should cover prefix length = 1 which is allowed by the function under test

comment:4 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit e09660860573d1b76c65a200d3682d3592406b67

Please propose a ChangeLog entry for this ticket.

src/lib/dhcpsrv/alloc_engine.cc

  • increasePrefix:
    • Suggest that the error string ''Prefix operations are for IPv6 only'' is extended to include the string representation of the prefix passed to this function. It would be useful for the debugging purposes.

Done.

  • I was hoping that Mr Bean would explain what is going on between line 104 and 106 but apparently he doesn't know DHCP very well. So, what exactly is going on there? :D I had to spend a while writting the variables state on the sheet of paper to understand how this code works.

Explanation is what kills even the best magic tricks :(
I had to spend some time with a sheet of paper to write it. But it was on a plane, so I had nothing better to do at that time.

  • pickAddress: If the pool found, doesn't cast to the Pool6 type, you should rather throw an Unexpected exception, not InvalidParameter to indicate that there is something ''gravely wrong''. Also, the exception string could indclude the pool (iterator) in the textual format (for debugging purposes).

Yeah, except that there's no method that returns textual representation of a pool. Just added one with two simple unit-tests. The exception is now more verbose.

  • pickAddress: There is no need to have pool6 in brackets in this statement:
            next = increasePrefix(last, (pool6)->getLength());
    

Sorry. It was a leftover when I was using dereferenced iterator there.

src/lib/dhcpsrv/alloc_engine.h

  • pickAddress: Commentary should be exteded to indicate that not only does this function pick an IPv6 address but also IPv6 prefix which is not obvious.

Done.

  • Both increaseAddress and increasePrefix could be static instead of const as they don't modify the state of the class.

Done.

  • Sorry for the neat pick but both the brief of each increaseAddress and increasePrefix should start with the capital letter.

There's a lot of other comments that do not start with capital letters. Fixed some of those. I'm sorry to ruin your expectations, but I'm afraid that we'll have to do a demo with lowercase comments. :P

  • The prefix_len parameter in the increasePrefix could be declared const.

Done.

  • increasePrefix documentation: s/increase/increases

Done.

  • increaseAddress documentation: it would be useful to have an example of the ''increased'' prefix, just like you did for increasePrefix.

Added.

  • Since allocateLease6 returns the collection of leases (as per @return tag), shouldn't function name be allocateLeases6 instead?

Renamed.

  • createLease6: the prefix_len (just like other parameters) could be declared const. The same is true for reuseExpiredLease

No. Because for leases other than PD prefix_len is overridden to be always 128. Updated comment to cleary state that.

  • createLease6: since prefix_len is for PD only it should be mentioned what value it should be assigned when creating a lease for IA_NA. Or, is it simply ignored? The same is the case for reuseExpiredLease

Added explanation. For non-PD leases, the value is actually ignored and set to 128.

src/lib/dhcpsrv/libdhcpsrv.dox

  • s/At lease 3 allocators will be implemented/At least 3 allocators will be implemented/

Done:

  • s/The advantages of this approach are/The advantages of this approach are:/

Done.

  • I would suggest that the first paragraph of the new section: ''Prefix Delegation support in Allocation Engine'' is moved to a separate section. I would just leave the second section which is strictly about the PD support which this section should be about.

I didn't get that comment. Prefix Delegation support in AllocEngine? *is* a separate section.

  • IMHO, the sentence: ''Prefixes are supported'' is ambiguous. Please consider saying: ''The Allocation Engine supports allocation of the IPv6 address prefixes''.

Fixed.

src/lib/dhcpsrv/subnet.cc

  • getPools: It may be worth to static_cast the type when returning exception string in the const variant too.

Done.

src/lib/dhcpsrv/subnet.h

  • inPool documentation: perhaps it would be better to say: type of pools to iterate over. Otherwise it sounds like you're passing multiple lease types, not just one.

Done.

Also, it sounds odd that Lease::Type argument identifies pool type but there is nothing we can now do about it.

We've been over this before. Let's not reopen that discussion.

  • getPool: Apparently the addr parameter could be passed by const reference. Also, assuming that inRange can be made const, the getPool could be made const too.

Done.

  • getPools: brief should start with capital letter.

Done.

  • getPools (variable variant): IMHO, the non-const variant would be more appropriate.

I don't get this comment. There are const and non-const variants. Depending on the case both are used.

  • The non-const variant of getPools is used internally by the Subnet class. In this case, there is no need to make it public. Especially, that it leads to the situation that the class members may be modified externally when returned via non-const variant of the function. However, if you make it private you will end-up having one overload of getPools private and another one public which will cause the compilation to fail for a different reason: compiler will complain that the getPools is private even though you want to access the public (const) variant. I believe, that the reasonable way to overcome this problem is to give the private function a different name: say getPoolsWritable or something like that and use it internally within Subnet class.

C++ purity is not a deliverable on the demo. I can add a @todo to the code if you like, but it is unlikely that anyone would implement this before November.

src/lib/dhcpsrv/tests/alloc_engine_unittest.cc

  • NakedIterativeAllocator: This class appears to be the only one which doesn't have any commentary. Please consider adding some brief description of its purpose.

Added.

  • Is comment in line 149 still relevant? - ''this is IA_NA, not IA_PD''

It is not. Removed.

  • checkPrefixIncrease doesn't have a comment, while other functions do have.

Added.

  • simpleAlloc6Test: you may have meant to add @brief at the beginning of the function description. You may have meant to start the description with the capital letter :D

Capital letters/@brief tag added.

  • allocWithUsedHintTest function appears to have no description.

Added.

  • IterativeAllocatorPrefixIncrease: should cover prefix length = 1 which is allowed by the function under test

Added.

The ticket is back with you, Marcin. Thanks for reviewing this.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

Replying to marcin:

Reviewed commit e09660860573d1b76c65a200d3682d3592406b67

Reviewed commit a019cd162f1ad9584dcc67732ee545c6395bebb4

I fixed one typo in the doxygen, so please pull these changes.

Please propose a ChangeLog entry for this ticket.

Please! :-)

src/lib/dhcpsrv/alloc_engine.cc

  • Sorry for the neat pick but both the brief of each increaseAddress and increasePrefix should start with the capital letter.

There's a lot of other comments that do not start with capital letters. Fixed some of those. I'm sorry to ruin your expectations, but I'm afraid that we'll have to do a demo with lowercase comments. :P

I am depressed but I will try to be a big boy and will not cry.

src/lib/dhcpsrv/libdhcpsrv.dox

  • s/At lease 3 allocators will be implemented/At least 3 allocators will be implemented/
  • I would suggest that the first paragraph of the new section: ''Prefix Delegation support in Allocation Engine'' is moved to a separate section. I would just leave the second section which is strictly about the PD support which this section should be about.

I didn't get that comment. Prefix Delegation support in AllocEngine *is* a separate section.

Sorry. I meant that the AllocEngine section consists of two paragraphs and the first one could be moved to other section. But, treat it suggestion. I don't insist.

src/lib/dhcpsrv/subnet.cc

  • getPools (variable variant): IMHO, the non-const variant would be more appropriate.

I don't get this comment. There are const and non-const variants. Depending on the case both are used.

Sorry. I wasn't clear enough here. I think the commentary should be changed to say:

    /// @brief Returns all pools (non-const variant)

The variable variant sounds ambiguous to me.

  • The non-const variant of getPools is used internally by the Subnet class. In this case, there is no need to make it public. Especially, that it leads to the situation that the class members may be modified externally when returned via non-const variant of the function. However, if you make it private you will end-up having one overload of getPools private and another one public which will cause the compilation to fail for a different reason: compiler will complain that the getPools is private even though you want to access the public (const) variant. I believe, that the reasonable way to overcome this problem is to give the private function a different name: say getPoolsWritable or something like that and use it internally within Subnet class.

C++ purity is not a deliverable on the demo. I can add a @todo to the code if you like, but it is unlikely that anyone would implement this before November.

It is unlikely to be implemented if we put the todo. It is not an extensive change. It is actually very simple and I was able to implement it locally within 5 minutes when doing this review. See:

http://pastebin.com/45R7Ky0C

I wouldn't say it is a C++ purity, but rather a standard way of coding that we struggle to apply everywhere. If you think it is a hassle, please add a @todo.

comment:6 Changed 6 years ago by tomek

  • Milestone changed from Sprint-DHCP-20130918 to Sprint-DHCP-20131016

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

  • Owner changed from tomek to marcin

Replying to marcin:

I fixed one typo in the doxygen, so please pull these changes.

Thanks.

Please propose a ChangeLog entry for this ticket.

Please! :-)

Since you asked so nicely, I just added it :)

Sorry. I meant that the AllocEngine section consists of two paragraphs and the first one could be moved to other section. But, treat it suggestion. I don't insist.

Ok. That makes sense. Added new section. We now have 2 very small sections, but hopefully they will grow over time.

src/lib/dhcpsrv/subnet.cc

  • getPools (variable variant): IMHO, the non-const variant would be more appropriate.

I don't get this comment. There are const and non-const variants. Depending on the case both are used.

Sorry. I wasn't clear enough here. I think the commentary should be changed to say:

    /// @brief Returns all pools (non-const variant)

Comment clarified.

C++ purity is not a deliverable on the demo. I can add a @todo to the code if you like, but it is unlikely that anyone would implement this before November.

It is unlikely to be implemented if we put the todo. It is not an extensive change. It is actually very simple and I was able to implement it locally within 5 minutes when doing this review. See:

http://pastebin.com/45R7Ky0C

I applied that patch, but as we discussed on jabber, it doesn't solve the problem completely. It would require defining PoolPtr?, ConstPoolPtr?, Pool4Ptr, ConstPool4Ptr, Pool6Ptr and ConstPool6Ptr and the casting between them as needed. I think that applying your proposed change was a reasonable compromise.

I think that addresses all your comments. If you think that the ChangeLog? commited is ok, the ticket should be ready to merge.

comment:8 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit 7d1431b4c887f0c7ee1b26b9b82d3d3b8464b34f

Yes, you did address all my comments. Thank you! Please merge this stuff to master.

comment:9 Changed 6 years ago by tomek

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

Merged. Thanks a lot for your review.

Note: See TracTickets for help on using tickets.