From f7e3cb12a6e728b3e62b9af14a9b191faaca1709 Mon Sep 17 00:00:00 2001 From: Steve Reed Date: Thu, 22 Jan 2015 14:12:37 -0800 Subject: [PATCH] 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 ad4ef5a798..8d4adbcbbd 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 a4cfa9c2f0..29bac114fb 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 0000000000..61ecc7e7d2 --- /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 0000000000..0c0c4c26d4 --- /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") + } +}