Merge pull request #5783 from LK4D4/fix_duplicate_ip_allocation_#5729

Fix duplicate ip allocation

daemon/networkdriver/ipallocator/allocator.go

@@ -7,9 +7,19 @@ import (
 	"github.com/dotcloud/docker/pkg/collections"
 	"net"
 	"sync"
+	"sync/atomic"
 )
 
-type networkSet map[string]*collections.OrderedIntSet
+type allocatedMap struct {
+	*collections.OrderedIntSet
+	last int32
+}
+
+func newAllocatedMap() *allocatedMap {
+	return &allocatedMap{OrderedIntSet: collections.NewOrderedIntSet()}
+}
+
+type networkSet map[string]*allocatedMap
 
 var (
 	ErrNoAvailableIPs     = errors.New("no available ip addresses on network")
@@ -19,7 +29,6 @@ var (
 var (
 	lock         = sync.Mutex{}
 	allocatedIPs = networkSet{}
-	availableIPS = networkSet{}
 )
 
 // RequestIP requests an available ip from the given network.  It
@@ -55,13 +64,11 @@ func ReleaseIP(address *net.IPNet, ip *net.IP) error {
 	checkAddress(address)
 
 	var (
-		existing  = allocatedIPs[address.String()]
-		available = availableIPS[address.String()]
+		allocated = allocatedIPs[address.String()]
 		pos       = getPosition(address, ip)
 	)
 
-	existing.Remove(int(pos))
-	available.Push(int(pos))
+	allocated.Remove(int(pos))
 
 	return nil
 }
@@ -82,29 +89,19 @@ func getPosition(address *net.IPNet, ip *net.IP) int32 {
 func getNextIp(address *net.IPNet) (*net.IP, error) {
 	var (
 		ownIP     = ipToInt(&address.IP)
-		available = availableIPS[address.String()]
 		allocated = allocatedIPs[address.String()]
 		first, _  = networkdriver.NetworkRange(address)
 		base      = ipToInt(&first)
 		size      = int(networkdriver.NetworkSize(address.Mask))
 		max       = int32(size - 2) // size -1 for the broadcast address, -1 for the gateway address
-		pos       = int32(available.Pop())
+		pos       = atomic.LoadInt32(&allocated.last)
 	)
 
-	// We pop and push the position not the ip
-	if pos != 0 {
-		ip := intToIP(int32(base + pos))
-		allocated.Push(int(pos))
-
-		return ip, nil
-	}
-
 	var (
 		firstNetIP = address.IP.To4().Mask(address.Mask)
 		firstAsInt = ipToInt(&firstNetIP) + 1
 	)
 
-	pos = int32(allocated.PullBack())
 	for i := int32(0); i < max; i++ {
 		pos = pos%max + 1
 		next := int32(base + pos)
@@ -116,6 +113,7 @@ func getNextIp(address *net.IPNet) (*net.IP, error) {
 		if !allocated.Exists(int(pos)) {
 			ip := intToIP(next)
 			allocated.Push(int(pos))
+			atomic.StoreInt32(&allocated.last, pos)
 			return ip, nil
 		}
 	}
@@ -124,15 +122,14 @@ func getNextIp(address *net.IPNet) (*net.IP, error) {
 
 func registerIP(address *net.IPNet, ip *net.IP) error {
 	var (
-		existing  = allocatedIPs[address.String()]
-		available = availableIPS[address.String()]
+		allocated = allocatedIPs[address.String()]
 		pos       = getPosition(address, ip)
 	)
 
-	if existing.Exists(int(pos)) {
+	if allocated.Exists(int(pos)) {
 		return ErrIPAlreadyAllocated
 	}
-	available.Remove(int(pos))
+	atomic.StoreInt32(&allocated.last, pos)
 
 	return nil
 }
@@ -153,7 +150,6 @@ func intToIP(n int32) *net.IP {
 func checkAddress(address *net.IPNet) {
 	key := address.String()
 	if _, exists := allocatedIPs[key]; !exists {
-		allocatedIPs[key] = collections.NewOrderedIntSet()
-		availableIPS[key] = collections.NewOrderedIntSet()
+		allocatedIPs[key] = newAllocatedMap()
 	}
 }

daemon/networkdriver/ipallocator/allocator_test.go

@@ -8,7 +8,6 @@ import (
 
 func reset() {
 	allocatedIPs = networkSet{}
-	availableIPS = networkSet{}
 }
 
 func TestRequestNewIps(t *testing.T) {
@@ -18,8 +17,10 @@ func TestRequestNewIps(t *testing.T) {
 		Mask: []byte{255, 255, 255, 0},
 	}
 
+	var ip *net.IP
+	var err error
 	for i := 2; i < 10; i++ {
-		ip, err := RequestIP(network, nil)
+		ip, err = RequestIP(network, nil)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -28,6 +29,17 @@ func TestRequestNewIps(t *testing.T) {
 			t.Fatalf("Expected ip %s got %s", expected, ip.String())
 		}
 	}
+	value := intToIP(ipToInt(ip) + 1).String()
+	if err := ReleaseIP(network, ip); err != nil {
+		t.Fatal(err)
+	}
+	ip, err = RequestIP(network, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ip.String() != value {
+		t.Fatalf("Expected to receive the next ip %s got %s", value, ip.String())
+	}
 }
 
 func TestReleaseIp(t *testing.T) {
@@ -64,6 +76,17 @@ func TestGetReleasedIp(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	for i := 0; i < 252; i++ {
+		_, err = RequestIP(network, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		err = ReleaseIP(network, ip)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
 	ip, err = RequestIP(network, nil)
 	if err != nil {
 		t.Fatal(err)
@@ -185,24 +208,6 @@ func TestIPAllocator(t *testing.T) {
 
 		newIPs[i] = ip
 	}
-	// Before loop begin
-	// 2(u) - 3(u) - 4(f) - 5(f) - 6(f)
-	//                       ↑
-
-	// After i = 0
-	// 2(u) - 3(u) - 4(f) - 5(u) - 6(f)
-	//                              ↑
-
-	// After i = 1
-	// 2(u) - 3(u) - 4(f) - 5(u) - 6(u)
-	//                ↑
-
-	// After i = 2
-	// 2(u) - 3(u) - 4(u) - 5(u) - 6(u)
-	//                       ↑
-
-	// Reordered these because the new set will always return the
-	// lowest ips first and not in the order that they were released
 	assertIPEquals(t, &expectedIPs[2], newIPs[0])
 	assertIPEquals(t, &expectedIPs[3], newIPs[1])
 	assertIPEquals(t, &expectedIPs[4], newIPs[2])
@@ -234,6 +239,86 @@ func TestAllocateFirstIP(t *testing.T) {
 	}
 }
 
+func TestAllocateAllIps(t *testing.T) {
+	defer reset()
+	network := &net.IPNet{
+		IP:   []byte{192, 168, 0, 1},
+		Mask: []byte{255, 255, 255, 0},
+	}
+
+	var (
+		current, first *net.IP
+		err            error
+		isFirst        = true
+	)
+
+	for err == nil {
+		current, err = RequestIP(network, nil)
+		if isFirst {
+			first = current
+			isFirst = false
+		}
+	}
+
+	if err != ErrNoAvailableIPs {
+		t.Fatal(err)
+	}
+
+	if _, err := RequestIP(network, nil); err != ErrNoAvailableIPs {
+		t.Fatal(err)
+	}
+
+	if err := ReleaseIP(network, first); err != nil {
+		t.Fatal(err)
+	}
+
+	again, err := RequestIP(network, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	assertIPEquals(t, first, again)
+}
+
+func TestAllocateDifferentSubnets(t *testing.T) {
+	defer reset()
+	network1 := &net.IPNet{
+		IP:   []byte{192, 168, 0, 1},
+		Mask: []byte{255, 255, 255, 0},
+	}
+	network2 := &net.IPNet{
+		IP:   []byte{127, 0, 0, 1},
+		Mask: []byte{255, 255, 255, 0},
+	}
+	expectedIPs := []net.IP{
+		0: net.IPv4(192, 168, 0, 2),
+		1: net.IPv4(192, 168, 0, 3),
+		2: net.IPv4(127, 0, 0, 2),
+		3: net.IPv4(127, 0, 0, 3),
+	}
+
+	ip11, err := RequestIP(network1, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ip12, err := RequestIP(network1, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ip21, err := RequestIP(network2, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ip22, err := RequestIP(network2, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	assertIPEquals(t, &expectedIPs[0], ip11)
+	assertIPEquals(t, &expectedIPs[1], ip12)
+	assertIPEquals(t, &expectedIPs[2], ip21)
+	assertIPEquals(t, &expectedIPs[3], ip22)
+}
+
 func assertIPEquals(t *testing.T, ip1, ip2 *net.IP) {
 	if !ip1.Equal(*ip2) {
 		t.Fatalf("Expected IP %s, got %s", ip1, ip2)

pkg/collections/orderedintset.go

@@ -1,12 +1,13 @@
 package collections
 
 import (
+	"sort"
 	"sync"
 )
 
 // OrderedIntSet is a thread-safe sorted set and a stack.
 type OrderedIntSet struct {
-	sync.RWMutex
+	sync.Mutex
 	set []int
 }
 
@@ -15,29 +16,22 @@ func NewOrderedIntSet() *OrderedIntSet {
 	return &OrderedIntSet{}
 }
 
-// Push takes a string and adds it to the set. If the elem aready exists, it has no effect.
+// Push takes an int and adds it to the set. If the elem aready exists, it has no effect.
 func (s *OrderedIntSet) Push(elem int) {
-	s.RLock()
-	for _, e := range s.set {
-		if e == elem {
-			s.RUnlock()
-			return
-		}
-	}
-	s.RUnlock()
-
 	s.Lock()
+	if len(s.set) == 0 {
+		s.set = append(s.set, elem)
+		s.Unlock()
+		return
+	}
 
 	// Make sure the list is always sorted
-	for i, e := range s.set {
-		if elem < e {
-			s.set = append(s.set[:i], append([]int{elem}, s.set[i:]...)...)
-			s.Unlock()
-			return
-		}
+	i := sort.SearchInts(s.set, elem)
+	if i < len(s.set) && s.set[i] == elem {
+		s.Unlock()
+		return
 	}
-	// If we reach here, then elem is the biggest elem of the list.
-	s.set = append(s.set, elem)
+	s.set = append(s.set[:i], append([]int{elem}, s.set[i:]...)...)
 	s.Unlock()
 }
 
@@ -46,28 +40,26 @@ func (s *OrderedIntSet) Pop() int {
 	return s.PopFront()
 }
 
-// Pop returns the first elemen from the list and removes it.
+// Pop returns the first element from the list and removes it.
 // If the list is empty, it returns 0
 func (s *OrderedIntSet) PopFront() int {
-	s.RLock()
-
-	for i, e := range s.set {
-		ret := e
-		s.RUnlock()
-		s.Lock()
-		s.set = append(s.set[:i], s.set[i+1:]...)
+	s.Lock()
+	if len(s.set) == 0 {
 		s.Unlock()
-		return ret
+		return 0
 	}
-	s.RUnlock()
-
-	return 0
+	ret := s.set[0]
+	s.set = s.set[1:]
+	s.Unlock()
+	return ret
 }
 
 // PullBack retrieve the last element of the list.
 // The element is not removed.
 // If the list is empty, an empty element is returned.
 func (s *OrderedIntSet) PullBack() int {
+	s.Lock()
+	defer s.Unlock()
 	if len(s.set) == 0 {
 		return 0
 	}
@@ -76,21 +68,28 @@ func (s *OrderedIntSet) PullBack() int {
 
 // Exists checks if the given element present in the list.
 func (s *OrderedIntSet) Exists(elem int) bool {
-	for _, e := range s.set {
-		if e == elem {
-			return true
-		}
+	s.Lock()
+	if len(s.set) == 0 {
+		s.Unlock()
+		return false
 	}
-	return false
+	i := sort.SearchInts(s.set, elem)
+	res := i < len(s.set) && s.set[i] == elem
+	s.Unlock()
+	return res
 }
 
 // Remove removes an element from the list.
 // If the element is not found, it has no effect.
 func (s *OrderedIntSet) Remove(elem int) {
-	for i, e := range s.set {
-		if e == elem {
-			s.set = append(s.set[:i], s.set[i+1:]...)
-			return
-		}
+	s.Lock()
+	if len(s.set) == 0 {
+		s.Unlock()
+		return
 	}
+	i := sort.SearchInts(s.set, elem)
+	if i < len(s.set) && s.set[i] == elem {
+		s.set = append(s.set[:i], s.set[i+1:]...)
+	}
+	s.Unlock()
 }

pkg/collections/orderedintset_test.go

@@ -0,0 +1,71 @@
+package collections
+
+import (
+	"math/rand"
+	"testing"
+)
+
+func BenchmarkPush(b *testing.B) {
+	var testSet []int
+	for i := 0; i < 1000; i++ {
+		testSet = append(testSet, rand.Int())
+	}
+	s := NewOrderedIntSet()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for _, elem := range testSet {
+			s.Push(elem)
+		}
+	}
+}
+
+func BenchmarkPop(b *testing.B) {
+	var testSet []int
+	for i := 0; i < 1000; i++ {
+		testSet = append(testSet, rand.Int())
+	}
+	s := NewOrderedIntSet()
+	for _, elem := range testSet {
+		s.Push(elem)
+	}
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for j := 0; j < 1000; j++ {
+			s.Pop()
+		}
+	}
+}
+
+func BenchmarkExist(b *testing.B) {
+	var testSet []int
+	for i := 0; i < 1000; i++ {
+		testSet = append(testSet, rand.Intn(2000))
+	}
+	s := NewOrderedIntSet()
+	for _, elem := range testSet {
+		s.Push(elem)
+	}
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for j := 0; j < 1000; j++ {
+			s.Exists(j)
+		}
+	}
+}
+
+func BenchmarkRemove(b *testing.B) {
+	var testSet []int
+	for i := 0; i < 1000; i++ {
+		testSet = append(testSet, rand.Intn(2000))
+	}
+	s := NewOrderedIntSet()
+	for _, elem := range testSet {
+		s.Push(elem)
+	}
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for j := 0; j < 1000; j++ {
+			s.Remove(j)
+		}
+	}
+}