From 39d6072cc176a0152240aca87d7798f61c44e74d Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 5 Aug 2016 16:08:35 -0700 Subject: [PATCH 1/4] Set difference when picking random ports --- nomad/structs/bitmap.go | 45 +++++++++++++++++++++++++++++-- nomad/structs/bitmap_test.go | 40 ++++++++++++++++++++++++++-- nomad/structs/network.go | 51 ++++++++++++++++++++++-------------- 3 files changed, 113 insertions(+), 23 deletions(-) diff --git a/nomad/structs/bitmap.go b/nomad/structs/bitmap.go index 22a4aedb82a5..1d16075d9add 100644 --- a/nomad/structs/bitmap.go +++ b/nomad/structs/bitmap.go @@ -6,8 +6,8 @@ import "fmt" type Bitmap []byte // NewBitmap returns a bitmap with up to size indexes -func NewBitmap(size int) (Bitmap, error) { - if size <= 0 { +func NewBitmap(size uint) (Bitmap, error) { + if size == 0 { return nil, fmt.Errorf("bitmap must be positive size") } if size&7 != 0 { @@ -17,6 +17,32 @@ func NewBitmap(size int) (Bitmap, error) { return Bitmap(b), nil } +// Copy returns a copy of the Bitmap +func (b Bitmap) Copy() (Bitmap, error) { + if b == nil { + return nil, fmt.Errorf("can't copy nil Bitmap") + } + + nb, err := NewBitmap(b.Size()) + if err != nil { + return nil, err + } + + s := b.Size() + for i := uint(0); i < s; i++ { + if b.Check(i) { + nb.Set(i) + } + } + + return nb, nil +} + +// Size returns the size of the bitmap +func (b Bitmap) Size() uint { + return uint(len(b) << 3) +} + // Set is used to set the given index of the bitmap func (b Bitmap) Set(idx uint) { bucket := idx >> 3 @@ -37,3 +63,18 @@ func (b Bitmap) Clear() { b[i] = 0 } } + +// IndexesFrom returns the indexes in which the values are either set or unset based +// on the passed parameter starting from the passed index +func (b Bitmap) IndexesFrom(set bool, from uint) []uint { + var indexes []uint + s := b.Size() + for i := from; i < s; i++ { + c := b.Check(i) + if c && set || !c && !set { + indexes = append(indexes, i) + } + } + + return indexes +} diff --git a/nomad/structs/bitmap_test.go b/nomad/structs/bitmap_test.go index 076d6c6d4711..cf5e97405959 100644 --- a/nomad/structs/bitmap_test.go +++ b/nomad/structs/bitmap_test.go @@ -1,6 +1,9 @@ package structs -import "testing" +import ( + "reflect" + "testing" +) func TestBitmap(t *testing.T) { // Check invalid sizes @@ -14,11 +17,16 @@ func TestBitmap(t *testing.T) { } // Create a normal bitmap - b, err := NewBitmap(256) + var s uint = 256 + b, err := NewBitmap(s) if err != nil { t.Fatalf("err: %v", err) } + if b.Size() != s { + t.Fatalf("bad size") + } + // Set a few bits b.Set(0) b.Set(255) @@ -46,6 +54,34 @@ func TestBitmap(t *testing.T) { } } + // Check the indexes + idxs := b.IndexesFrom(true, 0) + expected := []uint{0, 255} + if !reflect.DeepEqual(idxs, expected) { + t.Fatalf("bad: got %#v; want %#v", idxs, expected) + } + + idxs = b.IndexesFrom(true, 1) + expected = []uint{255} + if !reflect.DeepEqual(idxs, expected) { + t.Fatalf("bad: got %#v; want %#v", idxs, expected) + } + + idxs = b.IndexesFrom(false, 0) + if len(idxs) != 254 { + t.Fatalf("bad") + } + + // Check the copy is correct + b2, err := b.Copy() + if err != nil { + t.Fatalf("bad: %v", err) + } + + if !reflect.DeepEqual(b, b2) { + t.Fatalf("bad") + } + // Clear b.Clear() diff --git a/nomad/structs/network.go b/nomad/structs/network.go index 7f4435121436..d2d4217813e7 100644 --- a/nomad/structs/network.go +++ b/nomad/structs/network.go @@ -183,6 +183,8 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour return } + used := idx.UsedPorts[ipStr] + // Check if any of the reserved ports are in use for _, port := range ask.ReservedPorts { // Guard against invalid port @@ -192,7 +194,6 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour } // Check if in use - used := idx.UsedPorts[ipStr] if used != nil && used.Check(uint(port.Value)) { err = fmt.Errorf("reserved port collision") return @@ -208,28 +209,40 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour DynamicPorts: ask.DynamicPorts, } - // Check if we need to generate any ports - for i := 0; i < len(ask.DynamicPorts); i++ { - attempts := 0 - PICK: - attempts++ - if attempts > maxRandPortAttempts { - err = fmt.Errorf("dynamic port selection failed") + // Create a copy of the used ports and apply the new reserves + var usedSet Bitmap + if used != nil { + var lErr error + usedSet, lErr = used.Copy() + if lErr != nil { + err = lErr return } - - randPort := MinDynamicPort + rand.Intn(MaxDynamicPort-MinDynamicPort) - used := idx.UsedPorts[ipStr] - if used != nil && used.Check(uint(randPort)) { - goto PICK + } else { + var lErr error + usedSet, lErr = NewBitmap(maxValidPort) + if lErr != nil { + err = lErr + return } + } - for _, ports := range [][]Port{offer.ReservedPorts, offer.DynamicPorts} { - if isPortReserved(ports, randPort) { - goto PICK - } - } - offer.DynamicPorts[i].Value = randPort + for _, port := range ask.ReservedPorts { + usedSet.Set(uint(port.Value)) + } + + // Get the indexes of the unset + availablePorts := usedSet.IndexesFrom(false, MinDynamicPort) + + // Randomize the amount we need + numDyn := len(offer.DynamicPorts) + for i := 0; i < numDyn; i++ { + j := rand.Intn(numDyn) + availablePorts[i], availablePorts[j] = availablePorts[j], availablePorts[i] + } + + for i := 0; i < numDyn; i++ { + offer.DynamicPorts[i].Value = int(availablePorts[i]) } // Stop, we have an offer! From c0ddba326cdf74829d389b971638359e012a75a3 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Fri, 5 Aug 2016 16:23:41 -0700 Subject: [PATCH 2/4] Add a test --- nomad/structs/network_test.go | 54 +++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/nomad/structs/network_test.go b/nomad/structs/network_test.go index 55d59d9abfb5..3fb0117c0353 100644 --- a/nomad/structs/network_test.go +++ b/nomad/structs/network_test.go @@ -343,6 +343,60 @@ func TestNetworkIndex_AssignNetwork(t *testing.T) { } } +// This test ensures that even with a small domain of available ports we are +// able to make a dynamic port allocation. +func TestNetworkIndex_AssignNetwork_Dynamic_Contention(t *testing.T) { + + // Create a node that only has one free port + idx := NewNetworkIndex() + n := &Node{ + Resources: &Resources{ + Networks: []*NetworkResource{ + &NetworkResource{ + Device: "eth0", + CIDR: "192.168.0.100/32", + MBits: 1000, + }, + }, + }, + Reserved: &Resources{ + Networks: []*NetworkResource{ + &NetworkResource{ + Device: "eth0", + IP: "192.168.0.100", + MBits: 1, + }, + }, + }, + } + for i := MinDynamicPort; i < MaxDynamicPort-1; i++ { + n.Reserved.Networks[0].ReservedPorts = append(n.Reserved.Networks[0].ReservedPorts, Port{Value: i}) + } + + idx.SetNode(n) + + // Ask for dynamic ports + ask := &NetworkResource{ + DynamicPorts: []Port{{"http", 0}}, + } + offer, err := idx.AssignNetwork(ask) + if err != nil { + t.Fatalf("err: %v", err) + } + if offer == nil { + t.Fatalf("bad") + } + if offer.IP != "192.168.0.100" { + t.Fatalf("bad: %#v", offer) + } + if len(offer.DynamicPorts) != 1 { + t.Fatalf("There should be three dynamic ports") + } + if p := offer.DynamicPorts[0].Value; p == MaxDynamicPort { + t.Fatalf("Dynamic Port: should have been assigned %d; got %d", p, MaxDynamicPort) + } +} + func TestIntContains(t *testing.T) { l := []Port{{"one", 1}, {"two", 2}, {"ten", 10}, {"twenty", 20}} if isPortReserved(l, 50) { From 814ed084ce2d1f5d0a7773c117ec11504def7b22 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Wed, 10 Aug 2016 11:47:20 -0700 Subject: [PATCH 3/4] Try stochastic and fallback to precise --- nomad/structs/bitmap.go | 6 +- nomad/structs/bitmap_test.go | 4 +- nomad/structs/network.go | 125 +++++++++++++++++++++++++--------- nomad/structs/network_test.go | 2 +- 4 files changed, 99 insertions(+), 38 deletions(-) diff --git a/nomad/structs/bitmap.go b/nomad/structs/bitmap.go index 1d16075d9add..a9dae2d7af25 100644 --- a/nomad/structs/bitmap.go +++ b/nomad/structs/bitmap.go @@ -66,13 +66,13 @@ func (b Bitmap) Clear() { // IndexesFrom returns the indexes in which the values are either set or unset based // on the passed parameter starting from the passed index -func (b Bitmap) IndexesFrom(set bool, from uint) []uint { - var indexes []uint +func (b Bitmap) IndexesFrom(set bool, from uint) []int { + var indexes []int s := b.Size() for i := from; i < s; i++ { c := b.Check(i) if c && set || !c && !set { - indexes = append(indexes, i) + indexes = append(indexes, int(i)) } } diff --git a/nomad/structs/bitmap_test.go b/nomad/structs/bitmap_test.go index cf5e97405959..86327abbeb12 100644 --- a/nomad/structs/bitmap_test.go +++ b/nomad/structs/bitmap_test.go @@ -56,13 +56,13 @@ func TestBitmap(t *testing.T) { // Check the indexes idxs := b.IndexesFrom(true, 0) - expected := []uint{0, 255} + expected := []int{0, 255} if !reflect.DeepEqual(idxs, expected) { t.Fatalf("bad: got %#v; want %#v", idxs, expected) } idxs = b.IndexesFrom(true, 1) - expected = []uint{255} + expected = []int{255} if !reflect.DeepEqual(idxs, expected) { t.Fatalf("bad: got %#v; want %#v", idxs, expected) } diff --git a/nomad/structs/network.go b/nomad/structs/network.go index d2d4217813e7..a524e040816e 100644 --- a/nomad/structs/network.go +++ b/nomad/structs/network.go @@ -209,40 +209,25 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour DynamicPorts: ask.DynamicPorts, } - // Create a copy of the used ports and apply the new reserves - var usedSet Bitmap - if used != nil { - var lErr error - usedSet, lErr = used.Copy() - if lErr != nil { - err = lErr - return - } - } else { - var lErr error - usedSet, lErr = NewBitmap(maxValidPort) - if lErr != nil { - err = lErr - return - } - } - - for _, port := range ask.ReservedPorts { - usedSet.Set(uint(port.Value)) + // Try to stochastically pick the dynamic ports as it is faster and + // lower memory usage. + var dynPorts []int + var dynErr error + dynPorts, dynErr = getDynamicPortsStochastic(used, ask) + if err != nil { + goto BUILD_OFFER } - // Get the indexes of the unset - availablePorts := usedSet.IndexesFrom(false, MinDynamicPort) - - // Randomize the amount we need - numDyn := len(offer.DynamicPorts) - for i := 0; i < numDyn; i++ { - j := rand.Intn(numDyn) - availablePorts[i], availablePorts[j] = availablePorts[j], availablePorts[i] + // Fall back to the precise method if the random sampling failed. + dynPorts, dynErr = getDynamicPortsPrecise(used, ask) + if err != nil { + err = dynErr + return } - for i := 0; i < numDyn; i++ { - offer.DynamicPorts[i].Value = int(availablePorts[i]) + BUILD_OFFER: + for i, port := range dynPorts { + offer.DynamicPorts[i].Value = port } // Stop, we have an offer! @@ -253,10 +238,86 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour return } +// getDynamicPortsPrecise takes the nodes used port bitmap which may be nil if +// no ports have been allocated yet, the network ask and returns a set of unused +// ports to fullfil the ask's DynamicPorts or an error if it failed. An error +// means the ask can not be satisfied as the method does a precise search. +func getDynamicPortsPrecise(nodeUsed Bitmap, ask *NetworkResource) ([]int, error) { + // Create a copy of the used ports and apply the new reserves + var usedSet Bitmap + var err error + if nodeUsed != nil { + usedSet, err = nodeUsed.Copy() + if err != nil { + return nil, err + } + } else { + usedSet, err = NewBitmap(maxValidPort) + if err != nil { + return nil, err + } + } + + for _, port := range ask.ReservedPorts { + usedSet.Set(uint(port.Value)) + } + + // Get the indexes of the unset + availablePorts := usedSet.IndexesFrom(false, MinDynamicPort) + + // Randomize the amount we need + numDyn := len(ask.DynamicPorts) + if len(availablePorts) < numDyn { + return nil, fmt.Errorf("dynamic port selection failed") + } + + for i := 0; i < numDyn; i++ { + j := rand.Intn(numDyn) + availablePorts[i], availablePorts[j] = availablePorts[j], availablePorts[i] + } + + return availablePorts[:numDyn], nil +} + +// getDynamicPortsStochastic takes the nodes used port bitmap which may be nil if +// no ports have been allocated yet, the network ask and returns a set of unused +// ports to fullfil the ask's DynamicPorts or an error if it failed. An error +// does not mean the ask can not be satisfied as the method has a fixed amount +// of random probes and if these fail, the search is aborted. +func getDynamicPortsStochastic(nodeUsed Bitmap, ask *NetworkResource) ([]int, error) { + var reserved, dynamic []int + for _, port := range ask.ReservedPorts { + reserved = append(reserved, port.Value) + } + + for i := 0; i < len(ask.DynamicPorts); i++ { + attempts := 0 + PICK: + attempts++ + if attempts > maxRandPortAttempts { + return nil, fmt.Errorf("stochastic dynamic port selection failed") + } + + randPort := MinDynamicPort + rand.Intn(MaxDynamicPort-MinDynamicPort) + if nodeUsed != nil && nodeUsed.Check(uint(randPort)) { + goto PICK + } + + for _, ports := range [][]int{reserved, dynamic} { + if isPortReserved(ports, randPort) { + goto PICK + } + } + dynamic = append(dynamic, randPort) + } + + return dynamic, nil +} + // IntContains scans an integer slice for a value -func isPortReserved(haystack []Port, needle int) bool { +func isPortReserved(haystack []int, needle int) bool { for _, item := range haystack { - if item.Value == needle { + if item == needle { return true } } diff --git a/nomad/structs/network_test.go b/nomad/structs/network_test.go index 3fb0117c0353..bb897fb349ef 100644 --- a/nomad/structs/network_test.go +++ b/nomad/structs/network_test.go @@ -398,7 +398,7 @@ func TestNetworkIndex_AssignNetwork_Dynamic_Contention(t *testing.T) { } func TestIntContains(t *testing.T) { - l := []Port{{"one", 1}, {"two", 2}, {"ten", 10}, {"twenty", 20}} + l := []int{1, 2, 10, 20} if isPortReserved(l, 50) { t.Fatalf("bad") } From 5062224624a0a1a4ec33258ca7fbbcd0e1bc6218 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Wed, 10 Aug 2016 16:37:26 -0700 Subject: [PATCH 4/4] Fixes plus address feedback --- nomad/structs/bitmap.go | 25 +++++++------------------ nomad/structs/bitmap_test.go | 11 ++++++++--- nomad/structs/network.go | 6 +++--- 3 files changed, 18 insertions(+), 24 deletions(-) diff --git a/nomad/structs/bitmap.go b/nomad/structs/bitmap.go index a9dae2d7af25..d16665819d95 100644 --- a/nomad/structs/bitmap.go +++ b/nomad/structs/bitmap.go @@ -23,19 +23,9 @@ func (b Bitmap) Copy() (Bitmap, error) { return nil, fmt.Errorf("can't copy nil Bitmap") } - nb, err := NewBitmap(b.Size()) - if err != nil { - return nil, err - } - - s := b.Size() - for i := uint(0); i < s; i++ { - if b.Check(i) { - nb.Set(i) - } - } - - return nb, nil + raw := make([]byte, len(b)) + copy(raw, b) + return Bitmap(raw), nil } // Size returns the size of the bitmap @@ -64,12 +54,11 @@ func (b Bitmap) Clear() { } } -// IndexesFrom returns the indexes in which the values are either set or unset based -// on the passed parameter starting from the passed index -func (b Bitmap) IndexesFrom(set bool, from uint) []int { +// IndexesInRange returns the indexes in which the values are either set or unset based +// on the passed parameter in the passed range +func (b Bitmap) IndexesInRange(set bool, from, to uint) []int { var indexes []int - s := b.Size() - for i := from; i < s; i++ { + for i := from; i < to; i++ { c := b.Check(i) if c && set || !c && !set { indexes = append(indexes, int(i)) diff --git a/nomad/structs/bitmap_test.go b/nomad/structs/bitmap_test.go index 86327abbeb12..86bdd64da426 100644 --- a/nomad/structs/bitmap_test.go +++ b/nomad/structs/bitmap_test.go @@ -55,23 +55,28 @@ func TestBitmap(t *testing.T) { } // Check the indexes - idxs := b.IndexesFrom(true, 0) + idxs := b.IndexesInRange(true, 0, 256) expected := []int{0, 255} if !reflect.DeepEqual(idxs, expected) { t.Fatalf("bad: got %#v; want %#v", idxs, expected) } - idxs = b.IndexesFrom(true, 1) + idxs = b.IndexesInRange(true, 1, 256) expected = []int{255} if !reflect.DeepEqual(idxs, expected) { t.Fatalf("bad: got %#v; want %#v", idxs, expected) } - idxs = b.IndexesFrom(false, 0) + idxs = b.IndexesInRange(false, 0, 256) if len(idxs) != 254 { t.Fatalf("bad") } + idxs = b.IndexesInRange(false, 100, 200) + if len(idxs) != 100 { + t.Fatalf("bad") + } + // Check the copy is correct b2, err := b.Copy() if err != nil { diff --git a/nomad/structs/network.go b/nomad/structs/network.go index a524e040816e..126d83b43aa2 100644 --- a/nomad/structs/network.go +++ b/nomad/structs/network.go @@ -214,13 +214,13 @@ func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResour var dynPorts []int var dynErr error dynPorts, dynErr = getDynamicPortsStochastic(used, ask) - if err != nil { + if err == nil { goto BUILD_OFFER } // Fall back to the precise method if the random sampling failed. dynPorts, dynErr = getDynamicPortsPrecise(used, ask) - if err != nil { + if dynErr != nil { err = dynErr return } @@ -263,7 +263,7 @@ func getDynamicPortsPrecise(nodeUsed Bitmap, ask *NetworkResource) ([]int, error } // Get the indexes of the unset - availablePorts := usedSet.IndexesFrom(false, MinDynamicPort) + availablePorts := usedSet.IndexesInRange(false, MinDynamicPort, MaxDynamicPort) // Randomize the amount we need numDyn := len(ask.DynamicPorts)