From 5b01933aa3648e3fa1bf1dfa10f9c1d17d26bfeb Mon Sep 17 00:00:00 2001 From: Lionel Jouin Date: Thu, 9 Feb 2023 14:42:43 +0100 Subject: [PATCH] Fix target changes on LB If a target with an identifier was previously registered with IPs and is now changing its IPs (IP refresh, different target...), the LB can now handle that case by removing it completly and adding it again. the internal setTargets is now removing old targets before adding new ones. It also checks if IPs for an identifier have changed or not. --- pkg/loadbalancer/stream/loadbalancer.go | 35 +++++--- pkg/slice/compare.go | 40 +++++++++ pkg/slice/compare_test.go | 115 ++++++++++++++++++++++++ 3 files changed, 177 insertions(+), 13 deletions(-) create mode 100644 pkg/slice/compare.go create mode 100644 pkg/slice/compare_test.go diff --git a/pkg/loadbalancer/stream/loadbalancer.go b/pkg/loadbalancer/stream/loadbalancer.go index d5c0c2d7..1950d388 100644 --- a/pkg/loadbalancer/stream/loadbalancer.go +++ b/pkg/loadbalancer/stream/loadbalancer.go @@ -1,5 +1,5 @@ /* -Copyright (c) 2021-2022 Nordix Foundation +Copyright (c) 2021-2023 Nordix Foundation Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -33,6 +33,7 @@ import ( "github.com/nordix/meridio/pkg/log" "github.com/nordix/meridio/pkg/networking" "github.com/nordix/meridio/pkg/retry" + "github.com/nordix/meridio/pkg/slice" ) // LoadBalancer - @@ -340,31 +341,39 @@ func (lb *LoadBalancer) setTargets(targets []*nspAPI.Target) error { return nil } var errFinal error - toRemoveTargetsMap := make(map[int]struct{}) - for identifier := range lb.targets { - toRemoveTargetsMap[identifier] = struct{}{} - } - lb.logger.V(2).Info("setTargets", "targets", targets) - for _, target := range targets { // targets to add + newTargetsMap := make(map[int]types.Target) + for _, target := range targets { t, err := NewTarget(target, lb.netUtils) if err != nil { continue } - if lb.targetExists(t.GetIdentifier()) { - delete(toRemoveTargetsMap, t.GetIdentifier()) - } else { - err = lb.AddTarget(t) // todo: pending targets? + newTargetsMap[t.GetIdentifier()] = t + } + for identifier, target := range lb.targets { // targets to remove + newTarget, exists := newTargetsMap[identifier] + if !exists { + err := lb.RemoveTarget(identifier) if err != nil { errFinal = fmt.Errorf("%w; %v", errFinal, err) } + continue } - } - for identifier := range toRemoveTargetsMap { // targets to remove + if slice.ContainsSameStrings(target.GetIps(), newTarget.GetIps()) { // have the same IPs? + delete(newTargetsMap, identifier) + continue + } + // Have different IPs, so the target IPs have changed and need to be removed and re-added err := lb.RemoveTarget(identifier) if err != nil { errFinal = fmt.Errorf("%w; %v", errFinal, err) } } + for _, target := range newTargetsMap { // targets to add + err := lb.AddTarget(target) + if err != nil { + errFinal = fmt.Errorf("%w; %v", errFinal, err) + } + } return errFinal } diff --git a/pkg/slice/compare.go b/pkg/slice/compare.go new file mode 100644 index 00000000..df9d9f86 --- /dev/null +++ b/pkg/slice/compare.go @@ -0,0 +1,40 @@ +/* +Copyright (c) 2023 Nordix Foundation + +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 + +// ContainsSameStrings returns if 2 slices are containing the exactly the +// same strings, No matter the order. +func ContainsSameStrings(a []string, b []string) bool { + if len(a) != len(b) { + return false + } + aMap := map[string]int{} + for _, e := range a { + aMap[e]++ + } + for _, e := range b { + _, exists := aMap[e] + if !exists { + return false + } + aMap[e]-- + if aMap[e] == 0 { + delete(aMap, e) + } + } + return len(aMap) == 0 +} diff --git a/pkg/slice/compare_test.go b/pkg/slice/compare_test.go new file mode 100644 index 00000000..5c25e30c --- /dev/null +++ b/pkg/slice/compare_test.go @@ -0,0 +1,115 @@ +/* +Copyright (c) 2023 Nordix Foundation + +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_test + +import ( + "testing" + + "github.com/nordix/meridio/pkg/slice" +) + +func TestContainsSameStrings(t *testing.T) { + type args struct { + a []string + b []string + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "empty", + args: args{ + a: []string{}, + b: []string{}, + }, + want: true, + }, + { + name: "empty and nil", + args: args{ + a: []string{}, + b: nil, + }, + want: true, + }, + { + name: "a empty", + args: args{ + a: []string{"a"}, + b: []string{}, + }, + want: false, + }, + { + name: "b empty", + args: args{ + a: []string{"a"}, + b: []string{}, + }, + want: false, + }, + { + name: "equal with 1 element", + args: args{ + a: []string{"a"}, + b: []string{"a"}, + }, + want: true, + }, + { + name: "not equal with 1 element", + args: args{ + a: []string{"a"}, + b: []string{"b"}, + }, + want: false, + }, + { + name: "equal with 2 element same order", + args: args{ + a: []string{"a", "b"}, + b: []string{"a", "b"}, + }, + want: true, + }, + { + name: "equal with 2 element different order", + args: args{ + a: []string{"a", "b"}, + b: []string{"b", "a"}, + }, + want: true, + }, + { + name: "duplicates", + args: args{ + a: []string{"hello", "world", "hello"}, + b: []string{"hello", "hello", "world"}, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := slice.ContainsSameStrings(tt.args.a, tt.args.b); got != tt.want { + t.Errorf("ContainsSameStrings() = %v, want %v", got, tt.want) + } + }) + } +}