From 40923e73532e765763c00fec0a27d712b4d219e3 Mon Sep 17 00:00:00 2001 From: Christoph Ziebuhr Date: Tue, 27 Feb 2018 17:15:31 +0100 Subject: [PATCH 1/3] Use ordered array instead of heap for sb.endpoints Signed-off-by: Christoph Ziebuhr --- libnetwork/controller.go | 5 +--- libnetwork/endpoint.go | 5 +--- libnetwork/sandbox.go | 59 ++++++++++++++++++------------------- libnetwork/sandbox_store.go | 6 ++-- 4 files changed, 32 insertions(+), 43 deletions(-) diff --git a/libnetwork/controller.go b/libnetwork/controller.go index 225f7fa609..fe9c4ddf97 100644 --- a/libnetwork/controller.go +++ b/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() diff --git a/libnetwork/endpoint.go b/libnetwork/endpoint.go index 1e1b6a1675..822b9fe173 100644 --- a/libnetwork/endpoint.go +++ b/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() - heap.Push(&sb.endpoints, ep) - sb.Unlock() + sb.addEndpoint(ep) defer func() { if err != nil { sb.removeEndpoint(ep) diff --git a/libnetwork/sandbox.go b/libnetwork/sandbox.go index 423066c128..41e8d01c5d 100644 --- a/libnetwork/sandbox.go +++ b/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{} @@ -360,13 +358,31 @@ func (sb *sandbox) getConnectedEndpoints() []*endpoint { 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 +956,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,19 +1181,14 @@ func OptionIngress() SandboxOption { } } -func (eh epHeap) Len() int { return len(eh) } - -func (eh epHeap) Less(i, j int) bool { +func (epi *endpoint) Less(epj *endpoint) bool { var ( cip, cjp int ok bool ) - ci, _ := eh[i].getSandbox() - cj, _ := eh[j].getSandbox() - - epi := eh[i] - epj := eh[j] + ci, _ := epi.getSandbox() + cj, _ := epj.getSandbox() if epi.endpointInGWNetwork() { return false @@ -1207,40 +1218,26 @@ func (eh epHeap) Less(i, j int) bool { } if ci != nil { - cip, ok = ci.epPriority[eh[i].ID()] + cip, ok = ci.epPriority[epi.ID()] if !ok { cip = 0 } } if cj != nil { - cjp, ok = cj.epPriority[eh[j].ID()] + cjp, ok = cj.epPriority[epj.ID()] if !ok { cjp = 0 } } if cip == cjp { - return eh[i].network.Name() < eh[j].network.Name() + return epi.network.Name() < epj.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 { return sb.ndotsSet } diff --git a/libnetwork/sandbox_store.go b/libnetwork/sandbox_store.go index a083644598..ebc884e6f0 100644 --- a/libnetwork/sandbox_store.go +++ b/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 { From 6362d28969ab49b7a53a7569b5512037258a1a85 Mon Sep 17 00:00:00 2001 From: Christoph Ziebuhr Date: Thu, 8 Mar 2018 11:45:04 +0100 Subject: [PATCH 2/3] Make go-tools happy Signed-off-by: Christoph Ziebuhr --- libnetwork/resolver.go | 2 +- libnetwork/sandbox.go | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/libnetwork/resolver.go b/libnetwork/resolver.go index 64c73e53b7..c2b1f4e656 100644 --- a/libnetwork/resolver.go +++ b/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) diff --git a/libnetwork/sandbox.go b/libnetwork/sandbox.go index 41e8d01c5d..baf83e46df 100644 --- a/libnetwork/sandbox.go +++ b/libnetwork/sandbox.go @@ -351,9 +351,7 @@ func (sb *sandbox) getConnectedEndpoints() []*endpoint { defer sb.Unlock() eps := make([]*endpoint, len(sb.endpoints)) - for i, ep := range sb.endpoints { - eps[i] = ep - } + copy(eps, sb.endpoints) return eps } From 67dbb048520513de23be771d5e677a4103e173ec Mon Sep 17 00:00:00 2001 From: Christoph Ziebuhr Date: Wed, 28 Mar 2018 16:15:55 +0200 Subject: [PATCH 3/3] Improve interface order Signed-off-by: Christoph Ziebuhr --- libnetwork/sandbox.go | 74 ++++++++++++++++++-------------- libnetwork/sandbox_test.go | 88 +++++++++++++++++++++++++++++--------- 2 files changed, 109 insertions(+), 53 deletions(-) diff --git a/libnetwork/sandbox.go b/libnetwork/sandbox.go index baf83e46df..3cf88f4e84 100644 --- a/libnetwork/sandbox.go +++ b/libnetwork/sandbox.go @@ -1179,61 +1179,69 @@ func OptionIngress() SandboxOption { } } +// <=> Returns true if a < b, false if a > b and advances to next level if a == b +// epi.prio <=> epj.prio # 2 < 1 +// 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 - ok bool + prioi, prioj int ) - ci, _ := epi.getSandbox() - cj, _ := epj.getSandbox() + sbi, _ := epi.getSandbox() + sbj, _ := epj.getSandbox() - if epi.endpointInGWNetwork() { - return false + // Prio defaults to 0 + if sbi != nil { + prioi = sbi.epPriority[epi.ID()] + } + if sbj != nil { + prioj = sbj.epPriority[epj.ID()] } - if epj.endpointInGWNetwork() { - return true + if prioi != prioj { + return prioi > prioj } - if epi.getNetwork().Internal() { - return false + gwi := epi.endpointInGWNetwork() + gwj := epj.endpointInGWNetwork() + if gwi != gwj { + return gwj } - if epj.getNetwork().Internal() { - return true + inti := epi.getNetwork().Internal() + intj := epj.getNetwork().Internal() + if inti != intj { + return intj } - if epi.joinInfo != nil && epj.joinInfo != nil { - if (epi.joinInfo.gw != nil && epi.joinInfo.gw6 != nil) && - (epj.joinInfo.gw == nil || epj.joinInfo.gw6 == nil) { - return true + jii := 0 + if epi.joinInfo != nil { + if epi.joinInfo.gw != nil { + jii = jii + 1 } - if (epj.joinInfo.gw != nil && epj.joinInfo.gw6 != nil) && - (epi.joinInfo.gw == nil || epi.joinInfo.gw6 == nil) { - return false + if epi.joinInfo.gw6 != nil { + jii = jii + 2 } } - if ci != nil { - cip, ok = ci.epPriority[epi.ID()] - if !ok { - cip = 0 + jij := 0 + if epj.joinInfo != nil { + if epj.joinInfo.gw != nil { + jij = jij + 1 + } + if epj.joinInfo.gw6 != nil { + jij = jij + 2 } } - if cj != nil { - cjp, ok = cj.epPriority[epj.ID()] - if !ok { - cjp = 0 - } + if jii != jij { + return jii > jij } - if cip == cjp { - return epi.network.Name() < epj.network.Name() - } - - return cip > cjp + return epi.network.Name() < epj.network.Name() } func (sb *sandbox) NdotsSet() bool { diff --git a/libnetwork/sandbox_test.go b/libnetwork/sandbox_test.go index 76a55699ef..82e5e8b830 100644 --- a/libnetwork/sandbox_test.go +++ b/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) - for i := 0; i < numNetworks; i++ { + nwList := make([]Network, 0, len(opts)) + 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() { - 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 { + if err := epNw1.Join(sbx); err != nil { t.Fatal(err) } - if ctrlr.sandboxes[sid].endpoints[0].ID() != ep2.ID() { - 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") + if err := epIPv6.Join(sbx); err != nil { + 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) }