Opened 6 years ago

Closed 6 years ago

#3150 closed enhancement (complete)

PD: CfgMgr must be able to store info about Prefix Delegation

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130918
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

We need storage containers for prefix delegation configuration.

Subtickets

Change History (8)

comment:1 Changed 6 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20130918

comment:2 Changed 6 years ago by tomek

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

comment:3 Changed 6 years ago by tomek

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

The code is ready for review. It took longer than expected, because due to API change, not only CfgMgr?, but also AllocEngine? and DHCPv4 server code had to be updated. The changes are quite simple, though.

This is reasonably sized ticket (diff is less than 1500 lines).

There are many places in AllocEngine? that do not take advantage of that new API or otherwise are looking incomplete. That will be done as a part of ticket #3149.

Last edited 6 years ago by tomek (previous) (diff)

comment:4 Changed 6 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:5 follow-up: Changed 6 years ago by tmark

  • Owner changed from tmark to tomek

Review Comments:

This is twisty stuff, I'm glad you know what we're doing ;)


dhcpsrv/pool.h

In your commentary at line 187:

    /// Obviously, prefix_len must define bigger prefix than delegated_len,
    /// so prefix_len < delegated_len. Note that it is slightly confusing:
    /// bigger (larger) prefix actually has smaller prefix length, e.g.
    /// /56 is a bigger prefix than /64.
    ///

"prefix_len must define bigger prefix", isn't quite true. It must specify a SHORTER prefix.

I think if you alter your choice of words a bit and discuss the "size" of prefixes as shorter or longer, while the lengths (_len values) are smaller or larger, it's a bit easier to follow.

A smaller length yields a shorter prefix which describes a larger set of addresses.
A larger length yields a longer prefix which describes a smaller set of addresses.

A numerically larger _len value describes a longer prefix, which specifies more of the actual
address. The delegated length must yield a LONGER prefix the prefix length and therefore it's length value must be larger.

There's nothing really wrong here, I just think if you adopt the above semantics it makes the discussion clearer.


dhcpsrv/pool.cc:

Per the above, your error message should say "must be longer than prefix length"

line 115:

    if (prefix_len > delegated_len) {
        isc_throw(BadValue, "Delegated length (" << static_cast<int>(delegated_len)
                  << ") must be smaller than prefix length ("
                  << static_cast<int>(prefix_len) << ")");
    }

dhcpsrv/Allocator.h

If you are going to use a new enumeration, PoolType, for this then you should rename this
instance member to pool_type_. There is already Lease6::LeaseType? and these values are not the quite same.

        /// @brief defines lease type allocation
        Pool::PoolType lease_type_;

Perhaps we shouldn't have two sets of enums at all. I think it would be a cleaner, more universal approach to extract LeaseType out of Lease6 and add a value of LEASE_V4, and later we can add IA_PA etc. I think we are confusing matters with having two sets of enums, that are this closely related, especially since you were inclined to name the member above "lease_type_".

Regarding PoolType, why did you uses specific values?


dhcpsrv/subnet.h/cc

Using a explicit instance members for last allocated address and pool collection per type seems a bit awkward. For one thing the base class, Subnet, now has members that apply only to Subnet6. It also means the same essential switch statement is appearing often.

What if you were to replace:

typedef std::vector<PoolPtr> PoolCollection with a class:

with a class, something like this:

class PoolCollection {
    LeaseType/PoolType type_
    IOAddress last_allocated_addr_
    std::vector<PoolPtr> pools_

    void push_back(PoolPtr )  // wraps vector<>.push_back
    operator[]                // wraps vector [] operator
    }
}

You could then add a map, to Subnet something like this:

    std::map<PoolType, PoolCollection> pools_map_; 

Implement an accessor to return the appropriate PoolCollection instance
from the map based on type or throw if not found.

Derivations (Subnet4, Subnet6), then only add collections of supported types
to their pools_map_.

