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.

Sign up for GitHub

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

Jump to bottom

Fix for Duplicate IP issues #2105

Merged
merged 3 commits into from
Mar 9, 2018
Merged

Fix for Duplicate IP issues #2105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7df562
Fixing Duplicate IP issue in IPAM library
abhi Mar 8, 2018
48668c3
Fixing concurrent map access
abhi Mar 8, 2018
c6843ee
IPAM and Bitseq test cases
abhi Mar 8, 2018
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
73 changes: 49 additions & 24 deletions bitseq/sequence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ func (s *sequence) getAvailableBit(from uint64) (uint64, uint64, error) {
bitSel >>= 1
bits++
}
// Check if the loop exited because it could not
// find any available bit int block starting from
// "from". Return invalid pos in that case.
if bitSel == 0 {
return invalidPos, invalidPos, ErrNoBitAvailable
}
return bits / 8, bits % 8, nil
}

Expand Down Expand Up @@ -313,14 +319,14 @@ func (h *Handle) set(ordinal, start, end uint64, any bool, release bool, serial
curr := uint64(0)
h.Lock()
store = h.store
h.Unlock()
if store != nil {
h.Unlock() // The lock is acquired in the GetObject
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears store.GetObject will modify h (through the SetValue and SetIndex calls). If so, shouldn't we keep h locked while that is happening? Also, it appears the lock acquired in GetObject is a datastore lock rather than the object's lock. Is h.Unlock() needed and correct before the GetObject call invoked on h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way the store logic works in libnetwork is , the in memory data structure gets overloaded by the store data. So there need not be lock going into the GetObjec

if err := store.GetObject(datastore.Key(h.Key()...), h); err != nil && err != datastore.ErrKeyNotFound {
Copy link
Contributor

@ddebroy ddebroy Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

h.unlock() will be needed at this point since we are returning. Potentially in a later patch, do you think the code can be restructured a little bit to just defer unlocks (instead of the unlock/lock sequences) after grabbing a lock on h before the for loop. I.e.

h.lock()
defer h.unlock()
for {
    .
    .
    .
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I hate the sequence too. But because of the way we handle differently for swarmkit and native, its kind of needed like this. I will work on the refactor next.

return ret, err
}
h.Lock() // Acquire the lock back
}

h.Lock()
logrus.Debugf("Received set for ordinal %v, start %v, end %v, any %t, release %t, serial:%v curr:%d \n", ordinal, start, end, any, release, serial, h.curr)
if serial {
curr = h.curr
}
Expand All @@ -346,7 +352,6 @@ func (h *Handle) set(ordinal, start, end uint64, any bool, release bool, serial

// Create a private copy of h and work on it
nh := h.getCopy()
h.Unlock()

nh.head = pushReservation(bytePos, bitPos, nh.head, release)
if release {
Expand All @@ -355,22 +360,25 @@ func (h *Handle) set(ordinal, start, end uint64, any bool, release bool, serial
nh.unselected--
}

// Attempt to write private copy to store
if err := nh.writeToStore(); err != nil {
if _, ok := err.(types.RetryError); !ok {
return ret, fmt.Errorf("internal failure while setting the bit: %v", err)
if h.store != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check h.store again at this point? It seems we already check earlier in the function. If we lock h outside the for early in the function, I think h.store should not become nil mid-way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to differentiate between a store logic and a non store logic codepath. I need to unlock before going to write store it will lead to deadlock.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it regarding the logic to differentiate store/non-store codepaths.

For the locks, it looks like we are calling nh.writeToStore() below which will lock nh's mutex and not that of h, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep thats right. All the more reason not to have a lock around the operation right ? I retained the logic needed for store code path the way it was.

h.Unlock()
// Attempt to write private copy to store
if err := nh.writeToStore(); err != nil {
if _, ok := err.(types.RetryError); !ok {
return ret, fmt.Errorf("internal failure while setting the bit: %v", err)
}
// Retry
continue
}
// Retry
continue
h.Lock()
}

// Previous atomic push was succesfull. Save private copy to local copy
h.Lock()
defer h.Unlock()
h.unselected = nh.unselected
h.head = nh.head
h.dbExists = nh.dbExists
h.dbIndex = nh.dbIndex
h.Unlock()
return ret, nil
}
}
Expand Down Expand Up @@ -498,24 +506,40 @@ func (h *Handle) UnmarshalJSON(data []byte) error {
func getFirstAvailable(head *sequence, start uint64) (uint64, uint64, error) {
// Find sequence which contains the start bit
byteStart, bitStart := ordinalToPos(start)
Copy link

Choose a reason for hiding this comment

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

as a reminder for myself, byteStart and bitStart here are relative to the first byte of the block where the ordinal start falls into

current, _, _, inBlockBytePos := findSequence(head, byteStart)

current, _, precBlocks, inBlockBytePos := findSequence(head, byteStart)
Copy link

Choose a reason for hiding this comment

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

current is the pointer to the block where byteStart fall into
precBlocks, is the number of blocks within the same block where the byteStart fall into
example: head->0xFFFF00FF|10 and my byte start is 5 because my ordinal is 42, in this case precBlocks is 1, indicating that my ordinal falls in the second compressed block

// Derive the this sequence offsets
byteOffset := byteStart - inBlockBytePos
bitOffset := inBlockBytePos*8 + bitStart
var firstOffset uint64
if current == head {
firstOffset = byteOffset
}
for current != nil {
if current.block != blockMAX {
// If the current block is not full, check if there is any bit
// from the current bit in the current block. If not, before proceeding to the
// next block node, make sure we check for available bit in the next
// instance of the same block. Due to RLE same block signature will be
// compressed.
retry:
bytePos, bitPos, err := current.getAvailableBit(bitOffset)
if err != nil && precBlocks == current.count-1 {
// This is the last instance in the same block node,
// so move to the next block.
goto next
}
if err != nil {
// There are some more instances of the same block, so add the offset
Copy link

Choose a reason for hiding this comment

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

this is only possible in the case of serial allocation, where the start can be with in the block itself

// and be optimistic that you will find the available bit in the next
// instance of the same block.
bitOffset = 0
byteOffset += blockBytes
precBlocks++
goto retry
}
return byteOffset + bytePos, bitPos, err
}
// Moving to next block: Reset bit offset.
next:
bitOffset = 0
byteOffset += (current.count * blockBytes) - firstOffset
firstOffset = 0
byteOffset += (current.count * blockBytes) - (precBlocks * blockBytes)
precBlocks = 0
current = current.next
}
return invalidPos, invalidPos, ErrNoBitAvailable
Expand All @@ -526,19 +550,20 @@ func getFirstAvailable(head *sequence, start uint64) (uint64, uint64, error) {
// This can be further optimized to check from start till curr in case of a rollover
func getAvailableFromCurrent(head *sequence, start, curr, end uint64) (uint64, uint64, error) {
var bytePos, bitPos uint64
var err error
if curr != 0 && curr > start {
bytePos, bitPos, _ = getFirstAvailable(head, curr)
bytePos, bitPos, err = getFirstAvailable(head, curr)
ret := posToOrdinal(bytePos, bitPos)
if end < ret {
if end < ret || err != nil {
goto begin
}
return bytePos, bitPos, nil
}

begin:
bytePos, bitPos, _ = getFirstAvailable(head, start)
bytePos, bitPos, err = getFirstAvailable(head, start)
ret := posToOrdinal(bytePos, bitPos)
if end < ret {
if end < ret || err != nil {
return invalidPos, invalidPos, ErrNoBitAvailable
}
return bytePos, bitPos, nil
Expand Down
61 changes: 55 additions & 6 deletions bitseq/sequence_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ func TestGetFirstAvailable(t *testing.T) {
{&sequence{block: 0xffffffff, count: 1, next: &sequence{block: 0xfffffffe, count: 1, next: &sequence{block: 0xffffffff, count: 6}}}, 7, 7, 0},

{&sequence{block: 0xffffffff, count: 2, next: &sequence{block: 0x0, count: 6}}, 8, 0, 0},
{&sequence{block: 0xfffcffff, count: 1, next: &sequence{block: 0x0, count: 6}}, 4, 0, 16},
{&sequence{block: 0xfffcffff, count: 1, next: &sequence{block: 0x0, count: 6}}, 1, 7, 15},
{&sequence{block: 0xfffcffff, count: 1, next: &sequence{block: 0x0, count: 6}}, 1, 6, 10},
{&sequence{block: 0xfffcfffe, count: 1, next: &sequence{block: 0x0, count: 6}}, 3, 7, 31},
{&sequence{block: 0xfffcffff, count: 1, next: &sequence{block: 0xffffffff, count: 6}}, invalidPos, invalidPos, 31},
}

for n, i := range input {
Expand Down Expand Up @@ -1238,7 +1243,7 @@ func TestIsCorrupted(t *testing.T) {
}
}

func TestSetRollover(t *testing.T) {
func testSetRollover(t *testing.T, serial bool) {
ds, err := randomLocalStore()
if err != nil {
t.Fatal(err)
Expand All @@ -1253,7 +1258,7 @@ func TestSetRollover(t *testing.T) {

// Allocate first half of the bits
for i := 0; i < numBits/2; i++ {
_, err := hnd.SetAny(true)
_, err := hnd.SetAny(serial)
if err != nil {
t.Fatalf("Unexpected failure on allocation %d: %v\n%s", i, err, hnd)
}
Expand All @@ -1276,12 +1281,12 @@ func TestSetRollover(t *testing.T) {
}
}
if hnd.Unselected() != uint64(3*numBits/4) {
t.Fatalf("Expected full sequence. Instead found %d free bits.\nSeed: %d.\n%s", hnd.unselected, seed, hnd)
t.Fatalf("Unexpected free bits: found %d free bits.\nSeed: %d.\n%s", hnd.unselected, seed, hnd)
}

//request to allocate for remaining half of the bits
for i := 0; i < numBits/2; i++ {
_, err := hnd.SetAny(true)
_, err := hnd.SetAny(serial)
if err != nil {
t.Fatalf("Unexpected failure on allocation %d: %v\nSeed: %d\n%s", i, err, seed, hnd)
}
Expand All @@ -1294,19 +1299,63 @@ func TestSetRollover(t *testing.T) {
}

for i := 0; i < numBits/4; i++ {
_, err := hnd.SetAny(true)
_, err := hnd.SetAny(serial)
if err != nil {
t.Fatalf("Unexpected failure on allocation %d: %v\nSeed: %d\n%s", i, err, seed, hnd)
}
}
//Now requesting to allocate the unallocated random bits (qurter of the number of bits) should
//leave no more bits that can be allocated.
if hnd.Unselected() != 0 {
t.Fatalf("Unexpected number of unselected bits %d, Expected %d", hnd.Unselected(), numBits/4)
t.Fatalf("Unexpected number of unselected bits %d, Expected %d", hnd.Unselected(), 0)
}

err = hnd.Destroy()
if err != nil {
t.Fatal(err)
}
}

func TestSetRollover(t *testing.T) {
testSetRollover(t, false)
}

func TestSetRolloverSerial(t *testing.T) {
testSetRollover(t, true)
}

func TestGetFirstAvailableFromCurrent(t *testing.T) {
input := []struct {
mask *sequence
bytePos uint64
bitPos uint64
start uint64
curr uint64
end uint64
}{
{&sequence{block: 0xffffffff, count: 2048}, invalidPos, invalidPos, 0, 0, 65536},
{&sequence{block: 0x0, count: 8}, 0, 0, 0, 0, 256},
{&sequence{block: 0x80000000, count: 8}, 1, 0, 0, 8, 256},
{&sequence{block: 0xC0000000, count: 8}, 0, 2, 0, 2, 256},
{&sequence{block: 0xE0000000, count: 8}, 0, 3, 0, 0, 256},
{&sequence{block: 0xFFFB1FFF, count: 8}, 2, 0, 14, 0, 256},
{&sequence{block: 0xFFFFFFFE, count: 8}, 3, 7, 0, 0, 256},

{&sequence{block: 0xffffffff, count: 1, next: &sequence{block: 0x00000000, count: 1, next: &sequence{block: 0xffffffff, count: 14}}}, 4, 0, 0, 32, 512},
{&sequence{block: 0xfffeffff, count: 1, next: &sequence{block: 0xffffffff, count: 15}}, 1, 7, 0, 16, 512},
{&sequence{block: 0xfffeffff, count: 15, next: &sequence{block: 0xffffffff, count: 1}}, 5, 7, 0, 16, 512},
{&sequence{block: 0xfffeffff, count: 15, next: &sequence{block: 0xffffffff, count: 1}}, 9, 7, 0, 48, 512},
{&sequence{block: 0xffffffff, count: 2, next: &sequence{block: 0xffffffef, count: 14}}, 19, 3, 0, 124, 512},
{&sequence{block: 0xfffeffff, count: 15, next: &sequence{block: 0x0fffffff, count: 1}}, 60, 0, 0, 480, 512},
{&sequence{block: 0xffffffff, count: 1, next: &sequence{block: 0xfffeffff, count: 14, next: &sequence{block: 0xffffffff, count: 1}}}, 17, 7, 0, 124, 512},
{&sequence{block: 0xfffffffb, count: 1, next: &sequence{block: 0xffffffff, count: 14, next: &sequence{block: 0xffffffff, count: 1}}}, 3, 5, 0, 124, 512},
{&sequence{block: 0xfffffffb, count: 1, next: &sequence{block: 0xfffeffff, count: 14, next: &sequence{block: 0xffffffff, count: 1}}}, 13, 7, 0, 80, 512},
}

for n, i := range input {
bytePos, bitPos, _ := getAvailableFromCurrent(i.mask, i.start, i.curr, i.end)
if bytePos != i.bytePos || bitPos != i.bitPos {
t.Fatalf("Error in (%d) getFirstAvailable(). Expected (%d, %d). Got (%d, %d)", n, i.bytePos, i.bitPos, bytePos, bitPos)
}
}
}
8 changes: 4 additions & 4 deletions ipam/allocator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,15 +402,15 @@ func (a *Allocator) getPredefinedPool(as string, ipV6 bool) (*net.IPNet, error)
continue
}
aSpace.Lock()
_, ok := aSpace.subnets[SubnetKey{AddressSpace: as, Subnet: nw.String()}]
aSpace.Unlock()
if ok {
if _, ok := aSpace.subnets[SubnetKey{AddressSpace: as, Subnet: nw.String()}]; ok {
aSpace.Unlock()
continue
}

if !aSpace.contains(as, nw) {
aSpace.Unlock()
return nw, nil
}
aSpace.Unlock()
}

return nil, types.NotFoundErrorf("could not find an available, non-overlapping IPv%d address pool among the defaults to assign to the network", v)
Expand Down
Loading