Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use hash in federated replica set planner #31814

Merged
merged 1 commit into from
Sep 1, 2016
Merged

Use hash in federated replica set planner #31814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 15 additions & 5 deletions federation/pkg/federation-controller/replicaset/planner/planner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package planer

import (
"hash/fnv"
"math"
"sort"

Expand All @@ -31,6 +32,7 @@ type Planner struct {

type namedClusterReplicaSetPreferences struct {
clusterName string
hash uint32
fed_api.ClusterReplicaSetPreferences
}

Expand All @@ -39,9 +41,10 @@ type byWeight []*namedClusterReplicaSetPreferences
func (a byWeight) Len() int { return len(a) }
func (a byWeight) Swap(i, j int) { a[i], a[j] = a[j], a[i] }

// Preferences are sorted according by decreasing weight and increasing clusterName.
// Preferences are sorted according by decreasing weight and increasing hash (built on top of cluster name and rs name).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually - I thought that I understood this change initially, but now I no longer think I understand it.

Please add a comment, why you actually can't compare based on clusterName here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

// Sorting is made by a hash to avoid assigning single-replica rs to the alphabetically smallest cluster.
func (a byWeight) Less(i, j int) bool {
return (a[i].Weight > a[j].Weight) || (a[i].Weight == a[j].Weight && a[i].clusterName < a[j].clusterName)
return (a[i].Weight > a[j].Weight) || (a[i].Weight == a[j].Weight && a[i].hash < a[j].hash)
}

func NewPlanner(preferences *fed_api.FederatedReplicaSetPreferences) *Planner {
Expand All @@ -56,21 +59,28 @@ func NewPlanner(preferences *fed_api.FederatedReplicaSetPreferences) *Planner {
// have all of the replicas assigned. In such case a cluster with higher weight has priority over
// cluster with lower weight (or with lexicographically smaller name in case of draw).
// It can also use the current replica count and estimated capacity to provide better planning and
// adhere to rebalance policy.
// adhere to rebalance policy. To avoid prioritization of clusters with smaller lexiconographical names
// a semi-random string (like replica set name) can be provided.
// Two maps are returned:
// * a map that contains information how many replicas will be possible to run in a cluster.
// * a map that contains information how many extra replicas would be nice to schedule in a cluster so,
// if by chance, they are scheudled we will be closer to the desired replicas layout.
func (p *Planner) Plan(replicasToDistribute int64, availableClusters []string, currentReplicaCount map[string]int64,
estimatedCapacity map[string]int64) (map[string]int64, map[string]int64) {
estimatedCapacity map[string]int64, replicaSetKey string) (map[string]int64, map[string]int64) {

preferences := make([]*namedClusterReplicaSetPreferences, 0, len(availableClusters))
plan := make(map[string]int64, len(preferences))
overflow := make(map[string]int64, len(preferences))

named := func(name string, pref fed_api.ClusterReplicaSetPreferences) *namedClusterReplicaSetPreferences {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it probably should be:

named := func(clusterName string, pref ...)

[the name is actually name of a cluster right? And it's completely not clear for here.]

// Seems to work better than addler for our case.
hasher := fnv.New32()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all places in the codebases we are using adler32 hash, see e.g.:
https://github.com/kubernetes/kubernetes/blob/master/pkg/util/pod/pod.go#L34

I would like to be consistent and use the same hasing method here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some experiments and adler32 tends to preserve alphabetical order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand - alphabetical order of what?
If you are explicitly calling "hasher.Write(...)" on it, what alphabetical order are you talking about here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment.

hasher.Write([]byte(name))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may consider using this one:
https://github.com/kubernetes/kubernetes/blob/master/pkg/util/hash/hash.go#L28

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't need a hash over a whole object. Just name+namespace.

hasher.Write([]byte(replicaSetKey))

return &namedClusterReplicaSetPreferences{
clusterName: name,
clusterName: name,
hash: hasher.Sum32(),
ClusterReplicaSetPreferences: pref,
}
}
Expand Down
24 changes: 15 additions & 9 deletions federation/pkg/federation-controller/replicaset/planner/planner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func doCheck(t *testing.T, pref map[string]fed_api.ClusterReplicaSetPreferences,
planer := NewPlanner(&fed_api.FederatedReplicaSetPreferences{
Clusters: pref,
})
plan, overflow := planer.Plan(replicas, clusters, map[string]int64{}, map[string]int64{})
plan, overflow := planer.Plan(replicas, clusters, map[string]int64{}, map[string]int64{}, "")
assert.EqualValues(t, expected, plan)
assert.Equal(t, 0, len(overflow))
}
Expand All @@ -38,7 +38,7 @@ func doCheckWithExisting(t *testing.T, pref map[string]fed_api.ClusterReplicaSet
planer := NewPlanner(&fed_api.FederatedReplicaSetPreferences{
Clusters: pref,
})
plan, overflow := planer.Plan(replicas, clusters, existing, map[string]int64{})
plan, overflow := planer.Plan(replicas, clusters, existing, map[string]int64{}, "")
assert.Equal(t, 0, len(overflow))
assert.EqualValues(t, expected, plan)
}
Expand All @@ -52,7 +52,7 @@ func doCheckWithExistingAndCapacity(t *testing.T, rebalance bool, pref map[strin
Rebalance: rebalance,
Clusters: pref,
})
plan, overflow := planer.Plan(replicas, clusters, existing, capacity)
plan, overflow := planer.Plan(replicas, clusters, existing, capacity, "")
assert.EqualValues(t, expected, plan)
assert.Equal(t, expectedOverflow, overflow)
}
Expand All @@ -65,7 +65,8 @@ func TestEqual(t *testing.T) {
doCheck(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {Weight: 1}},
50, []string{"A", "B", "C"},
map[string]int64{"A": 17, "B": 17, "C": 16})
// hash dependent
map[string]int64{"A": 16, "B": 17, "C": 17})

doCheck(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {Weight: 1}},
Expand All @@ -75,12 +76,14 @@ func TestEqual(t *testing.T) {
doCheck(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {Weight: 1}},
1, []string{"A", "B"},
map[string]int64{"A": 1, "B": 0})
// hash dependent
map[string]int64{"A": 0, "B": 1})

doCheck(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {Weight: 1}},
1, []string{"A", "B", "C", "D"},
map[string]int64{"A": 1, "B": 0, "C": 0, "D": 0})
// hash dependent
map[string]int64{"A": 0, "B": 0, "C": 0, "D": 1})

doCheck(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {Weight: 1}},
Expand Down Expand Up @@ -122,7 +125,9 @@ func TestEqualWithExisting(t *testing.T) {
"*": {Weight: 1}},
50, []string{"A", "B"},
map[string]int64{"A": 10, "B": 70},
map[string]int64{"A": 10, "B": 40})
// hash dependent
// TODO: Should be 10:40, update algorithm. Issue: #31816
map[string]int64{"A": 0, "B": 50})

