From 2bd0964b32291a8f15396c4a1032734d714cf0f8 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Sun, 20 Oct 2024 17:05:37 -0500 Subject: [PATCH 1/7] fix(router-id): default to the primary IPv4 addr Somehow during refactoring this default got lost and we stopped defaulting to the primary IPv4 address when a router ID was not passed. --- pkg/bgp/id.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/bgp/id.go b/pkg/bgp/id.go index 3396fe714..6c9a82a51 100644 --- a/pkg/bgp/id.go +++ b/pkg/bgp/id.go @@ -37,7 +37,7 @@ func GenerateRouterID(nodeIPAware utils.NodeIPAware, configRouterID string) (str if nodeIPAware.GetPrimaryNodeIP().To4() == nil { return "", errors.New("router-id must be specified when primary node IP is an IPv6 address") } - return configRouterID, nil + return nodeIPAware.GetPrimaryNodeIP().String(), nil } // ValidateCommunity takes in a string and attempts to parse a BGP community out of it in a way that is similar to From 7f478c8445fe612c4c2375c30363ab1776e4e091 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Wed, 9 Oct 2024 17:52:02 -0500 Subject: [PATCH 2/7] feat(Makefile): run goimports on gofmt-fix --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index 48956cf4b..8ee3cd5dd 100644 --- a/Makefile +++ b/Makefile @@ -191,6 +191,7 @@ gofmt: ## Tells you what files need to be gofmt'd. gofmt-fix: ## Fixes files that need to be gofmt'd. gofmt -s -w $(shell find . -not \( \( -wholename '*/vendor/*' \) -prune \) -name '*.go') + goimports -w $(shell find . -not \( \( -wholename '*/vendor/*' \) -prune \) -name '*.go') # List of all file_moq.go files which would need to be regenerated # from file.go if changed From 97263920de476f67546a7e82aa3ac8f717f6a2f9 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Fri, 11 Oct 2024 14:20:28 -0500 Subject: [PATCH 3/7] fix(lb): remove race condition from tests --- pkg/controllers/lballoc/lballoc.go | 12 ++++++++++++ pkg/controllers/lballoc/lballoc_test.go | 5 ++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/pkg/controllers/lballoc/lballoc.go b/pkg/controllers/lballoc/lballoc.go index 7ae2fdbbf..12bd1a256 100644 --- a/pkg/controllers/lballoc/lballoc.go +++ b/pkg/controllers/lballoc/lballoc.go @@ -38,6 +38,7 @@ type LoadBalancerController struct { clientset kubernetes.Interface isDefault bool syncPeriod time.Duration + unitTestWG *sync.WaitGroup } func getNamespace() (namespace string, err error) { @@ -346,6 +347,12 @@ func appendIngressIP(svc *v1core.Service, ip net.IP) { } func (lbc *LoadBalancerController) updateService(svc *v1core.Service, ips ...net.IP) { + // This is only non-nil during certain unit tests that need to understand when this goroutine is finished to remove + // chance of race conditions + if lbc.unitTestWG != nil { + defer lbc.unitTestWG.Done() + } + if lbc.clientset == nil { panic("clientset") } @@ -399,6 +406,11 @@ func (lbc *LoadBalancerController) allocateService(svc *v1core.Service) error { return errors.New("unable to allocate dual-stack addresses: " + err.Error()) } + // This is only non-nil during certain unit tests that need to understand when this goroutine is finished to remove + // chance of race conditions + if lbc.unitTestWG != nil { + lbc.unitTestWG.Add(1) + } go lbc.updateService(svc, ipv4, ipv6) return nil } diff --git a/pkg/controllers/lballoc/lballoc_test.go b/pkg/controllers/lballoc/lballoc_test.go index ef2a82345..c8547e9c7 100644 --- a/pkg/controllers/lballoc/lballoc_test.go +++ b/pkg/controllers/lballoc/lballoc_test.go @@ -3,6 +3,7 @@ package lballoc import ( "errors" "net" + "sync" "testing" "time" @@ -572,7 +573,8 @@ func TestAppendIngressIP(t *testing.T) { func TestAllocateService(t *testing.T) { mlbc := &LoadBalancerController{ - clientset: fake.NewSimpleClientset(), + clientset: fake.NewSimpleClientset(), + unitTestWG: &sync.WaitGroup{}, } ir4, ir6 := makeIPRanges("127.127.127.127/30", "ffff::/80") mlbc.ipv4Ranges = ir4 @@ -586,6 +588,7 @@ func TestAllocateService(t *testing.T) { t.Fatalf("expected %v, got %s", nil, err) } + mlbc.unitTestWG.Wait() svc = makeTestService() mlbc.ipv4Ranges = newipRanges(nil) fp := v1core.IPFamilyPolicyRequireDualStack From 06e573b6dc13fb798b1f61c0cfd692463e4c40c1 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Fri, 11 Oct 2024 15:59:30 -0500 Subject: [PATCH 4/7] test: reduce unit test run time --- pkg/controllers/routing/ecmp_vip_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controllers/routing/ecmp_vip_test.go b/pkg/controllers/routing/ecmp_vip_test.go index 9ea00e1dc..5fa53bb60 100644 --- a/pkg/controllers/routing/ecmp_vip_test.go +++ b/pkg/controllers/routing/ecmp_vip_test.go @@ -903,6 +903,7 @@ func Test_getVIPsForService(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { + t.Parallel() for _, serviceAdvertisedIP := range test.serviceAdvertisedIPs { endpoints := serviceAdvertisedIP.endpoints clientset := fake.NewSimpleClientset() From 384564c38f9d80002c268a43e6bde16195416450 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Fri, 11 Oct 2024 15:23:47 -0500 Subject: [PATCH 5/7] test(unit_test_timing.py): parse unit test timing Adds a Python script that is useful for seeing where very slow unit tests can be found and fixed. --- build/test-scripts/unit_test_timing.py | 34 ++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100755 build/test-scripts/unit_test_timing.py diff --git a/build/test-scripts/unit_test_timing.py b/build/test-scripts/unit_test_timing.py new file mode 100755 index 000000000..db2544636 --- /dev/null +++ b/build/test-scripts/unit_test_timing.py @@ -0,0 +1,34 @@ +#!/usr/bin/env python + +# Taken from: https://rotational.io/blog/speeding-up-go-tests/ this python script assists in parsing Golang JSON unit +# tests and sorting them by the amount of time taken +# +# To use, run Go unit tests via the following: +# go test -v -json -count 1 github.com/cloudnativelabs/kube-router/v2/cmd/kube-router/ github.com/cloudnativelabs/kube-router/v2/pkg/... >testing_output.json +# +# Then run this script via: +# build/test-scripts/unit_test_timing.py testing_output.json + +import json +import sys + +if __name__ == "__main__": + tests = [] + + with open(sys.argv[1], 'r') as f: + for line in f: + data = json.loads(line) + if data['Action'] != 'pass': + continue + + if 'Test' not in data: + continue + + if data['Elapsed'] < 0.1: + continue + + tests.append(data) + + tests.sort(key=lambda d: d['Elapsed'], reverse=True) + for t in tests: + print(f"{t['Elapsed']:0.3f}s\t{t['Package']} {t['Test']}") From 7127360245c5ee6d056daddb63ec6c39de232607 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Fri, 11 Oct 2024 15:55:41 -0500 Subject: [PATCH 6/7] fix(node): make ordering more specific We should be returning IP addresses in specific orders based upon what type of IP they are. --- pkg/utils/node.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/pkg/utils/node.go b/pkg/utils/node.go index b580de137..87da1dd36 100644 --- a/pkg/utils/node.go +++ b/pkg/utils/node.go @@ -85,18 +85,16 @@ type NodeAware interface { // GetNodeIPv4Addrs returns the node's IPv4 addresses as defined by the Kubernetes Node Object. func (n *KRNode) GetNodeIPv4Addrs() []net.IP { var nodeIPs []net.IP - for _, ip := range n.NodeIPv4Addrs { - nodeIPs = append(nodeIPs, ip...) - } + nodeIPs = append(nodeIPs, n.NodeIPv4Addrs[apiv1.NodeInternalIP]...) + nodeIPs = append(nodeIPs, n.NodeIPv4Addrs[apiv1.NodeExternalIP]...) return nodeIPs } // GetNodeIPv6Addrs returns the node's IPv6 addresses as defined by the Kubernetes Node Object. func (n *KRNode) GetNodeIPv6Addrs() []net.IP { var nodeIPs []net.IP - for _, ip := range n.NodeIPv6Addrs { - nodeIPs = append(nodeIPs, ip...) - } + nodeIPs = append(nodeIPs, n.NodeIPv6Addrs[apiv1.NodeInternalIP]...) + nodeIPs = append(nodeIPs, n.NodeIPv6Addrs[apiv1.NodeExternalIP]...) return nodeIPs } @@ -196,12 +194,10 @@ func (n *LocalKRNode) GetNodeMTU() (int, error) { // Node Object. func (n *KRNode) GetNodeIPAddrs() []net.IP { var nodeIPs []net.IP - for _, ip := range n.NodeIPv4Addrs { - nodeIPs = append(nodeIPs, ip...) - } - for _, ip := range n.NodeIPv6Addrs { - nodeIPs = append(nodeIPs, ip...) - } + ipv4IPs := n.GetNodeIPv4Addrs() + nodeIPs = append(nodeIPs, ipv4IPs...) + ipv6IPs := n.GetNodeIPv6Addrs() + nodeIPs = append(nodeIPs, ipv6IPs...) return nodeIPs } From f1814c6c912d08abb73eb7d9908da85236b5afb3 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Sun, 20 Oct 2024 17:15:56 -0500 Subject: [PATCH 7/7] fix(linux_networking_moq.go): import def order --- pkg/controllers/proxy/linux_networking_moq.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/controllers/proxy/linux_networking_moq.go b/pkg/controllers/proxy/linux_networking_moq.go index 940044fc4..8a13c38c9 100644 --- a/pkg/controllers/proxy/linux_networking_moq.go +++ b/pkg/controllers/proxy/linux_networking_moq.go @@ -4,11 +4,12 @@ package proxy import ( + "net" + "sync" + "github.com/moby/ipvs" "github.com/vishvananda/netlink" "github.com/vishvananda/netns" - "net" - "sync" ) // Ensure, that LinuxNetworkingMock does implement LinuxNetworking.