From 1c8cc73fee86c98a2d130189cda5ca5b1069eb37 Mon Sep 17 00:00:00 2001 From: MichalK Date: Thu, 6 Oct 2022 09:36:42 +0200 Subject: [PATCH] [WRR] remove percentage (#953) closes #930 remove percentage, using portions instead. see #930 Signed-off-by: kuritka Signed-off-by: kuritka --- api/v1beta1/gslb_types.go | 32 +------- api/v1beta1/zz_generated.deepcopy.go | 23 +----- chart/k8gb/crd/k8gb.absa.oss_gslbs.yaml | 5 +- controllers/depresolver/depresolver_spec.go | 14 +--- controllers/depresolver/depresolver_test.go | 80 ++++++++----------- .../testdata/filled_omitempty.yaml | 6 +- controllers/dnsupdate.go | 2 +- ...test.go => gslb_controller_weight_test.go} | 36 ++++----- terratest/examples/roundrobin_weight1.yaml | 4 +- terratest/test/k8gb_weight_test.go | 4 +- 10 files changed, 66 insertions(+), 140 deletions(-) rename controllers/{weight_test.go => gslb_controller_weight_test.go} (82%) diff --git a/api/v1beta1/gslb_types.go b/api/v1beta1/gslb_types.go index 50465bddba..c083f52afe 100644 --- a/api/v1beta1/gslb_types.go +++ b/api/v1beta1/gslb_types.go @@ -19,9 +19,6 @@ Generated by GoLic, for more details see: https://github.com/AbsaOSS/golic */ import ( - "strconv" - "strings" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -33,8 +30,8 @@ import ( type Strategy struct { // Load balancing strategy type:(roundRobin|failover) Type string `json:"type"` - // roundrobin and in the future consistent may (but also may not) contain a weight - Weight Weight `json:"weight,omitempty"` + // Weight is defined by map region:weight + Weight map[string]int `json:"weight,omitempty"` // Primary Geo Tag. Valid for failover strategy only PrimaryGeoTag string `json:"primaryGeoTag,omitempty"` // Defines DNS record TTL in seconds @@ -103,28 +100,3 @@ func (h HealthStatus) String() string { func init() { SchemeBuilder.Register(&Gslb{}, &GslbList{}) } - -type Percentage string - -type Weight map[string]Percentage - -func (p Percentage) String() string { - return string(p) -} - -func (p Percentage) IsEmpty() bool { - return string(p) == "" -} - -func (p Percentage) TryParse() (v int, err error) { - return strconv.Atoi(strings.TrimSuffix(strings.ReplaceAll(p.String(), " ", ""), "%")) -} - -func (p Percentage) Int() int { - v, _ := p.TryParse() - return v -} - -func (w Weight) IsEmpty() bool { - return len(w) == 0 -} diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index e1099381ab..a6b00336fd 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -175,7 +175,7 @@ func (in *Strategy) DeepCopyInto(out *Strategy) { *out = *in if in.Weight != nil { in, out := &in.Weight, &out.Weight - *out = make(Weight, len(*in)) + *out = make(map[string]int, len(*in)) for key, val := range *in { (*out)[key] = val } @@ -191,24 +191,3 @@ func (in *Strategy) DeepCopy() *Strategy { in.DeepCopyInto(out) return out } - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in Weight) DeepCopyInto(out *Weight) { - { - in := &in - *out = make(Weight, len(*in)) - for key, val := range *in { - (*out)[key] = val - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Weight. -func (in Weight) DeepCopy() Weight { - if in == nil { - return nil - } - out := new(Weight) - in.DeepCopyInto(out) - return *out -} diff --git a/chart/k8gb/crd/k8gb.absa.oss_gslbs.yaml b/chart/k8gb/crd/k8gb.absa.oss_gslbs.yaml index f2ac5c7b7c..05c67224f0 100644 --- a/chart/k8gb/crd/k8gb.absa.oss_gslbs.yaml +++ b/chart/k8gb/crd/k8gb.absa.oss_gslbs.yaml @@ -324,9 +324,8 @@ spec: type: string weight: additionalProperties: - type: string - description: roundrobin and in the future consistent may (but - also may not) contain a weight + type: integer + description: Weight is defined by map region:weight type: object required: - type diff --git a/controllers/depresolver/depresolver_spec.go b/controllers/depresolver/depresolver_spec.go index 0be4e0998f..daa10451e3 100644 --- a/controllers/depresolver/depresolver_spec.go +++ b/controllers/depresolver/depresolver_spec.go @@ -69,29 +69,19 @@ func (dr *DependencyResolver) validateSpec(strategy k8gbv1beta1.Strategy) (err e if err != nil { return } - if !strategy.Weight.IsEmpty() { + if len(strategy.Weight) != 0 { if strategy.Type != RoundRobinStrategy { return fmt.Errorf(`weight is allowed only for roundRobin strategy`) } - summary := 0 for k, w := range strategy.Weight { - var wi int err = field("weight", k).isNotEmpty().matchRegexp(geoTagRegex).err if err != nil { return err } - wi, err = w.TryParse() - if err != nil { - return fmt.Errorf("weight invalid value %s", w.String()) - } - err = field("weight", wi).isHigherOrEqualToZero().isLessOrEqualTo(100).err + err = field("weight", w).isHigherOrEqualToZero().isLessOrEqualTo(1000).err if err != nil { return err } - summary += wi - } - if summary != 100 { - return fmt.Errorf("weight the sum of the weights must be equal to 100") } } return nil diff --git a/controllers/depresolver/depresolver_test.go b/controllers/depresolver/depresolver_test.go index 8f5912af0c..5c4b2fdd8e 100644 --- a/controllers/depresolver/depresolver_test.go +++ b/controllers/depresolver/depresolver_test.go @@ -98,10 +98,10 @@ func TestResolveSpecWithFilledFields(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 35, gslb.Spec.Strategy.DNSTtlSeconds) assert.Equal(t, 305, gslb.Spec.Strategy.SplitBrainThresholdSeconds) - assert.False(t, gslb.Spec.Strategy.Weight.IsEmpty()) - assert.Equal(t, 15, gslb.Spec.Strategy.Weight["za"].Int()) - assert.Equal(t, 25, gslb.Spec.Strategy.Weight["eu"].Int()) - assert.Equal(t, 60, gslb.Spec.Strategy.Weight["us"].Int()) + assert.False(t, len(gslb.Spec.Strategy.Weight) == 0) + assert.Equal(t, 15, gslb.Spec.Strategy.Weight["za"]) + assert.Equal(t, 25, gslb.Spec.Strategy.Weight["eu"]) + assert.Equal(t, 60, gslb.Spec.Strategy.Weight["us"]) assert.Equal(t, 3, len(gslb.Spec.Strategy.Weight)) } @@ -115,7 +115,7 @@ func TestResolveSpecWithoutFields(t *testing.T) { assert.NoError(t, err) assert.Equal(t, predefinedStrategy.DNSTtlSeconds, gslb.Spec.Strategy.DNSTtlSeconds) assert.Equal(t, predefinedStrategy.SplitBrainThresholdSeconds, gslb.Spec.Strategy.SplitBrainThresholdSeconds) - assert.True(t, gslb.Spec.Strategy.Weight.IsEmpty()) + assert.True(t, len(gslb.Spec.Strategy.Weight) == 0) } func TestResolveSpecWithZeroSplitBrain(t *testing.T) { @@ -181,55 +181,41 @@ func TestResolveSpecWithType(t *testing.T) { } func TestResolveSpecWithWeightCornerCases(t *testing.T) { - type test struct { - strategy string - weight k8gbv1beta1.Weight - expectederr bool - empty bool - ignoreSumValidation bool - } - tests := []test{ - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "-20%"}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"eu": "-20"}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"za": "+20"}, false, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": " 20% "}, false, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "20%"}, false, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "20"}, false, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "100%"}, false, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "101"}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "101%"}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "0"}, false, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "0%"}, false, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": ""}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "%"}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us": "blah"}, true, false, true}, - {FailoverStrategy, map[string]k8gbv1beta1.Percentage{"us": "10%"}, true, false, true}, - {GeoStrategy, map[string]k8gbv1beta1.Percentage{"us": "0%"}, true, false, true}, - {GeoStrategy, nil, false, true, true}, - {RoundRobinStrategy, nil, false, true, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"": ""}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"": "20%"}, true, false, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{}, false, true, true}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us-1": "100%", "us-2": "0%"}, false, false, false}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us-1": "100%", "us-2": "100%"}, true, false, false}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us-1": "0%", "us-2": "0%"}, true, false, false}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us-1": "50 %", "eu": "20%", "za": "30"}, false, false, false}, - {RoundRobinStrategy, map[string]k8gbv1beta1.Percentage{"us-1": "50 %", "0": "20%", "-": "-"}, true, false, false}, + tests := []struct { + strategy string + weight map[string]int + err bool + empty bool + }{ + {RoundRobinStrategy, map[string]int{"us": -20}, true, false}, + {RoundRobinStrategy, map[string]int{"za": 2}, false, false}, + {RoundRobinStrategy, map[string]int{"us": 1000}, false, false}, + {RoundRobinStrategy, map[string]int{"us": 0}, false, false}, + + {GeoStrategy, map[string]int{"za": 20}, true, false}, + {GeoStrategy, map[string]int{"us": 0}, true, false}, + {GeoStrategy, map[string]int{}, false, true}, + {GeoStrategy, nil, false, true}, + + {RoundRobinStrategy, map[string]int{}, false, true}, + {RoundRobinStrategy, map[string]int{"us-1": 100, "us-2": 0}, false, false}, + {RoundRobinStrategy, map[string]int{"us-1": 100, "us-2": 100}, false, false}, + {RoundRobinStrategy, map[string]int{"us-1": 10000, "us-2": 100}, true, false}, + {RoundRobinStrategy, map[string]int{"us-1": 0, "us-2": 0}, false, false}, + {RoundRobinStrategy, map[string]int{"us-1": 50, "eu": 20, "za": 0}, false, false}, } // arrange cl, gslb := getTestContext("./testdata/filled_omitempty.yaml") resolver := NewDependencyResolver() // act - for _, tt := range tests { - t.Run(fmt.Sprintf("weight_%s_%s", tt.strategy, tt.weight), func(t *testing.T) { - gslb.Spec.Strategy.Weight = tt.weight - gslb.Spec.Strategy.Type = tt.strategy + for _, test := range tests { + t.Run(fmt.Sprintf("weight_%s_%v", test.strategy, test.weight), func(t *testing.T) { + gslb.Spec.Strategy.Weight = test.weight + gslb.Spec.Strategy.Type = test.strategy err := resolver.ResolveGslbSpec(context.TODO(), gslb, cl) // assert - assert.Equal(t, tt.empty, gslb.Spec.Strategy.Weight.IsEmpty()) - if !(tt.ignoreSumValidation && err != nil && strings.Contains(err.Error(), `the sum of the weights must be equal to 100`)) { - assert.True(t, (err != nil) == tt.expectederr) - } + assert.Equal(t, test.empty, len(gslb.Spec.Strategy.Weight) == 0) + assert.True(t, (err != nil) == test.err) }) } } diff --git a/controllers/depresolver/testdata/filled_omitempty.yaml b/controllers/depresolver/testdata/filled_omitempty.yaml index 55b45b75cb..4a406c5fe7 100644 --- a/controllers/depresolver/testdata/filled_omitempty.yaml +++ b/controllers/depresolver/testdata/filled_omitempty.yaml @@ -38,7 +38,7 @@ spec: splitBrainThresholdSeconds: 305 dnsTtlSeconds: 35 weight: - us: 60% - eu: 25% - za: 15% + us: 60 + eu: 25 + za: 15 diff --git a/controllers/dnsupdate.go b/controllers/dnsupdate.go index 3e5a1cb952..524726af6e 100644 --- a/controllers/dnsupdate.go +++ b/controllers/dnsupdate.go @@ -174,7 +174,7 @@ func (r *GslbReconciler) getLabels(gslb *k8gbv1beta1.Gslb, targets assistant.Tar continue } for i, ip := range t.IPs { - l := fmt.Sprintf("weight-%s-%v-%v", k, i, v.Int()) + l := fmt.Sprintf("weight-%s-%v-%v", k, i, v) labels[l] = ip } } diff --git a/controllers/weight_test.go b/controllers/gslb_controller_weight_test.go similarity index 82% rename from controllers/weight_test.go rename to controllers/gslb_controller_weight_test.go index a4bbcca55f..dc8cf42e32 100644 --- a/controllers/weight_test.go +++ b/controllers/gslb_controller_weight_test.go @@ -38,7 +38,7 @@ Generated by GoLic, for more details see: https://github.com/AbsaOSS/golic func TestWeight(t *testing.T) { // arrange type wrr struct { - weight string + weight int targets []string } var tests = []struct { @@ -51,9 +51,9 @@ func TestWeight(t *testing.T) { name: "eu35-us50-za15", injectWeights: true, data: map[string]wrr{ - "eu": {weight: "35%", targets: []string{"10.10.0.1", "10.10.0.2"}}, - "us": {weight: "50%", targets: []string{"10.0.0.1", "10.0.0.2"}}, - "za": {weight: "15%", targets: []string{"10.22.0.1", "10.22.0.2", "10.22.1.1"}}, + "eu": {weight: 35, targets: []string{"10.10.0.1", "10.10.0.2"}}, + "us": {weight: 50, targets: []string{"10.0.0.1", "10.0.0.2"}}, + "za": {weight: 15, targets: []string{"10.22.0.1", "10.22.0.2", "10.22.1.1"}}, }, expectedLabels: map[string]string{ "strategy": depresolver.RoundRobinStrategy, @@ -70,9 +70,9 @@ func TestWeight(t *testing.T) { name: "eu100-us0-za0", injectWeights: true, data: map[string]wrr{ - "eu": {weight: "100%", targets: []string{"10.10.0.1", "10.10.0.2"}}, - "us": {weight: "0%", targets: []string{"10.0.0.1", "10.0.0.2"}}, - "za": {weight: "0%", targets: []string{"10.22.0.1", "10.22.0.2", "10.22.1.1"}}, + "eu": {weight: 100, targets: []string{"10.10.0.1", "10.10.0.2"}}, + "us": {weight: 0, targets: []string{"10.0.0.1", "10.0.0.2"}}, + "za": {weight: 0, targets: []string{"10.22.0.1", "10.22.0.2", "10.22.1.1"}}, }, expectedLabels: map[string]string{ "strategy": depresolver.RoundRobinStrategy, @@ -89,9 +89,9 @@ func TestWeight(t *testing.T) { name: "weights-without-external-targets", injectWeights: true, data: map[string]wrr{ - "eu": {weight: "25%", targets: []string{}}, - "us": {weight: "75%", targets: []string{}}, - "za": {weight: "0%", targets: []string{}}, + "eu": {weight: 25, targets: []string{}}, + "us": {weight: 75, targets: []string{}}, + "za": {weight: 0, targets: []string{}}, }, expectedLabels: map[string]string{ "strategy": depresolver.RoundRobinStrategy, @@ -101,9 +101,9 @@ func TestWeight(t *testing.T) { name: "weights-without-external-targets", injectWeights: true, data: map[string]wrr{ - "eu": {weight: "25%", targets: []string{}}, - "us": {weight: "75%", targets: []string{}}, - "za": {weight: "0%", targets: []string{}}, + "eu": {weight: 25, targets: []string{}}, + "us": {weight: 75, targets: []string{}}, + "za": {weight: 0, targets: []string{}}, }, expectedLabels: map[string]string{ "strategy": depresolver.RoundRobinStrategy, @@ -121,9 +121,9 @@ func TestWeight(t *testing.T) { name: "no weights with external targets", injectWeights: false, data: map[string]wrr{ - "eu": {weight: "100%", targets: []string{"10.10.0.1", "10.10.0.2"}}, - "us": {weight: "0%", targets: []string{"10.0.0.1", "10.0.0.2"}}, - "za": {weight: "0%", targets: []string{"10.22.0.1", "10.22.0.2", "10.22.1.1"}}, + "eu": {weight: 100, targets: []string{"10.10.0.1", "10.10.0.2"}}, + "us": {weight: 0, targets: []string{"10.0.0.1", "10.0.0.2"}}, + "za": {weight: 0, targets: []string{"10.22.0.1", "10.22.0.2", "10.22.1.1"}}, }, expectedLabels: map[string]string{ "strategy": depresolver.RoundRobinStrategy, @@ -147,9 +147,9 @@ func TestWeight(t *testing.T) { if !test.injectWeights { return nil } - gslb.Spec.Strategy.Weight = make(map[string]k8gbv1beta1.Percentage, 0) + gslb.Spec.Strategy.Weight = make(map[string]int, 0) for k, w := range test.data { - gslb.Spec.Strategy.Weight[k] = k8gbv1beta1.Percentage(w.weight) + gslb.Spec.Strategy.Weight[k] = w.weight } return nil } diff --git a/terratest/examples/roundrobin_weight1.yaml b/terratest/examples/roundrobin_weight1.yaml index a71045f184..7a6d026dbc 100644 --- a/terratest/examples/roundrobin_weight1.yaml +++ b/terratest/examples/roundrobin_weight1.yaml @@ -40,5 +40,5 @@ spec: type: roundRobin # Use a round robin load balancing strategy, when deciding which downstream clusters to route clients too dnsTtlSeconds: 5 weight: - eu: 50% - us: 50% + eu: 5 + us: 5 diff --git a/terratest/test/k8gb_weight_test.go b/terratest/test/k8gb_weight_test.go index b786d4fd38..dbeb260bdc 100644 --- a/terratest/test/k8gb_weight_test.go +++ b/terratest/test/k8gb_weight_test.go @@ -83,8 +83,8 @@ func TestWeightsExistsInLocalDNSEndpoint(t *testing.T) { require.NoError(t, err, "missing endpoint", host) // check all labels are correct require.Equal(t, "roundRobin", ep.Labels["strategy"]) - require.NotEqual(t, ep.Labels["weight-eu-0-50"], ep.Labels["weight-eu-1-50"]) - require.NotEqual(t, ep.Labels["weight-us-0-50"], ep.Labels["weight-us-1-50"]) + require.NotEqual(t, ep.Labels["weight-eu-0-5"], ep.Labels["weight-eu-1-5"]) + require.NotEqual(t, ep.Labels["weight-us-0-5"], ep.Labels["weight-us-1-5"]) // check all targets are correct for _, v := range epExternalEU.Targets { require.True(t, Contains(v, ep.Targets))