UPSTREAM: <carry>: ip allocator with reserved addresses #921
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
|
/assign @deads2k @Miciah Openshift DNS IP reserved, but I left it open because it may be needed for other later |
aojea
force-pushed
the
reserved_random_allocation
branch
from
715cc3f to
0282d20
Compare
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
aojea
force-pushed
the
reserved_random_allocation
branch
from
0282d20 to
564ebc6
Compare
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
aojea
force-pushed
the
reserved_random_allocation
branch
from
564ebc6 to
7e5bf29
Compare
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
|
/approve |
|
/cc @candita |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
aojea
force-pushed
the
reserved_random_allocation
branch
from
2532849 to
a409486
Compare
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
|
/retest |
| } | ||
|
|
||
| func (rss randomScanStrategyReserved) AllocateBit(allocated *big.Int, max, count int) (int, bool) { | ||
| if count >= max { |
There was a problem hiding this comment.
Should you account for the reserved address here, and check if count + 1 >= max? E.g. if you have assigned everything except .10, the reserved one, yet something is still trying to allocate an address with this, will it keep trying even though all addresses have been either handed out or reserved?
There was a problem hiding this comment.
the number of addresses available remains the same, the only difference is that the reserved can not be allocated randomly, but still can be allocated directly and should be counted.
The for loop below skip the reserved values and exit after iterating over the whole range
There was a problem hiding this comment.
Let me put it another way - when all that is left is the reserved address, I expect that this will just return 0, false without iterating over the whole range. Does it do that?
There was a problem hiding this comment.
that is indeed an optimization, but the problem is that you don't know beforehand if the missing address is the reserved or not.
Assume an allocator with size 4 and the value 1 is the reserved.
Case A:
(1,2,3) values reserved, count=3 , we bail out and don't allocate 4
Case B:
(2,3,4) values reserved, count=3, we bail out because we can't allocate 1 here that is correct
The algorithms keeps being O(n) and the code is much simpler, I think we are good this way
There was a problem hiding this comment.
added TestPostAllocateReservedFull_BitmapReserved and TestPreAllocateReservedFull_BitmapReserved test cases
|
|
||
| // 9 is a reserved value used for OpenshiftDNS | ||
| // it can only be allocated explicitly using Allocate() | ||
| func TestAllocateReservedOffset_BitmapReserved(t *testing.T) { |
There was a problem hiding this comment.
Can you create a test that tries to allocate the .10 address explicitly?
There was a problem hiding this comment.
The test in pkg/registry/core/service/ipallocator/patch_allocator_test.go tests that with ip addresses.
It first randomly allocate as much addresses as possible, and then allocates the .10.
aojea
force-pushed
the
reserved_random_allocation
branch
from
a409486 to
4d9cf9b
Compare
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
| } | ||
| found.Insert(ip.String()) | ||
| } | ||
| if _, err := r.AllocateNext(); err != ErrFull { |
There was a problem hiding this comment.
This is the test that exercises the new random/reserved strategy best. Can you point it out?
This adds a new bitmap allocator used by the Services IP allocators. This new bitmap allocator allows to reserve certain offsets, so this offsets can not be allocated randomly. This offsets are hardcoded on the code and are not externally configurable. This is required to guarantee that certain services, in this case the Openshift DNS service that always uses the IP with offset 10, ie.. in 192.168.0.0/24 it will use 192.168.0.10, doesn't race with other components that create services with random allocated IPs. Signed-off-by: Antonio Ojea <[email protected]>
aojea
force-pushed
the
reserved_random_allocation
branch
from
4d9cf9b to
f300734
Compare
|
@aojea: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
|
|
@aojea: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
better solution in kubernetes/enhancements#3071 |
/kind feature
/kind bug