From 79a6bfb95f57b0e0ae1cb28cd466256e9f91f488 Mon Sep 17 00:00:00 2001 From: Steve Reed Date: Tue, 20 Jan 2015 16:01:01 -0800 Subject: [PATCH 1/5] Fixes #3640 by shuffling endpoints in the round-robin load balancer --- pkg/proxy/roundrobin.go | 17 ++- pkg/proxy/roundrobin_test.go | 222 +++++++++++++++++++++-------------- 2 files changed, 150 insertions(+), 89 deletions(-) diff --git a/pkg/proxy/roundrobin.go b/pkg/proxy/roundrobin.go index 0e50b0879b9..7b7391a0ca2 100644 --- a/pkg/proxy/roundrobin.go +++ b/pkg/proxy/roundrobin.go @@ -18,8 +18,10 @@ package proxy import ( "errors" + "math/rand" "net" "reflect" + "sort" "strconv" "sync" "time" @@ -170,6 +172,15 @@ func filterValidEndpoints(endpoints []string) []string { return result } +func shuffleEndpoints(endpoints []string) []string { + shuffled := make([]string, len(endpoints)) + perm := rand.Perm(len(endpoints)) + for i, v := range perm { + shuffled[v] = endpoints[i] + } + return shuffled +} + //remove any session affinity records associated to a particular endpoint (for example when a pod goes down). func removeSessionAffinityByEndpoint(lb *LoadBalancerRR, service string, endpoint string) { for _, affinityDetail := range lb.serviceDtlMap[service].sessionAffinityMap { @@ -210,6 +221,10 @@ func (lb *LoadBalancerRR) OnUpdate(endpoints []api.Endpoints) { for _, endpoint := range endpoints { existingEndpoints, exists := lb.endpointsMap[endpoint.Name] validEndpoints := filterValidEndpoints(endpoint.Endpoints) + // Need to compare sorted endpoints here, since they are shuffled below + // before being put into endpointsMap + sort.Strings(existingEndpoints) + sort.Strings(validEndpoints) if !exists || !reflect.DeepEqual(existingEndpoints, validEndpoints) { glog.V(3).Infof("LoadBalancerRR: Setting endpoints for %s to %+v", endpoint.Name, endpoint.Endpoints) updateServiceDetailMap(lb, endpoint.Name, validEndpoints) @@ -217,7 +232,7 @@ func (lb *LoadBalancerRR) OnUpdate(endpoints []api.Endpoints) { // to be safe we will call it here. A new service will only be created // if one does not already exist. lb.NewService(endpoint.Name, api.AffinityTypeNone, 0) - lb.endpointsMap[endpoint.Name] = validEndpoints + lb.endpointsMap[endpoint.Name] = shuffleEndpoints(validEndpoints) // Reset the round-robin index. lb.rrIndex[endpoint.Name] = 0 diff --git a/pkg/proxy/roundrobin_test.go b/pkg/proxy/roundrobin_test.go index bb9f54b030f..a4cfa9c2f06 100644 --- a/pkg/proxy/roundrobin_test.go +++ b/pkg/proxy/roundrobin_test.go @@ -18,6 +18,7 @@ package proxy import ( "net" + "sort" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -56,6 +57,26 @@ func TestFilterWorks(t *testing.T) { } } +func TestShuffleWorks(t *testing.T) { + endpoints := []string{"foobar:1", "foobar:2", "foobar:3"} + shuffled := shuffleEndpoints(endpoints) + + if len(shuffled) != 3 { + t.Errorf("Failed to shuffle to the correct size") + } + + sort.Strings(shuffled) + if shuffled[0] != "foobar:1" { + t.Errorf("Index zero is not foobar:1") + } + if shuffled[1] != "foobar:2" { + t.Errorf("Index one is not foobar:2") + } + if shuffled[2] != "foobar:3" { + t.Errorf("Index two is not foobar:3") + } +} + func TestLoadBalanceFailsWithNoEndpoints(t *testing.T) { loadBalancer := NewLoadBalancerRR() var endpoints []api.Endpoints @@ -75,7 +96,7 @@ func expectEndpoint(t *testing.T, loadBalancer *LoadBalancerRR, service string, t.Errorf("Didn't find a service for %s, expected %s, failed with: %v", service, expected, err) } if endpoint != expected { - t.Errorf("Didn't get expected endpoint for service %s, expected %s, got: %s", service, expected, endpoint) + t.Errorf("Didn't get expected endpoint for service %s client %v, expected %s, got: %s", service, netaddr, expected, endpoint) } } @@ -109,10 +130,11 @@ func TestLoadBalanceWorksWithMultipleEndpoints(t *testing.T) { Endpoints: []string{"endpoint:1", "endpoint:2", "endpoint:3"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", nil) + shuffledEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], nil) } func TestLoadBalanceWorksWithMultipleEndpointsAndUpdates(t *testing.T) { @@ -127,21 +149,23 @@ func TestLoadBalanceWorksWithMultipleEndpointsAndUpdates(t *testing.T) { Endpoints: []string{"endpoint:1", "endpoint:2", "endpoint:3"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", nil) + shuffledEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], nil) // Then update the configuration with one fewer endpoints, make sure // we start in the beginning again endpoints[0] = api.Endpoints{ObjectMeta: api.ObjectMeta{Name: "foo"}, Endpoints: []string{"endpoint:8", "endpoint:9"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:8", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:9", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:8", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:9", nil) + shuffledEndpoints = loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], nil) // Clear endpoints endpoints[0] = api.Endpoints{ObjectMeta: api.ObjectMeta{Name: "foo"}, Endpoints: []string{}} loadBalancer.OnUpdate(endpoints) @@ -168,17 +192,19 @@ func TestLoadBalanceWorksWithServiceRemoval(t *testing.T) { Endpoints: []string{"endpoint:4", "endpoint:5"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", nil) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", nil) + shuffledFooEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[2], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[1], nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", nil) + shuffledBarEndpoints := loadBalancer.endpointsMap["bar"] + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], nil) // Then update the configuration by removing foo loadBalancer.OnUpdate(endpoints[1:]) @@ -188,10 +214,10 @@ func TestLoadBalanceWorksWithServiceRemoval(t *testing.T) { } // but bar is still there, and we continue RR from where we left off. - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", nil) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[1], nil) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], nil) } func TestStickyLoadBalanceWorksWithSingleEndpoint(t *testing.T) { @@ -232,14 +258,15 @@ func TestStickyLoadBalanaceWorksWithMultipleEndpoints(t *testing.T) { Endpoints: []string{"endpoint:1", "endpoint:2", "endpoint:3"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) + shuffledEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], client3) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], client3) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) } func TestStickyLoadBalanaceWorksWithMultipleEndpointsStickyNone(t *testing.T) { @@ -259,14 +286,15 @@ func TestStickyLoadBalanaceWorksWithMultipleEndpointsStickyNone(t *testing.T) { Endpoints: []string{"endpoint:1", "endpoint:2", "endpoint:3"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client1) + shuffledEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client3) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], client3) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client1) } func TestStickyLoadBalanaceWorksWithMultipleEndpointsRemoveOne(t *testing.T) { @@ -289,32 +317,45 @@ func TestStickyLoadBalanaceWorksWithMultipleEndpointsRemoveOne(t *testing.T) { Endpoints: []string{"endpoint:1", "endpoint:2", "endpoint:3"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client3) + shuffledEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + client1Endpoint := shuffledEndpoints[0] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + client2Endpoint := shuffledEndpoints[1] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], client3) + client3Endpoint := shuffledEndpoints[2] endpoints[0] = api.Endpoints{ ObjectMeta: api.ObjectMeta{Name: "foo"}, Endpoints: []string{"endpoint:1", "endpoint:2"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client3) + shuffledEndpoints = loadBalancer.endpointsMap["foo"] + if client1Endpoint == "endpoint:3" { + client1Endpoint = shuffledEndpoints[0] + } else if client2Endpoint == "endpoint:3" { + client2Endpoint = shuffledEndpoints[0] + } else if client3Endpoint == "endpoint:3" { + client3Endpoint = shuffledEndpoints[0] + } + expectEndpoint(t, loadBalancer, "foo", client1Endpoint, client1) + expectEndpoint(t, loadBalancer, "foo", client2Endpoint, client2) + expectEndpoint(t, loadBalancer, "foo", client3Endpoint, client3) endpoints[0] = api.Endpoints{ ObjectMeta: api.ObjectMeta{Name: "foo"}, Endpoints: []string{"endpoint:1", "endpoint:2", "endpoint:4"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client4) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client5) - expectEndpoint(t, loadBalancer, "foo", "endpoint:4", client6) + shuffledEndpoints = loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", client1Endpoint, client1) + expectEndpoint(t, loadBalancer, "foo", client2Endpoint, client2) + expectEndpoint(t, loadBalancer, "foo", client3Endpoint, client3) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client4) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client5) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], client6) } func TestStickyLoadBalanceWorksWithMultipleEndpointsAndUpdates(t *testing.T) { @@ -334,24 +375,26 @@ func TestStickyLoadBalanceWorksWithMultipleEndpointsAndUpdates(t *testing.T) { Endpoints: []string{"endpoint:1", "endpoint:2", "endpoint:3"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) + shuffledEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[2], client3) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) // Then update the configuration with one fewer endpoints, make sure // we start in the beginning again endpoints[0] = api.Endpoints{ObjectMeta: api.ObjectMeta{Name: "foo"}, Endpoints: []string{"endpoint:4", "endpoint:5"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:5", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:5", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:5", client2) + shuffledEndpoints = loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledEndpoints[1], client2) // Clear endpoints endpoints[0] = api.Endpoints{ObjectMeta: api.ObjectMeta{Name: "foo"}, Endpoints: []string{}} @@ -384,19 +427,21 @@ func TestStickyLoadBalanceWorksWithServiceRemoval(t *testing.T) { Endpoints: []string{"endpoint:4", "endpoint:5"}, } loadBalancer.OnUpdate(endpoints) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:3", client3) - expectEndpoint(t, loadBalancer, "foo", "endpoint:1", client1) - expectEndpoint(t, loadBalancer, "foo", "endpoint:2", client2) + shuffledFooEndpoints := loadBalancer.endpointsMap["foo"] + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[2], client3) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[2], client3) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[1], client2) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", client2) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", client2) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) + shuffledBarEndpoints := loadBalancer.endpointsMap["bar"] + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "foo", shuffledFooEndpoints[0], client1) // Then update the configuration by removing foo loadBalancer.OnUpdate(endpoints[1:]) @@ -406,10 +451,11 @@ func TestStickyLoadBalanceWorksWithServiceRemoval(t *testing.T) { } // but bar is still there, and we continue RR from where we left off. - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", client2) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "bar", "endpoint:5", client2) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) - expectEndpoint(t, loadBalancer, "bar", "endpoint:4", client1) + shuffledBarEndpoints = loadBalancer.endpointsMap["bar"] + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[1], client2) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], client1) + expectEndpoint(t, loadBalancer, "bar", shuffledBarEndpoints[0], client1) } From 38241c7e80cfaf6d2e4f88fe2d66716baed38b16 Mon Sep 17 00:00:00 2001 From: Steve Reed Date: Wed, 21 Jan 2015 10:52:09 -0800 Subject: [PATCH 2/5] Copies endpoint slices before any sorting --- pkg/proxy/roundrobin.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/roundrobin.go b/pkg/proxy/roundrobin.go index 7b7391a0ca2..ad4ef5a798a 100644 --- a/pkg/proxy/roundrobin.go +++ b/pkg/proxy/roundrobin.go @@ -172,6 +172,21 @@ func filterValidEndpoints(endpoints []string) []string { return result } +func endpointsAreEqual(left, right []string) bool { + if len(left) != len(right) { + return false + } + + leftSorted := make([]string, len(left)) + copy(leftSorted, left) + sort.Strings(leftSorted) + rightSorted := make([]string, len(right)) + copy(rightSorted, right) + sort.Strings(rightSorted) + + return reflect.DeepEqual(leftSorted, rightSorted) +} + func shuffleEndpoints(endpoints []string) []string { shuffled := make([]string, len(endpoints)) perm := rand.Perm(len(endpoints)) @@ -221,11 +236,7 @@ func (lb *LoadBalancerRR) OnUpdate(endpoints []api.Endpoints) { for _, endpoint := range endpoints { existingEndpoints, exists := lb.endpointsMap[endpoint.Name] validEndpoints := filterValidEndpoints(endpoint.Endpoints) - // Need to compare sorted endpoints here, since they are shuffled below - // before being put into endpointsMap - sort.Strings(existingEndpoints) - sort.Strings(validEndpoints) - if !exists || !reflect.DeepEqual(existingEndpoints, validEndpoints) { + if !exists || !endpointsAreEqual(existingEndpoints, validEndpoints) { glog.V(3).Infof("LoadBalancerRR: Setting endpoints for %s to %+v", endpoint.Name, endpoint.Endpoints) updateServiceDetailMap(lb, endpoint.Name, validEndpoints) // On update can be called without NewService being called externally. From f7e3cb12a6e728b3e62b9af14a9b191faaca1709 Mon Sep 17 00:00:00 2001 From: Steve Reed Date: Thu, 22 Jan 2015 14:12:37 -0800 Subject: [PATCH 3/5] Moves string slice sorting, copying and shuffling into pkg/util/slice --- pkg/proxy/roundrobin.go | 31 ++------------------ pkg/proxy/roundrobin_test.go | 21 -------------- pkg/util/slice/slice.go | 49 +++++++++++++++++++++++++++++++ pkg/util/slice/slice_test.go | 56 ++++++++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 49 deletions(-) create mode 100644 pkg/util/slice/slice.go create mode 100644 pkg/util/slice/slice_test.go diff --git a/pkg/proxy/roundrobin.go b/pkg/proxy/roundrobin.go index ad4ef5a798a..8d4adbcbbd7 100644 --- a/pkg/proxy/roundrobin.go +++ b/pkg/proxy/roundrobin.go @@ -18,15 +18,14 @@ package proxy import ( "errors" - "math/rand" "net" "reflect" - "sort" "strconv" "sync" "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util/slice" "github.com/golang/glog" ) @@ -172,30 +171,6 @@ func filterValidEndpoints(endpoints []string) []string { return result } -func endpointsAreEqual(left, right []string) bool { - if len(left) != len(right) { - return false - } - - leftSorted := make([]string, len(left)) - copy(leftSorted, left) - sort.Strings(leftSorted) - rightSorted := make([]string, len(right)) - copy(rightSorted, right) - sort.Strings(rightSorted) - - return reflect.DeepEqual(leftSorted, rightSorted) -} - -func shuffleEndpoints(endpoints []string) []string { - shuffled := make([]string, len(endpoints)) - perm := rand.Perm(len(endpoints)) - for i, v := range perm { - shuffled[v] = endpoints[i] - } - return shuffled -} - //remove any session affinity records associated to a particular endpoint (for example when a pod goes down). func removeSessionAffinityByEndpoint(lb *LoadBalancerRR, service string, endpoint string) { for _, affinityDetail := range lb.serviceDtlMap[service].sessionAffinityMap { @@ -236,14 +211,14 @@ func (lb *LoadBalancerRR) OnUpdate(endpoints []api.Endpoints) { for _, endpoint := range endpoints { existingEndpoints, exists := lb.endpointsMap[endpoint.Name] validEndpoints := filterValidEndpoints(endpoint.Endpoints) - if !exists || !endpointsAreEqual(existingEndpoints, validEndpoints) { + if !exists || !reflect.DeepEqual(slice.SortStrings(slice.CopyStrings(existingEndpoints)), slice.SortStrings(validEndpoints)) { glog.V(3).Infof("LoadBalancerRR: Setting endpoints for %s to %+v", endpoint.Name, endpoint.Endpoints) updateServiceDetailMap(lb, endpoint.Name, validEndpoints) // On update can be called without NewService being called externally. // to be safe we will call it here. A new service will only be created // if one does not already exist. lb.NewService(endpoint.Name, api.AffinityTypeNone, 0) - lb.endpointsMap[endpoint.Name] = shuffleEndpoints(validEndpoints) + lb.endpointsMap[endpoint.Name] = slice.ShuffleStrings(validEndpoints) // Reset the round-robin index. lb.rrIndex[endpoint.Name] = 0 diff --git a/pkg/proxy/roundrobin_test.go b/pkg/proxy/roundrobin_test.go index a4cfa9c2f06..29bac114fbc 100644 --- a/pkg/proxy/roundrobin_test.go +++ b/pkg/proxy/roundrobin_test.go @@ -18,7 +18,6 @@ package proxy import ( "net" - "sort" "testing" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -57,26 +56,6 @@ func TestFilterWorks(t *testing.T) { } } -func TestShuffleWorks(t *testing.T) { - endpoints := []string{"foobar:1", "foobar:2", "foobar:3"} - shuffled := shuffleEndpoints(endpoints) - - if len(shuffled) != 3 { - t.Errorf("Failed to shuffle to the correct size") - } - - sort.Strings(shuffled) - if shuffled[0] != "foobar:1" { - t.Errorf("Index zero is not foobar:1") - } - if shuffled[1] != "foobar:2" { - t.Errorf("Index one is not foobar:2") - } - if shuffled[2] != "foobar:3" { - t.Errorf("Index two is not foobar:3") - } -} - func TestLoadBalanceFailsWithNoEndpoints(t *testing.T) { loadBalancer := NewLoadBalancerRR() var endpoints []api.Endpoints diff --git a/pkg/util/slice/slice.go b/pkg/util/slice/slice.go new file mode 100644 index 00000000000..61ecc7e7d27 --- /dev/null +++ b/pkg/util/slice/slice.go @@ -0,0 +1,49 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package slice provides utility methods for common operations on slices. +package slice + +import ( + "math/rand" + "sort" +) + +// CopyStrings copies the contents of the specified string slice +// into a new slice. +func CopyStrings(s []string) []string { + c := make([]string, len(s)) + copy(c, s) + return c +} + +// SortStrings sorts the specified string slice in place. It returns the same +// slice that was provided in order to facilitate method chaining. +func SortStrings(s []string) []string { + sort.Strings(s) + return s +} + +// ShuffleStrings copies strings from the specified slice into a copy in random +// order. It returns a new slice. +func ShuffleStrings(s []string) []string { + shuffled := make([]string, len(s)) + perm := rand.Perm(len(s)) + for i, j := range perm { + shuffled[j] = s[i] + } + return shuffled +} diff --git a/pkg/util/slice/slice_test.go b/pkg/util/slice/slice_test.go new file mode 100644 index 00000000000..0c0c4c26d4a --- /dev/null +++ b/pkg/util/slice/slice_test.go @@ -0,0 +1,56 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package slice + +import ( + "reflect" + "testing" +) + +func TestCopyStrings(t *testing.T) { + src := []string{"a", "c", "b"} + dest := CopyStrings(src) + + if !reflect.DeepEqual(src, dest) { + t.Errorf("%v and %v are not equal", src, dest) + } + + src[0] = "A" + if reflect.DeepEqual(src, dest) { + t.Errorf("CopyStrings didn't make a copy") + } +} + +func TestSortStrings(t *testing.T) { + src := []string{"a", "c", "b"} + dest := SortStrings(src) + expected := []string{"a", "b", "c"} + + if !reflect.DeepEqual(dest, expected) { + t.Errorf("SortString didn't sort the strings") + } +} + +func TestSortStringsSortsInPlace(t *testing.T) { + src := []string{"a", "c", "b"} + _ = SortStrings(src) + expected := []string{"a", "b", "c"} + + if !reflect.DeepEqual(src, expected) { + t.Errorf("SortString didn't sort in place") + } +} From ab1ede3aaae359ed40a4199b7599e60b7ebbfe79 Mon Sep 17 00:00:00 2001 From: Steve Reed Date: Thu, 22 Jan 2015 14:24:07 -0800 Subject: [PATCH 4/5] Sets year in copyright to 2015 --- pkg/util/slice/slice.go | 2 +- pkg/util/slice/slice_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/util/slice/slice.go b/pkg/util/slice/slice.go index 61ecc7e7d27..d6f5eb74e42 100644 --- a/pkg/util/slice/slice.go +++ b/pkg/util/slice/slice.go @@ -1,5 +1,5 @@ /* -Copyright 2014 Google Inc. All rights reserved. +Copyright 2015 Google Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/pkg/util/slice/slice_test.go b/pkg/util/slice/slice_test.go index 0c0c4c26d4a..2f842fcc068 100644 --- a/pkg/util/slice/slice_test.go +++ b/pkg/util/slice/slice_test.go @@ -1,5 +1,5 @@ /* -Copyright 2014 Google Inc. All rights reserved. +Copyright 2015 Google Inc. All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. From a186b47cd3d6387651111045a027c4c88107cf4b Mon Sep 17 00:00:00 2001 From: Steve Reed Date: Fri, 23 Jan 2015 09:30:09 -0800 Subject: [PATCH 5/5] Adds test for slice.ShuffleStrings --- pkg/util/slice/slice_test.go | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/pkg/util/slice/slice_test.go b/pkg/util/slice/slice_test.go index 2f842fcc068..a0a61a5b1d4 100644 --- a/pkg/util/slice/slice_test.go +++ b/pkg/util/slice/slice_test.go @@ -43,14 +43,28 @@ func TestSortStrings(t *testing.T) { if !reflect.DeepEqual(dest, expected) { t.Errorf("SortString didn't sort the strings") } -} - -func TestSortStringsSortsInPlace(t *testing.T) { - src := []string{"a", "c", "b"} - _ = SortStrings(src) - expected := []string{"a", "b", "c"} if !reflect.DeepEqual(src, expected) { t.Errorf("SortString didn't sort in place") } } + +func TestShuffleStrings(t *testing.T) { + src := []string{"a", "b", "c", "d", "e", "f"} + dest := ShuffleStrings(src) + + if len(src) != len(dest) { + t.Errorf("Shuffled slice is wrong length, expected %v got %v", len(src), len(dest)) + } + + m := make(map[string]bool, len(dest)) + for _, s := range dest { + m[s] = true + } + + for _, k := range src { + if _, exists := m[k]; !exists { + t.Errorf("Element %v missing from shuffled slice", k) + } + } +}