Support allocating shared LB for services #1

StrongMonkey · 2023-10-31T10:18:21Z

This PR adds ability to configure services to share load balancers for different listeners. The purpose of this is to reduce cost when bring NLB for service loadbalancer, while one service will provision one dedicated NLB.

There are two major changes for the implemetation:

A controller that watches services, and allocate service to a "stack". A "stack" contains virtual resources like NLB, listeners.
Support multiple services sharing "stack". This is done through global in-memory cache where multiple services can be allocated to one virtual stack, with shared LB and different listener for ports in order to share resources.

controllers/service/service_controller.go

StrongMonkey · 2023-10-31T10:30:28Z

controllers/service/service_controller.go

 		}
+		if svc.Annotations[service.LoadBalancerAllocatingPortKey] == "true" {


When service is in "shared" mode, it will use the nodePort as the listener. This avoids the need to allocate a dedicated port for service, as nodePort is already unique across cluster that is allocated by k8s server.

StrongMonkey · 2023-10-31T10:31:09Z

main.go

-	elbv2webhook.NewTargetGroupBindingMutator(cloud.ELBV2(), ctrl.Log).SetupWithManager(mgr)
-	elbv2webhook.NewTargetGroupBindingValidator(mgr.GetClient(), cloud.ELBV2(), ctrl.Log).SetupWithManager(mgr)
-	networkingwebhook.NewIngressValidator(mgr.GetClient(), controllerCFG.IngressConfig, ctrl.Log).SetupWithManager(mgr)
+	//podReadinessGateInjector := inject.NewPodReadinessGate(controllerCFG.PodWebhookConfig,


Need to uncomment this, used for local development.

StrongMonkey · 2023-10-31T10:33:49Z

pkg/service/model_builder.go

 }

 func (b *defaultModelBuilder) Build(ctx context.Context, service *corev1.Service) (core.Stack, *elbv2model.LoadBalancer, bool, error) {
-	stack := core.NewDefaultStack(core.StackID(k8s.NamespacedName(service)))
+	// Initialize the global cache if not initialized
+	if !b.initialized {


This makes sure that when services are in shared "stack", it reused the global cache that contains the stack. So

Initialize the global cache when controller is started.

Always looks for existin stack in cache when deploying. This is so that two services that in shared stack will be deployed together by sharing the same LB.

StrongMonkey · 2023-10-31T10:34:05Z

pkg/service/model_builder.go

@@ -221,8 +283,49 @@ func (t *defaultModelBuildTask) run(ctx context.Context) error {
 				return errors.Errorf("deletion_protection is enabled, cannot delete the service: %v", t.service.Name)
 			}
 		}
+		// When service is deleted, update resources in the stack to make sure things are cleaned up properly.
+		for _, port := range t.service.Spec.Ports {


This also cleans up resources when service is deleted.

thedadams · 2023-11-01T11:22:11Z

controllers/service/service_controller.go

+		}
+		for _, svc := range serviceList.Items {
+			if svc.Annotations[service.LoadBalancerStackKey] != "" {
+				r.allocatedServices[svc.Annotations[service.LoadBalancerStackKey]] = map[int]bool{}


nit: if you only want to track existence, this is more efficient.

Suggested change

r.allocatedServices[svc.Annotations[service.LoadBalancerStackKey]] = map[int]bool{}

r.allocatedServices[svc.Annotations[service.LoadBalancerStackKey]] = map[int]struct{}{}

You could also make the maps map[int32]struct{}{} to avoid the int(port.NodePort) everywhere.

👍 addressed

controllers/service/service_controller.go

thedadams · 2023-11-01T11:32:53Z

controllers/service/service_controller.go

+		r.lock.Lock()
+		for _, port := range svc.Spec.Ports {
+			r.allocatedServices[stackName] = map[int]bool{
+				int(port.Port): true,


This is the only place you use port.Port instead of port.NodePort. Just wanted to make sure that is intentional.

yeah mistake. Should be nodePort 👍

controllers/service/service_controller.go

pkg/model/core/stack.go

thedadams · 2023-11-01T11:45:50Z

pkg/service/model_build_load_balancer.go

@@ -487,6 +501,10 @@ func (t *defaultModelBuildTask) buildLoadBalancerName(_ context.Context, scheme
 	_, _ = uuidHash.Write([]byte(scheme))
 	uuid := hex.EncodeToString(uuidHash.Sum(nil))

+	if t.service.Annotations[LoadBalancerStackKey] != "" {
+		return fmt.Sprintf("k8s-%.8s", t.service.Annotations[LoadBalancerStackKey]), nil


This format is used in at least a couple different places. Can it be made a function?

thedadams · 2023-11-01T11:49:53Z

pkg/service/model_builder.go

+	if !b.initialized {
+		// if not initialized, we need to build the global cache based on existing services
+		var serviceList corev1.ServiceList
+		if err := b.client.List(context.Background(), &serviceList); err != nil {


Can this be

Suggested change

if err := b.client.List(context.Background(), &serviceList); err != nil {

if err := b.client.List(ctx, &serviceList); err != nil {

thedadams · 2023-11-01T11:50:41Z

pkg/service/model_builder.go

+	var stack core.Stack
+	stack = core.NewDefaultStack(stackID)
+	if service.Annotations[LoadBalancerAllocatingPortKey] == "true" {
+		// service will be allocated to a stack. The external controller will be


Seems like an incomplete comment.

pkg/service/model_builder.go

controllers/service/service_controller.go

Signed-off-by: Daishan Peng <[email protected]>

controllers/service/service_controller.go

thedadams · 2023-11-02T19:39:14Z

pkg/service/model_builder.go

+				if b.stackGlobalCache[stackID] == nil {
+					b.lock.Lock()
+					b.stackGlobalCache[stackID] = core.NewDefaultStack(stackID)
+					b.lock.Unlock()
+				}
+				b.lock.Lock()
+				b.stackGlobalCache[stackID].AddService(&svc)
+				b.lock.Unlock()


The read should also be protected here.

Suggested change

if b.stackGlobalCache[stackID] == nil {

b.lock.Lock()

b.stackGlobalCache[stackID] = core.NewDefaultStack(stackID)

b.lock.Unlock()

}

b.lock.Lock()

b.stackGlobalCache[stackID].AddService(&svc)

b.lock.Unlock()

b.lock.Lock()

if b.stackGlobalCache[stackID] == nil {

b.stackGlobalCache[stackID] = core.NewDefaultStack(stackID)

}

b.stackGlobalCache[stackID].AddService(&svc)

b.lock.Unlock()

Actually, is the lock here even necessary? You have the other lock and that should take care of this, too, right?

Yeah I will change to lock the whole snippet. It is necessary since another lock onlys locks per stack, not globally.

pkg/service/model_builder.go

Signed-off-by: Daishan Peng <[email protected]>

StrongMonkey commented Oct 31, 2023

View reviewed changes

controllers/service/service_controller.go Show resolved Hide resolved

StrongMonkey commented Oct 31, 2023

View reviewed changes

StrongMonkey requested review from thedadams, g-linville, njhale, keyallis and tylerslaton October 31, 2023 17:08

thedadams reviewed Nov 1, 2023

View reviewed changes

StrongMonkey force-pushed the acorn-v2.6.2 branch 2 times, most recently from 01317fc to 39f802d Compare November 2, 2023 09:40

StrongMonkey commented Nov 2, 2023

View reviewed changes

controllers/service/service_controller.go Show resolved Hide resolved

Support allocating shared LB for services

337919f

Signed-off-by: Daishan Peng <[email protected]>

StrongMonkey force-pushed the acorn-v2.6.2 branch from 39f802d to 337919f Compare November 2, 2023 10:07

StrongMonkey requested a review from thedadams November 2, 2023 17:09

thedadams reviewed Nov 2, 2023

View reviewed changes

StrongMonkey requested a review from thedadams November 2, 2023 19:52

Address comments

6c74e5f

Signed-off-by: Daishan Peng <[email protected]>

StrongMonkey force-pushed the acorn-v2.6.2 branch from 4c8f739 to 6c74e5f Compare November 2, 2023 23:03

thedadams approved these changes Nov 2, 2023

View reviewed changes

StrongMonkey merged commit 5af52aa into acorn-io:acorn-v2.6.2 Nov 3, 2023
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support allocating shared LB for services #1

Support allocating shared LB for services #1

StrongMonkey commented Oct 31, 2023

StrongMonkey Oct 31, 2023

StrongMonkey Oct 31, 2023

StrongMonkey Oct 31, 2023

StrongMonkey Oct 31, 2023

thedadams Nov 1, 2023

StrongMonkey Nov 2, 2023

thedadams Nov 1, 2023

StrongMonkey Nov 2, 2023

thedadams Nov 1, 2023

StrongMonkey Nov 2, 2023

thedadams Nov 1, 2023

thedadams Nov 1, 2023

StrongMonkey Nov 2, 2023

thedadams Nov 2, 2023

thedadams Nov 2, 2023

StrongMonkey Nov 2, 2023

		}
		if svc.Annotations[service.LoadBalancerAllocatingPortKey] == "true" {

	r.allocatedServices[svc.Annotations[service.LoadBalancerStackKey]] = map[int]bool{}
	r.allocatedServices[svc.Annotations[service.LoadBalancerStackKey]] = map[int]struct{}{}

	if err := b.client.List(context.Background(), &serviceList); err != nil {
	if err := b.client.List(ctx, &serviceList); err != nil {

Support allocating shared LB for services #1

Support allocating shared LB for services #1

Conversation

StrongMonkey commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment