From 25c7e891144cae49c0f6994e56be757877cce108 Mon Sep 17 00:00:00 2001 From: Philipp Eberle Date: Fri, 26 Feb 2021 12:20:01 +0100 Subject: [PATCH 1/5] Refactor the implementation of nextFreePort --- pkg/lbmanager/lbmanager.go | 46 +++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/pkg/lbmanager/lbmanager.go b/pkg/lbmanager/lbmanager.go index 7741df19..c9dc5338 100644 --- a/pkg/lbmanager/lbmanager.go +++ b/pkg/lbmanager/lbmanager.go @@ -11,13 +11,15 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +// LBManager Responsible for the creation and deletion of externally accessible Services to access the Postgresql clusters managed by the Postgreslet. type LBManager struct { - client.Client // todo: service cluster - LBIP string // todo: via configmap - PortRangeStart int32 // todo: via configmap + client.Client + LBIP string + PortRangeStart int32 PortRangeSize int32 } +// New Creates a new LBManager with the given configuration func New(client client.Client, lbIP string, portRangeStart, portRangeSize int32) *LBManager { return &LBManager{ Client: client, @@ -27,6 +29,7 @@ func New(client client.Client, lbIP string, portRangeStart, portRangeSize int32) } } +// CreateSvcLBIfNone Creates a new Service of type LoadBalancer for the given Postgres resource if neccessary func (m *LBManager) CreateSvcLBIfNone(ctx context.Context, in *api.Postgres) error { if err := m.Get(ctx, client.ObjectKey{ Namespace: in.ToPeripheralResourceNamespace(), @@ -60,6 +63,7 @@ func (m *LBManager) CreateSvcLBIfNone(ctx context.Context, in *api.Postgres) err return nil } +// DeleteSvcLB Deletes the corresponding Service of type LoadBalancer of the given Postgres resource. func (m *LBManager) DeleteSvcLB(ctx context.Context, in *api.Postgres) error { lb := &corev1.Service{} lb.Namespace = in.ToPeripheralResourceNamespace() @@ -70,37 +74,59 @@ func (m *LBManager) DeleteSvcLB(ctx context.Context, in *api.Postgres) error { return nil } +// nextFreeSocket finds any existing LoadBalancerIP and the next free port out of the configure port range. func (m *LBManager) nextFreeSocket(ctx context.Context) (string, int32, error) { // TODO prevent concurrency issues when calculating port / ip. existingLBIP := "" + // Fetch all services managed by this postgreslet lbs := &corev1.ServiceList{} if err := m.List(ctx, lbs, client.MatchingLabels(api.SvcLoadBalancerLabel)); err != nil { return existingLBIP, 0, fmt.Errorf("failed to fetch the list of services of type LoadBalancer: %w", err) } + // If there are none, this will be the first (managed) service we create, so start with PortRangeStart and return if len(lbs.Items) == 0 { return existingLBIP, m.PortRangeStart, nil } - // Record weather any port is occupied - isOccupied := make([]bool, int(m.PortRangeSize)) + // If there are already any managed services, store all the used ports in a slice. + // Also store the LoadBalancerIP. + portsInUse := make([]int32, len(lbs.Items)) for i := range lbs.Items { svc := lbs.Items[i] if len(svc.Spec.Ports) > 0 { - isOccupied[svc.Spec.Ports[0].Port-m.PortRangeStart] = true + portsInUse = append(portsInUse, svc.Spec.Ports[0].Port) } if svc.Spec.LoadBalancerIP != "" { + // Technically, we only store the IP of the last Service in this list. + // As there should only be one IP per postgreslet and one postgreslet per cluster, this is good enough. existingLBIP = svc.Spec.LoadBalancerIP } } - for i := range isOccupied { - if !isOccupied[i] { - return existingLBIP, m.PortRangeStart + int32(i), nil + // Now try all ports in the configured port range to find a free one. + // While not as effective as other implementations, this allows us to freely change PortRangeStart and PortRangeSize + // retroactively without breaking the implementation. + for port := m.PortRangeStart; port < m.PortRangeStart+m.PortRangeSize; port++ { + if containsElem(portsInUse, port) { + // Port already in use, try the next one + continue } + // The postgreslet hasn't assigned this port yet, so use it. + return existingLBIP, port, nil } - return existingLBIP, 0, errors.New("no free port") + // If we made it this far, no free port could be found. + return existingLBIP, 0, errors.New("no free port in the configured port range found") +} + +func containsElem(s []int32, v int32) bool { + for _, elem := range s { + if elem == v { + return true + } + } + return false } From 52a1f47359e7deb607080fcf43474a1e67aea0d8 Mon Sep 17 00:00:00 2001 From: Philipp Eberle Date: Fri, 26 Feb 2021 12:35:04 +0100 Subject: [PATCH 2/5] Further refactoring --- pkg/lbmanager/lbmanager.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/lbmanager/lbmanager.go b/pkg/lbmanager/lbmanager.go index c9dc5338..52202272 100644 --- a/pkg/lbmanager/lbmanager.go +++ b/pkg/lbmanager/lbmanager.go @@ -78,17 +78,17 @@ func (m *LBManager) DeleteSvcLB(ctx context.Context, in *api.Postgres) error { func (m *LBManager) nextFreeSocket(ctx context.Context) (string, int32, error) { // TODO prevent concurrency issues when calculating port / ip. - existingLBIP := "" + anyExistingLBIP := "" // Fetch all services managed by this postgreslet lbs := &corev1.ServiceList{} if err := m.List(ctx, lbs, client.MatchingLabels(api.SvcLoadBalancerLabel)); err != nil { - return existingLBIP, 0, fmt.Errorf("failed to fetch the list of services of type LoadBalancer: %w", err) + return anyExistingLBIP, 0, fmt.Errorf("failed to fetch the list of services of type LoadBalancer: %w", err) } // If there are none, this will be the first (managed) service we create, so start with PortRangeStart and return if len(lbs.Items) == 0 { - return existingLBIP, m.PortRangeStart, nil + return anyExistingLBIP, m.PortRangeStart, nil } // If there are already any managed services, store all the used ports in a slice. @@ -102,7 +102,7 @@ func (m *LBManager) nextFreeSocket(ctx context.Context) (string, int32, error) { if svc.Spec.LoadBalancerIP != "" { // Technically, we only store the IP of the last Service in this list. // As there should only be one IP per postgreslet and one postgreslet per cluster, this is good enough. - existingLBIP = svc.Spec.LoadBalancerIP + anyExistingLBIP = svc.Spec.LoadBalancerIP } } @@ -115,11 +115,11 @@ func (m *LBManager) nextFreeSocket(ctx context.Context) (string, int32, error) { continue } // The postgreslet hasn't assigned this port yet, so use it. - return existingLBIP, port, nil + return anyExistingLBIP, port, nil } // If we made it this far, no free port could be found. - return existingLBIP, 0, errors.New("no free port in the configured port range found") + return anyExistingLBIP, 0, errors.New("no free port in the configured port range found") } func containsElem(s []int32, v int32) bool { From d36fe7d62edc4015c21dc820e59aa5df4c608f8d Mon Sep 17 00:00:00 2001 From: "Lim, Kian-An" Date: Mon, 29 Mar 2021 17:17:57 +0200 Subject: [PATCH 3/5] Add tests --- pkg/lbmanager/lbmanager.go | 37 +++-------- pkg/lbmanager/lbmanager_test.go | 110 ++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 28 deletions(-) create mode 100644 pkg/lbmanager/lbmanager_test.go diff --git a/pkg/lbmanager/lbmanager.go b/pkg/lbmanager/lbmanager.go index 52202272..fd450d93 100644 --- a/pkg/lbmanager/lbmanager.go +++ b/pkg/lbmanager/lbmanager.go @@ -39,23 +39,12 @@ func (m *LBManager) CreateSvcLBIfNone(ctx context.Context, in *api.Postgres) err return fmt.Errorf("failed to fetch Service of type LoadBalancer: %w", err) } - existingLBIP, nextFreePort, err := m.nextFreeSocket(ctx) + nextFreePort, err := m.nextFreePort(ctx) if err != nil { - return fmt.Errorf("failed to get a free port for creating Service of type LoadBalancer: %w", err) - } - var lbIPToUse string - if m.LBIP != "" { - // a specific IP was configured in the config, so use that one - lbIPToUse = m.LBIP - } else if existingLBIP != "" { - // no ip was configured, but one is already in use, so use the existing one - lbIPToUse = existingLBIP - } else { - // nothing was configured, nothing exists yet, so use an empty address so a new loadbalancer will be created and assigned - lbIPToUse = "" + return fmt.Errorf("failed to get the next free port: %w", err) } - if err := m.Create(ctx, in.ToSvcLB(lbIPToUse, nextFreePort)); err != nil { + if err := m.Create(ctx, in.ToSvcLB(m.LBIP, nextFreePort)); err != nil { return fmt.Errorf("failed to create Service of type LoadBalancer: %w", err) } return nil @@ -75,35 +64,27 @@ func (m *LBManager) DeleteSvcLB(ctx context.Context, in *api.Postgres) error { } // nextFreeSocket finds any existing LoadBalancerIP and the next free port out of the configure port range. -func (m *LBManager) nextFreeSocket(ctx context.Context) (string, int32, error) { +func (m *LBManager) nextFreePort(ctx context.Context) (int32, error) { // TODO prevent concurrency issues when calculating port / ip. - anyExistingLBIP := "" - // Fetch all services managed by this postgreslet lbs := &corev1.ServiceList{} if err := m.List(ctx, lbs, client.MatchingLabels(api.SvcLoadBalancerLabel)); err != nil { - return anyExistingLBIP, 0, fmt.Errorf("failed to fetch the list of services of type LoadBalancer: %w", err) + return 0, fmt.Errorf("failed to fetch the list of services of type LoadBalancer: %w", err) } // If there are none, this will be the first (managed) service we create, so start with PortRangeStart and return if len(lbs.Items) == 0 { - return anyExistingLBIP, m.PortRangeStart, nil + return m.PortRangeStart, nil } // If there are already any managed services, store all the used ports in a slice. - // Also store the LoadBalancerIP. - portsInUse := make([]int32, len(lbs.Items)) + portsInUse := []int32{} for i := range lbs.Items { svc := lbs.Items[i] if len(svc.Spec.Ports) > 0 { portsInUse = append(portsInUse, svc.Spec.Ports[0].Port) } - if svc.Spec.LoadBalancerIP != "" { - // Technically, we only store the IP of the last Service in this list. - // As there should only be one IP per postgreslet and one postgreslet per cluster, this is good enough. - anyExistingLBIP = svc.Spec.LoadBalancerIP - } } // Now try all ports in the configured port range to find a free one. @@ -115,11 +96,11 @@ func (m *LBManager) nextFreeSocket(ctx context.Context) (string, int32, error) { continue } // The postgreslet hasn't assigned this port yet, so use it. - return anyExistingLBIP, port, nil + return port, nil } // If we made it this far, no free port could be found. - return anyExistingLBIP, 0, errors.New("no free port in the configured port range found") + return 0, errors.New("no free port in the configured port range found") } func containsElem(s []int32, v int32) bool { diff --git a/pkg/lbmanager/lbmanager_test.go b/pkg/lbmanager/lbmanager_test.go new file mode 100644 index 00000000..68825a35 --- /dev/null +++ b/pkg/lbmanager/lbmanager_test.go @@ -0,0 +1,110 @@ +package lbmanager + +import ( + "context" + "fmt" + "testing" + + api "github.com/fi-ts/postgreslet/api/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestLBManager_nextFreePort(t *testing.T) { + portRangeStart := int32(0) + portRangeSize := int32(5) + + tests := []struct { + name string + lbMgr *LBManager + want int32 + wantErr bool + }{ + { + name: "no svc in the cluster", + lbMgr: &LBManager{ + Client: fake.NewClientBuilder().WithScheme(scheme()).WithLists(svcListWithPorts()).Build(), + LBIP: "0.0.0.0", + PortRangeStart: portRangeStart, + PortRangeSize: portRangeSize, + }, + want: 0, + wantErr: false, + }, + { + name: "one svc already in the cluster", + lbMgr: &LBManager{ + Client: fake.NewClientBuilder().WithScheme(scheme()).WithLists(svcListWithPorts(0)).Build(), + LBIP: "0.0.0.0", + PortRangeStart: portRangeStart, + PortRangeSize: portRangeSize, + }, + want: 1, + wantErr: false, + }, + { + name: "last free port left", + lbMgr: &LBManager{ + Client: fake.NewClientBuilder().WithScheme(scheme()).WithLists(svcListWithPorts(0, 1, 2, 3)).Build(), + LBIP: "0.0.0.0", + PortRangeStart: portRangeStart, + PortRangeSize: portRangeSize, + }, + want: 4, + wantErr: false, + }, + { + name: "no free port", + lbMgr: &LBManager{ + Client: fake.NewClientBuilder().WithScheme(scheme()).WithLists(svcListWithPorts(0, 1, 2, 3, 4)).Build(), + LBIP: "0.0.0.0", + PortRangeStart: portRangeStart, + PortRangeSize: portRangeSize, + }, + want: 0, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := tt.lbMgr.nextFreePort(context.Background()) + if (err != nil) != tt.wantErr { + t.Errorf("LBManager.nextFreePort() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("LBManager.nextFreePort() = %v, want %v", got, tt.want) + } + }) + } +} + +func scheme() *runtime.Scheme { + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + + return scheme +} + +// svcListWithPorts generates a `ServiceList` containing `Service`s with ports respectively +func svcListWithPorts(ports ...int32) *corev1.ServiceList { + svcList := &corev1.ServiceList{} + for _, port := range ports { + svcList.Items = append(svcList.Items, *svcWithPort(port)) + } + return svcList +} + +func svcWithPort(port int32) *corev1.Service { + svc := corev1.Service{} + svc.Name = fmt.Sprintf("svc-with-port-%d", port) + svc.Labels = api.SvcLoadBalancerLabel + svc.Spec.Ports = []corev1.ServicePort{ + { + Port: port, + }, + } + return &svc +} From 54af2e45036e2ca0d268c71cdeba8d4db2bdf088 Mon Sep 17 00:00:00 2001 From: Philipp Eberle Date: Tue, 6 Apr 2021 09:54:44 +0200 Subject: [PATCH 4/5] Add additional test --- pkg/lbmanager/lbmanager_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/lbmanager/lbmanager_test.go b/pkg/lbmanager/lbmanager_test.go index 68825a35..e93a2ae8 100644 --- a/pkg/lbmanager/lbmanager_test.go +++ b/pkg/lbmanager/lbmanager_test.go @@ -65,6 +65,17 @@ func TestLBManager_nextFreePort(t *testing.T) { want: 0, wantErr: true, }, + { + name: "re-use releaased port", + lbMgr: &LBManager{ + Client: fake.NewClientBuilder().WithScheme(scheme()).WithLists(svcListWithPorts(0, 2, 4)).Build(), + LBIP: "0.0.0.0", + PortRangeStart: portRangeStart, + PortRangeSize: portRangeSize, + }, + want: 1, + wantErr: false, + }, } for _, tt := range tests { From 65c2f4bb4493bda20adb5b749fa28c2304a731d7 Mon Sep 17 00:00:00 2001 From: Philipp Eberle Date: Tue, 6 Apr 2021 10:58:13 +0200 Subject: [PATCH 5/5] Small tweak --- pkg/lbmanager/lbmanager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/lbmanager/lbmanager_test.go b/pkg/lbmanager/lbmanager_test.go index e93a2ae8..3ffe3ad2 100644 --- a/pkg/lbmanager/lbmanager_test.go +++ b/pkg/lbmanager/lbmanager_test.go @@ -68,7 +68,7 @@ func TestLBManager_nextFreePort(t *testing.T) { { name: "re-use releaased port", lbMgr: &LBManager{ - Client: fake.NewClientBuilder().WithScheme(scheme()).WithLists(svcListWithPorts(0, 2, 4)).Build(), + Client: fake.NewClientBuilder().WithScheme(scheme()).WithLists(svcListWithPorts(0, 2, 3)).Build(), LBIP: "0.0.0.0", PortRangeStart: portRangeStart, PortRangeSize: portRangeSize,