Test overlapping pool allocation #2173
Conversation
|
I generated the test data with a simple Python script which does a brute force search for overlapping subnets in various configurations. I can add the script to this PR, but I'm not sure where it should go in the repo. Generating the overlaps in this way in the test itself is likely to be too slow - the chance of two random subnets overlapping seems to be about 12% in IPv4 space and 3% in IPv6 space, so you would need lots of trials which would make the test very slow. |
Codecov Report
@@ Coverage Diff @@
## master #2173 +/- ##
=========================================
Coverage ? 40.58%
=========================================
Files ? 139
Lines ? 22496
Branches ? 0
=========================================
Hits ? 9129
Misses ? 12044
Partials ? 1323
Continue to review full report at Codecov.
|
|
@euanh the base PR got merged, can you rebase? |
|
@fcrisciani Rebased. |
|
@abhi PTAL |
abhi
left a comment
abhi
left a comment
There was a problem hiding this comment.
Overall LGTM. Thanks for adding the comments. Its nice to test every combination in the test.
But I feel that the input testing table can be compressed considerably since its testing similar things.
ipam/allocator_test.go
Outdated
| ok bool | ||
| }{ | ||
| // Ok: Non-overlapping left and right | ||
| {[]string{"10.0.0.0/8"}, "11.0.0.0/8", true}, |
There was a problem hiding this comment.
I think some of these lists can be compressed because they test the same aspect of it.
There was a problem hiding this comment.
for eg {[]string{"10.0.0.0/8"}, "11.0.0.0/8", true}, and {[]string{"10.0.0.0/8"}, "9.0.0.0/8", true}, are pretty much same and so are the rest of rows in "ok" .
ipam/allocator_test.go
Outdated
| {[]string{"10.0.0.0/8"}, "10.0.0.0/7", false}, // superset | ||
| {[]string{"10.0.0.0/8"}, "10.0.0.0/6", false}, // superset, non-canonical | ||
| {[]string{"10.0.0.0/8"}, "8.0.0.0/6", false}, // superset, canonical | ||
| {[]string{"10.0.0.0/8"}, "10.0.0.0/9", false}, // subset |
There was a problem hiding this comment.
{[]string{"10.0.0.0/8"}, "10.0.0.0/9", false}, and {[]string{"10.0.0.0/8"}, "10.0.0.0/16", false}, and {[]string{"10.0.0.0/8"}, "10.0.0.0/24", false} are verifying similar things ?
|
|
||
| // IPv4 | ||
| // Previously allocated network does not overlap with request | ||
| {[]string{"74.0.0.0/7"}, "9.111.99.72/30", true}, |
There was a problem hiding this comment.
how are these different from the "ok" section ?
There was a problem hiding this comment.
These are script-generated test cases which cover the same property. I can combine them with the earlier ones and keep some of both - the 10.0.0.0 examples are easier to read and understand.
| {[]string{"30.0.0.0/7"}, "31.56.203.32/27", false}, | ||
| {[]string{"135.0.0.0/9"}, "135.48.0.0/16", false}, | ||
|
|
||
| // Previously allocated network overlaps beginning of request |
There was a problem hiding this comment.
same this section is similar to "not ok" section ?
|
@euanh can you reply/address the comments so to make progress with this PR? |
|
@fcrisciani Yup, I took a look at @abhi's comments on Friday but then got sidetracked on #2193 yesterday. I can condense the test cases, but I still want to have a couple of point tests per category. Originally I wanted to have a couple of point tests and a property-based test for each category, but generating suitable test data efficiently isn't straightforward. |
|
ping @abhi |
abhi
left a comment
abhi
left a comment
There was a problem hiding this comment.
LGTM. Please squash commits
Signed-off-by: Euan Harris <euan.harris@docker.com>
TestOverlappingRequests checks that pool requests which are supersets or subsets of existing allocations, and those which overlap with existing allocations at the beginning or the end. Multiple allocation is now tested by TestOverlappingRequests, so TestDoublePoolRelease only needs to test double releasing. Signed-off-by: Euan Harris <euan.harris@docker.com>
|
@abhi I've squashed to two commits, one for comment changes and the other for test changes. |
4 participants
This pull adds some unit tests for the changes to reject overlapping pool allocations in #2148.