Fix target changes on LB #363
Conversation
| } | ||
| } | ||
| for identifier := range toRemoveTargetsMap { // targets to remove | ||
| if slice.ContainsSameStrings(target.GetIps(), newTarget.GetIps()) { // have the same IPs? |
There was a problem hiding this comment.
This is a set-operation. The right way is to add a set package, not a slice package. Here are generic sets as implemented in K8s
https://github.com/kubernetes/apimachinery/blob/v0.26.1/pkg/util/sets/set.go#L24
target.GetIps() should really return a set (set[string]). This would make the code more maintainable. This may be a large update, but to implement (steal) a set package may be a good first step.
There was a problem hiding this comment.
There was a problem hiding this comment.
I used this sets implementation from apimachinery as you mentioned, but for now I kept the usage in the function, the target struct will need a refactoring later.
There was a problem hiding this comment.
On second thought, since we are using slices (where a set would be more appropriate), it may be ok to have a slice package, but a generic one, with a function like.
package main
import (
"fmt"
"golang.org/x/exp/slices"
"golang.org/x/exp/constraints"
)
func main() {
s1 := []string{"A", "B", "C", "D"}
s2 := []string{"D", "C", "B", "A"}
fmt.Println(SameContents(s1, s2))
var n1, n2 []string // nil should work
fmt.Println(SameContents(n1, n2))
}
// SameContents return true if the slices contains the same elements in any order.
// On exit the slices may be sorted.
func SameContents[E constraints.Ordered](s1, s2 []E) bool {
if len(s1) != len(s2) {
return false
}
slices.Sort(s1)
slices.Sort(s2)
return slices.Compare(s1, s2) == 0
}This has O(N log N) and sorts in-place, but since the assumed lists are small and unordered, the gain in maintainability is more important IMHO (e.g. no unit-test is necessary)
In a wonderful future, data that are sets (like a collection of addresses) will be sets.
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.
LionelJouin
force-pushed
the
target-handling
branch
from
5b01933
to
829f143
Compare
|
/reverify |
|
Please see #363 (comment). Sorry for the fuss, but I think it's better to replace slices that are sets with sets in the future and use set-operations, but for now since we use slices maybe we should stick with them. But slice functions should be generic when it's possible. |
Description
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.
Issue link
#356
Checklist