I'm not sure how the Hashed and Random allocators will work, but they might be
able to leverage this as well, in the event they need additional stateful values
per specific pooltype collection.

Maybe as a ticket to refactor?


subnet_unittest.cc

In TEST(Subnet4Test, PoolType?), your calls to addPool() for the valid V4 pools
should be wrapped in EXPECT_NO_THROWs.


subnet_unittest.cc

In TEST(Subnet6Test, PoolType?), your calls to addPool() for the valid V6 pools
should be wrapped in EXPECT_NO_THROWs.


cppcheck and valgrind on unit tests were clean.

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

  • Owner changed from tomek to tmark

Replying to tmark:

Review Comments:

This is twisty stuff, I'm glad you know what we're doing ;)

I don't, but a good mix of acting skills and self-confidence does wonders ;)


dhcpsrv/pool.h

In your commentary at line 187:

    /// Obviously, prefix_len must define bigger prefix than delegated_len,
    /// so prefix_len < delegated_len. Note that it is slightly confusing:
    /// bigger (larger) prefix actually has smaller prefix length, e.g.
    /// /56 is a bigger prefix than /64.
    ///

"prefix_len must define bigger prefix", isn't quite true. It must specify a SHORTER prefix.

I think if you alter your choice of words a bit and discuss the "size" of prefixes as shorter or longer, while the lengths (_len values) are smaller or larger, it's a bit easier to follow.

A smaller length yields a shorter prefix which describes a larger set of addresses.
A larger length yields a longer prefix which describes a smaller set of addresses.

A numerically larger _len value describes a longer prefix, which specifies more of the actual
address. The delegated length must yield a LONGER prefix the prefix length and therefore it's length value must be larger.

Yup, that's confusing. I tried to clarify it a bit, using parts of your comments. Let me know if it is better now.

dhcpsrv/pool.cc:

Per the above, your error message should say "must be longer than prefix length"

Done.

dhcpsrv/Allocator.h

Eeh, no such file :)

If you are going to use a new enumeration, PoolType, for this then you should rename this
instance member to pool_type_. There is already Lease6::LeaseType? and these values are not the quite same.

They are similar, but different. I agree that its name was not the best one, so I renamed it to pool_type_. There are 4 pool types, but there's only 3 lease6 types. If we decide to stick v4 and v6 addresses in one table, we may have the same enum. But I think it would be a grave mistake - lots of problems for very little gain.

Perhaps we shouldn't have two sets of enums at all. I think it would be a cleaner, more universal approach to extract LeaseType out of Lease6 and add a value of LEASE_V4, and

But there's no such thing as V4 lease6.

Regarding PoolType, why did you uses specific values?

Just for the pleasure of maintaining a tradition of abusing enums. Ok, removed.


dhcpsrv/subnet.h/cc

Using a explicit instance members for last allocated address and pool collection per type seems a bit awkward. For one thing the base class, Subnet, now has members that apply only to Subnet6. It also means the same essential switch statement is appearing often.

...

Maybe as a ticket to refactor?

Added ticket #3168, referencing your comment. I agree that the code is not the most elegant stuff.

subnet_unittest.cc
In TEST(Subnet4Test, PoolType?), your calls to addPool() for the valid V4 pools
should be wrapped in EXPECT_NO_THROWs.

Added.

subnet_unittest.cc
In TEST(Subnet6Test, PoolType?), your calls to addPool() for the valid V6 pools
should be wrapped in EXPECT_NO_THROWs.

Added.

cppcheck and valgrind on unit tests were clean.

Thanks for checking.

Ticket is back with you. Thanks for very quick review.

comment:7 Changed 6 years ago by tmark

  • Owner changed from tmark to tomek

I am fine with the changes and responses above. Please merge the changes.

comment:8 Changed 6 years ago by tomek

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

Thanks for your quick review.

Note: See TracTickets for help on using tickets.