Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UPSTREAM: <carry>: ip allocator with reserved addresses #921

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/registry/core/rest/storage_core.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi
}

serviceClusterIPAllocator, err := ipallocator.NewAllocatorCIDRRange(&serviceClusterIPRange, func(max int, rangeSpec string) (allocator.Interface, error) {
mem := allocator.NewAllocationMap(max, rangeSpec)
mem := allocator.NewAllocationMapReserved(max, rangeSpec)
// TODO etcdallocator package to return a storage interface via the storageFactory
etcd, err := serviceallocator.NewEtcd(mem, "/ranges/serviceips", api.Resource("serviceipallocations"), serviceStorageConfig)
if err != nil {
Expand All @@ -218,7 +218,7 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi
if utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) && c.SecondaryServiceIPRange.IP != nil {
var secondaryServiceClusterIPRegistry rangeallocation.RangeRegistry
secondaryServiceClusterIPAllocator, err = ipallocator.NewAllocatorCIDRRange(&c.SecondaryServiceIPRange, func(max int, rangeSpec string) (allocator.Interface, error) {
mem := allocator.NewAllocationMap(max, rangeSpec)
mem := allocator.NewAllocationMapReserved(max, rangeSpec)
aojea marked this conversation as resolved.
Show resolved Hide resolved
// TODO etcdallocator package to return a storage interface via the storageFactory
etcd, err := serviceallocator.NewEtcd(mem, "/ranges/secondaryserviceips", api.Resource("serviceipallocations"), serviceStorageConfig)
if err != nil {
Expand Down
73 changes: 73 additions & 0 deletions pkg/registry/core/service/allocator/patch_bitmap.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
Copyright 2015 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package allocator

import (
"math/big"
"math/rand"
"time"
)

// NewAllocationMapReserved creates an allocation bitmap using a modified random scan strategy
// that maintains a reserved offset for OpenShift.
func NewAllocationMapReserved(max int, rangeSpec string) *AllocationBitmap {
// OpenShift Reserved Offsets:
reserved := make(map[int]struct{})
// - OpenShift DNS always uses the .10 address (0 counts so we reserve the 9 offset)
reserved[9] = struct{}{}

a := AllocationBitmap{
strategy: randomScanReservedStrategy{
rand: rand.New(rand.NewSource(time.Now().UnixNano())),
reserved: reserved,
},
allocated: big.NewInt(0),
count: 0,
max: max,
rangeSpec: rangeSpec,
}
return &a
}

// randomScanReservedStrategy chooses a random address from the provided big.Int, and then
// scans forward looking for the next available address (it will wrap the range if necessary).
aojea marked this conversation as resolved.
Show resolved Hide resolved
// randomScanReservedStrategy omit some offsets that are "special" so they can't be allocated
// randomly, only explicitly
type randomScanReservedStrategy struct {
rand *rand.Rand
reserved map[int]struct{}
}

func (rss randomScanReservedStrategy) AllocateBit(allocated *big.Int, max, count int) (int, bool) {
if count >= max {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added TestPostAllocateReservedFull_BitmapReserved and TestPreAllocateReservedFull_BitmapReserved test cases

return 0, false
}
offset := rss.rand.Intn(max)
for i := 0; i < max; i++ {
at := (offset + i) % max
// skip reserved values
if _, ok := rss.reserved[at]; ok {
continue
}
if allocated.Bit(at) == 0 {
return at, true
}
}
return 0, false
}

var _ bitAllocator = randomScanReservedStrategy{}
186 changes: 186 additions & 0 deletions pkg/registry/core/service/allocator/patch_bitmap_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
/*
Copyright 2015 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package allocator

import (
"testing"

"k8s.io/apimachinery/pkg/util/sets"
)

func TestAllocate_BitmapReserved(t *testing.T) {
max := 20
m := NewAllocationMapReserved(max, "test")

if _, ok, _ := m.AllocateNext(); !ok {
t.Fatalf("unexpected error")
}
if m.count != 1 {
t.Errorf("expect to get %d, but got %d", 1, m.count)
}
if f := m.Free(); f != max-1 {
t.Errorf("expect to get %d, but got %d", max-1, f)
}
}

// TestAllocateMaxReserved depends on the number of reserved offsets
// Currently there is only one value reserved, if this value change
// in the future the test has to be modified accordingly
func TestAllocateMax_BitmapReserved(t *testing.T) {
max := 20
// modify if necessary
reserved := 1
m := NewAllocationMapReserved(max, "test")
for i := 0; i < max-reserved; i++ {
if _, ok, _ := m.AllocateNext(); !ok {
t.Fatalf("unexpected error")
}
}

if _, ok, _ := m.AllocateNext(); ok {
t.Errorf("unexpected success")
}
if f := m.Free(); f != reserved {
t.Errorf("expect to get %d, but got %d", 0, f)
}
}

func TestAllocateError_BitmapReserved(t *testing.T) {
m := NewAllocationMapReserved(20, "test")
if ok, _ := m.Allocate(3); !ok {
t.Errorf("error allocate offset %v", 3)
}
if ok, _ := m.Allocate(3); ok {
t.Errorf("unexpected success")
}
}

// 9 is a reserved value used for OpenshiftDNS
// it can only be allocated explicitly using Allocate()
func TestAllocateReservedOffset_BitmapReserved(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a test that tries to allocate the .10 address explicitly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

m := NewAllocationMapReserved(20, "test")
if ok, _ := m.Allocate(9); !ok {
t.Errorf("error allocate offset %v", 9)
aojea marked this conversation as resolved.
Show resolved Hide resolved
}
if ok, _ := m.Allocate(9); ok {
t.Errorf("unexpected success")
}
}

func TestPreAllocateReservedFull_BitmapReserved(t *testing.T) {
max := 20
reserved := 1
m := NewAllocationMapReserved(max, "test")
// Allocate the reserved value
if ok, _ := m.Allocate(9); !ok {
t.Errorf("error allocate offset %v", 9)
}
// Allocate all possible values except the reserved
for i := 0; i < max-reserved; i++ {
if _, ok, _ := m.AllocateNext(); !ok {
t.Fatalf("unexpected error")
}
}

if _, ok, _ := m.AllocateNext(); ok {
t.Errorf("unexpected success")
}
if m.count != max {
t.Errorf("expect to get %d, but got %d", max, m.count)
}
if f := m.Free(); f != 0 {
t.Errorf("expect to get %d, but got %d", max-1, f)
}
}

func TestPostAllocateReservedFull_BitmapReserved(t *testing.T) {
max := 20
reserved := 1
m := NewAllocationMapReserved(max, "test")

// Allocate all possible values except the reserved
for i := 0; i < max-reserved; i++ {
if _, ok, _ := m.AllocateNext(); !ok {
t.Fatalf("unexpected error")
}
}

if _, ok, _ := m.AllocateNext(); ok {
t.Errorf("unexpected success")
}
// Allocate the reserved value
if ok, _ := m.Allocate(9); !ok {
t.Errorf("error allocate offset %v", 9)
}
if m.count != max {
t.Errorf("expect to get %d, but got %d", max, m.count)
}
if f := m.Free(); f != 0 {
t.Errorf("expect to get %d, but got %d", max-1, f)
}
}

func TestRelease_BitmapReserved(t *testing.T) {
offset := 3
m := NewAllocationMapReserved(20, "test")
if ok, _ := m.Allocate(offset); !ok {
t.Errorf("error allocate offset %v", offset)
}

if !m.Has(offset) {
t.Errorf("expect offset %v allocated", offset)
}

if err := m.Release(offset); err != nil {
t.Errorf("unexpected error: %v", err)
}

if m.Has(offset) {
t.Errorf("expect offset %v to have been released", offset)
}
}

func TestForEach_BitmapReserved(t *testing.T) {
testCases := []sets.Int{
sets.NewInt(),
sets.NewInt(0),
sets.NewInt(0, 2, 5, 9),
sets.NewInt(0, 1, 2, 3, 4, 5, 6, 7, 8, 9),
}

for i, tc := range testCases {
m := NewAllocationMapReserved(20, "test")
for offset := range tc {
if ok, _ := m.Allocate(offset); !ok {
t.Errorf("[%d] error allocate offset %v", i, offset)
}
if !m.Has(offset) {
t.Errorf("[%d] expect offset %v allocated", i, offset)
}
}
calls := sets.NewInt()
m.ForEach(func(i int) {
calls.Insert(i)
})
if len(calls) != len(tc) {
t.Errorf("[%d] expected %d calls, got %d", i, len(tc), len(calls))
}
if !calls.Equal(tc) {
t.Errorf("[%d] expected calls to equal testcase: %v vs %v", i, calls.List(), tc.List())
}
}
}
Loading