Merge pull request #2132 from cziebuhr/2093-iface_order2

Improve interface order

libnetwork/controller.go

@@ -44,7 +44,6 @@ create network namespaces and allocate interfaces for containers to use.
 package libnetwork
 
 import (
-	"container/heap"
 	"fmt"
 	"net"
 	"path/filepath"
@@ -1085,7 +1084,7 @@ func (c *controller) NewSandbox(containerID string, options ...SandboxOption) (S
 		sb = &sandbox{
 			id:                 sandboxID,
 			containerID:        containerID,
-			endpoints:          epHeap{},
+			endpoints:          []*endpoint{},
 			epPriority:         map[string]int{},
 			populatedEndpoints: map[string]struct{}{},
 			config:             containerConfig{},
@@ -1094,8 +1093,6 @@ func (c *controller) NewSandbox(containerID string, options ...SandboxOption) (S
 		}
 	}
 
-	heap.Init(&sb.endpoints)
-
 	sb.processOptions(options...)
 
 	c.Lock()

libnetwork/endpoint.go

@@ -1,7 +1,6 @@
 package libnetwork
 
 import (
-	"container/heap"
 	"encoding/json"
 	"fmt"
 	"net"
@@ -514,9 +513,7 @@ func (ep *endpoint) sbJoin(sb *sandbox, options ...EndpointOption) (err error) {
 	// Current endpoint providing external connectivity for the sandbox
 	extEp := sb.getGatewayEndpoint()
 
-	sb.Lock()
+	sb.addEndpoint(ep)
-	heap.Push(&sb.endpoints, ep)
-	sb.Unlock()
 	defer func() {
 		if err != nil {
 			sb.removeEndpoint(ep)

libnetwork/resolver.go

@@ -280,7 +280,7 @@ func (r *resolver) handleIPQuery(name string, query *dns.Msg, ipType int) (*dns.
 }
 
 func (r *resolver) handlePTRQuery(ptr string, query *dns.Msg) (*dns.Msg, error) {
-	parts := []string{}
+	var parts []string
 
 	if strings.HasSuffix(ptr, ptrIPv4domain) {
 		parts = strings.Split(ptr, ptrIPv4domain)

libnetwork/sandbox.go

@@ -1,10 +1,10 @@
 package libnetwork
 
 import (
-	"container/heap"
 	"encoding/json"
 	"fmt"
 	"net"
+	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -63,8 +63,6 @@ func (sb *sandbox) processOptions(options ...SandboxOption) {
 	}
 }
 
-type epHeap []*endpoint
-
 type sandbox struct {
 	id                 string
 	containerID        string
@@ -75,7 +73,7 @@ type sandbox struct {
 	resolver           Resolver
 	resolverOnce       sync.Once
 	refCnt             int
-	endpoints          epHeap
+	endpoints          []*endpoint
 	epPriority         map[string]int
 	populatedEndpoints map[string]struct{}
 	joinLeaveDone      chan struct{}
@@ -353,20 +351,36 @@ func (sb *sandbox) getConnectedEndpoints() []*endpoint {
 	defer sb.Unlock()
 
 	eps := make([]*endpoint, len(sb.endpoints))
-	for i, ep := range sb.endpoints {
+	copy(eps, sb.endpoints)
-		eps[i] = ep
-	}
 
 	return eps
 }
 
+func (sb *sandbox) addEndpoint(ep *endpoint) {
+	sb.Lock()
+	defer sb.Unlock()
+
+	l := len(sb.endpoints)
+	i := sort.Search(l, func(j int) bool {
+		return ep.Less(sb.endpoints[j])
+	})
+
+	sb.endpoints = append(sb.endpoints, nil)
+	copy(sb.endpoints[i+1:], sb.endpoints[i:])
+	sb.endpoints[i] = ep
+}
+
 func (sb *sandbox) removeEndpoint(ep *endpoint) {
 	sb.Lock()
 	defer sb.Unlock()
 
+	sb.removeEndpointRaw(ep)
+}
+
+func (sb *sandbox) removeEndpointRaw(ep *endpoint) {
 	for i, e := range sb.endpoints {
 		if e == ep {
-			heap.Remove(&sb.endpoints, i)
+			sb.endpoints = append(sb.endpoints[:i], sb.endpoints[i+1:]...)
 			return
 		}
 	}
@@ -940,7 +954,7 @@ func (sb *sandbox) clearNetworkResources(origEp *endpoint) error {
 		return nil
 	}
 
-	heap.Remove(&sb.endpoints, index)
+	sb.removeEndpointRaw(ep)
 	for _, e := range sb.endpoints {
 		if len(e.Gateway()) > 0 {
 			gwepAfter = e
@@ -1165,80 +1179,69 @@ func OptionIngress() SandboxOption {
 	}
 }
 
-func (eh epHeap) Len() int { return len(eh) }
+// <=> Returns true if a < b, false if a > b and advances to next level if a == b
-
+// epi.prio <=> epj.prio           # 2 < 1
-func (eh epHeap) Less(i, j int) bool {
+// epi.gw <=> epj.gw               # non-gw < gw
+// epi.internal <=> epj.internal   # non-internal < internal
+// epi.joininfo <=> epj.joininfo   # ipv6 < ipv4
+// epi.name <=> epj.name           # bar < foo
+func (epi *endpoint) Less(epj *endpoint) bool {
 	var (
-		cip, cjp int
+		prioi, prioj int
-		ok       bool
 	)
 
-	ci, _ := eh[i].getSandbox()
+	sbi, _ := epi.getSandbox()
-	cj, _ := eh[j].getSandbox()
+	sbj, _ := epj.getSandbox()
 
-	epi := eh[i]
+	// Prio defaults to 0
-	epj := eh[j]
+	if sbi != nil {
-
+		prioi = sbi.epPriority[epi.ID()]
-	if epi.endpointInGWNetwork() {
+	}
-		return false
+	if sbj != nil {
+		prioj = sbj.epPriority[epj.ID()]
 	}
 
-	if epj.endpointInGWNetwork() {
+	if prioi != prioj {
-		return true
+		return prioi > prioj
 	}
 
-	if epi.getNetwork().Internal() {
+	gwi := epi.endpointInGWNetwork()
-		return false
+	gwj := epj.endpointInGWNetwork()
+	if gwi != gwj {
+		return gwj
 	}
 
-	if epj.getNetwork().Internal() {
+	inti := epi.getNetwork().Internal()
-		return true
+	intj := epj.getNetwork().Internal()
+	if inti != intj {
+		return intj
 	}
 
-	if epi.joinInfo != nil && epj.joinInfo != nil {
+	jii := 0
-		if (epi.joinInfo.gw != nil && epi.joinInfo.gw6 != nil) &&
+	if epi.joinInfo != nil {
-			(epj.joinInfo.gw == nil || epj.joinInfo.gw6 == nil) {
+		if epi.joinInfo.gw != nil {
-			return true
+			jii = jii + 1
 		}
-		if (epj.joinInfo.gw != nil && epj.joinInfo.gw6 != nil) &&
+		if epi.joinInfo.gw6 != nil {
-			(epi.joinInfo.gw == nil || epi.joinInfo.gw6 == nil) {
+			jii = jii + 2
-			return false
 		}
 	}
 
-	if ci != nil {
+	jij := 0
-		cip, ok = ci.epPriority[eh[i].ID()]
+	if epj.joinInfo != nil {
-		if !ok {
+		if epj.joinInfo.gw != nil {
-			cip = 0
+			jij = jij + 1
+		}
+		if epj.joinInfo.gw6 != nil {
+			jij = jij + 2
 		}
 	}
 
-	if cj != nil {
+	if jii != jij {
-		cjp, ok = cj.epPriority[eh[j].ID()]
+		return jii > jij
-		if !ok {
-			cjp = 0
-		}
 	}
 
-	if cip == cjp {
+	return epi.network.Name() < epj.network.Name()
-		return eh[i].network.Name() < eh[j].network.Name()
-	}
-
-	return cip > cjp
-}
-
-func (eh epHeap) Swap(i, j int) { eh[i], eh[j] = eh[j], eh[i] }
-
-func (eh *epHeap) Push(x interface{}) {
-	*eh = append(*eh, x.(*endpoint))
-}
-
-func (eh *epHeap) Pop() interface{} {
-	old := *eh
-	n := len(old)
-	x := old[n-1]
-	*eh = old[0 : n-1]
-	return x
 }
 
 func (sb *sandbox) NdotsSet() bool {

libnetwork/sandbox_store.go

@@ -1,7 +1,6 @@
 package libnetwork
 
 import (
-	"container/heap"
 	"encoding/json"
 	"sync"
 
@@ -215,7 +214,7 @@ func (c *controller) sandboxCleanup(activeSandboxes map[string]interface{}) {
 			id:                 sbs.ID,
 			controller:         sbs.c,
 			containerID:        sbs.Cid,
-			endpoints:          epHeap{},
+			endpoints:          []*endpoint{},
 			populatedEndpoints: map[string]struct{}{},
 			dbIndex:            sbs.dbIndex,
 			isStub:             true,
@@ -242,7 +241,6 @@ func (c *controller) sandboxCleanup(activeSandboxes map[string]interface{}) {
 			sb.processOptions(opts...)
 			sb.restorePath()
 			create = !sb.config.useDefaultSandBox
-			heap.Init(&sb.endpoints)
 		}
 		sb.osSbox, err = osl.NewSandbox(sb.Key(), create, isRestore)
 		if err != nil {
@@ -272,7 +270,7 @@ func (c *controller) sandboxCleanup(activeSandboxes map[string]interface{}) {
 				logrus.Errorf("failed to restore endpoint %s in %s for container %s due to %v", eps.Eid, eps.Nid, sb.ContainerID(), err)
 				continue
 			}
-			heap.Push(&sb.endpoints, ep)
+			sb.addEndpoint(ep)
 		}
 
 		if _, ok := activeSandboxes[sb.ID()]; !ok {

libnetwork/sandbox_test.go

@@ -5,13 +5,14 @@ import (
 	"testing"
 
 	"github.com/docker/libnetwork/config"
+	"github.com/docker/libnetwork/ipamapi"
 	"github.com/docker/libnetwork/netlabel"
 	"github.com/docker/libnetwork/options"
 	"github.com/docker/libnetwork/osl"
 	"github.com/docker/libnetwork/testutils"
 )
 
-func getTestEnv(t *testing.T, numNetworks int) (NetworkController, []Network) {
+func getTestEnv(t *testing.T, opts ...[]NetworkOption) (NetworkController, []Network) {
 	netType := "bridge"
 
 	option := options.Generic{
@@ -29,19 +30,22 @@ func getTestEnv(t *testing.T, numNetworks int) (NetworkController, []Network) {
 		t.Fatal(err)
 	}
 
-	if numNetworks == 0 {
+	if len(opts) == 0 {
 		return c, nil
 	}
 
-	nwList := make([]Network, 0, numNetworks)
+	nwList := make([]Network, 0, len(opts))
-	for i := 0; i < numNetworks; i++ {
+	for i, opt := range opts {
 		name := fmt.Sprintf("test_nw_%d", i)
 		netOption := options.Generic{
 			netlabel.GenericData: options.Generic{
 				"BridgeName": name,
 			},
 		}
-		n, err := c.NewNetwork(netType, name, "", NetworkOptionGeneric(netOption))
+		newOptions := make([]NetworkOption, 1, len(opt)+1)
+		newOptions[0] = NetworkOptionGeneric(netOption)
+		newOptions = append(newOptions, opt...)
+		n, err := c.NewNetwork(netType, name, "", newOptions...)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -53,7 +57,7 @@ func getTestEnv(t *testing.T, numNetworks int) (NetworkController, []Network) {
 }
 
 func TestSandboxAddEmpty(t *testing.T) {
-	c, _ := getTestEnv(t, 0)
+	c, _ := getTestEnv(t)
 	ctrlr := c.(*controller)
 
 	sbx, err := ctrlr.NewSandbox("sandbox0")
@@ -72,12 +76,19 @@ func TestSandboxAddEmpty(t *testing.T) {
 	osl.GC()
 }
 
+// // If different priorities are specified, internal option and ipv6 addresses mustn't influence endpoint order
 func TestSandboxAddMultiPrio(t *testing.T) {
 	if !testutils.IsRunningInContainer() {
 		defer testutils.SetupTestOSContext(t)()
 	}
 
-	c, nws := getTestEnv(t, 3)
+	opts := [][]NetworkOption{
+		{NetworkOptionEnableIPv6(true), NetworkOptionIpam(ipamapi.DefaultIPAM, "", nil, []*IpamConf{{PreferredPool: "fe90::/64"}}, nil)},
+		{NetworkOptionInternalNetwork()},
+		{},
+	}
+
+	c, nws := getTestEnv(t, opts...)
 	ctrlr := c.(*controller)
 
 	sbx, err := ctrlr.NewSandbox("sandbox1")
@@ -158,7 +169,14 @@ func TestSandboxAddSamePrio(t *testing.T) {
 		defer testutils.SetupTestOSContext(t)()
 	}
 
-	c, nws := getTestEnv(t, 2)
+	opts := [][]NetworkOption{
+		{},
+		{},
+		{NetworkOptionEnableIPv6(true), NetworkOptionIpam(ipamapi.DefaultIPAM, "", nil, []*IpamConf{{PreferredPool: "fe90::/64"}}, nil)},
+		{NetworkOptionInternalNetwork()},
+	}
+
+	c, nws := getTestEnv(t, opts...)
 
 	ctrlr := c.(*controller)
 
@@ -168,36 +186,66 @@ func TestSandboxAddSamePrio(t *testing.T) {
 	}
 	sid := sbx.ID()
 
-	ep1, err := nws[0].CreateEndpoint("ep1")
+	epNw1, err := nws[1].CreateEndpoint("ep1")
 	if err != nil {
 		t.Fatal(err)
 	}
-	ep2, err := nws[1].CreateEndpoint("ep2")
+	epIPv6, err := nws[2].CreateEndpoint("ep2")
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	if err := ep1.Join(sbx); err != nil {
+	epInternal, err := nws[3].CreateEndpoint("ep3")
+	if err != nil {
 		t.Fatal(err)
 	}
 
-	if err := ep2.Join(sbx); err != nil {
+	epNw0, err := nws[0].CreateEndpoint("ep4")
+	if err != nil {
 		t.Fatal(err)
 	}
 
-	if ctrlr.sandboxes[sid].endpoints[0].ID() != ep1.ID() {
+	if err := epNw1.Join(sbx); err != nil {
-		t.Fatal("Expected ep1 to be at the top of the heap. But did not find ep1 at the top of the heap")
-	}
-
-	if err := ep1.Leave(sbx); err != nil {
 		t.Fatal(err)
 	}
 
-	if ctrlr.sandboxes[sid].endpoints[0].ID() != ep2.ID() {
+	if err := epIPv6.Join(sbx); err != nil {
-		t.Fatal("Expected ep2 to be at the top of the heap after removing ep3. But did not find ep2 at the top of the heap")
+		t.Fatal(err)
 	}
 
-	if err := ep2.Leave(sbx); err != nil {
+	if err := epInternal.Join(sbx); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := epNw0.Join(sbx); err != nil {
+		t.Fatal(err)
+	}
+
+	// order should now be: epIPv6, epNw0, epNw1, epInternal
+	if len(sbx.Endpoints()) != 4 {
+		t.Fatal("Expected 4 endpoints to be connected to the sandbox.")
+	}
+
+	// IPv6 has precedence over IPv4
+	if ctrlr.sandboxes[sid].endpoints[0].ID() != epIPv6.ID() {
+		t.Fatal("Expected epIPv6 to be at the top of the heap. But did not find epIPv6 at the top of the heap")
+	}
+
+	// internal network has lowest precedence
+	if ctrlr.sandboxes[sid].endpoints[3].ID() != epInternal.ID() {
+		t.Fatal("Expected epInternal to be at the bottom of the heap. But did not find epInternal at the bottom of the heap")
+	}
+
+	if err := epIPv6.Leave(sbx); err != nil {
+		t.Fatal(err)
+	}
+
+	// 'test_nw_0' has precedence over 'test_nw_1'
+	if ctrlr.sandboxes[sid].endpoints[0].ID() != epNw0.ID() {
+		t.Fatal("Expected epNw0 to be at the top of the heap after removing epIPv6. But did not find epNw0 at the top of the heap")
+	}
+
+	if err := epNw1.Leave(sbx); err != nil {
 		t.Fatal(err)
 	}