doCheckWithExisting(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {Weight: 1}},
Expand All @@ -145,7 +150,7 @@ func TestWithExistingAndCapacity(t *testing.T) {
map[string]int64{},
map[string]int64{"C": 10},
map[string]int64{"A": 20, "B": 20, "C": 10},
map[string]int64{"C": 6})
map[string]int64{"C": 7})

// desired B:50 C:0
doCheckWithExistingAndCapacity(t, true, map[string]fed_api.ClusterReplicaSetPreferences{
Expand Down Expand Up @@ -228,7 +233,8 @@ func TestMin(t *testing.T) {
doCheck(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {MinReplicas: 20, Weight: 0}},
50, []string{"A", "B", "C"},
map[string]int64{"A": 20, "B": 20, "C": 10})
// hash dependant.
map[string]int64{"A": 10, "B": 20, "C": 20})

doCheck(t, map[string]fed_api.ClusterReplicaSetPreferences{
"*": {MinReplicas: 20, Weight: 0},
Expand Down
3 changes: 2 additions & 1 deletion federation/pkg/federation-controller/replicaset/replicasetcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,8 @@ func (frsc *ReplicaSetController) schedule(frs *extensionsv1.ReplicaSet, cluster
for _, cluster := range clusters {
clusterNames = append(clusterNames, cluster.Name)
}
scheduleResult, overflow := plnr.Plan(replicas, clusterNames, current, estimatedCapacity)
scheduleResult, overflow := plnr.Plan(replicas, clusterNames, current, estimatedCapacity,
frs.Namespace+"/"+frs.Name)
// make sure the return contains clusters need to zero the replicas
result := make(map[string]int64)
for clusterName := range current {
Expand Down