Add locality loadbalance to kmesh workload mode. #900

derekwin · 2024-09-26T07:03:27Z

What type of PR is this?
/kind enhancement

~~This PR is not ready to be merged; some features are not working properly.~~
1. The priority of the service Pod written to the BPF map is observed to be correct in user space, but the behavior of locality load balancing in kernel space is random. (This issue does not exist in the test version of the code at this link.)
2. The code for the workload has undergone significant changes in the past one to two months. The current PR's code attempts to adapt to the new logic of the workload as much as possible. However, a lot of redundant deletion behaviors have been observed. These parts of the code need to be optimized in conjunction with the existing workload logic.

This pr is ok.

But i find a bug in new version kubectl and istio work with kmesh(#910).

this pr's enhancement need new version kubectl and istio.

Signed-off-by: derekwin <[email protected]>

hzxuzhonghu · 2024-09-29T07:03:59Z

pkg/controller/workload/bpfcache/locality_cache.go

+	// notice: s should set by lb.GetRoutingPreference()
+	if len(s) > 0 {
+		l.RoutingPreference = s
+		l.LbStrictIndex = uint32(len(s))


Not sure i understand this, previously i thought this is a bool flag for strict mode.

In strict mode, only the workloads that exactly match every item in the routePreference are considered. In other words, only the workloads with a priority equal to len(routePreference) are taken into account.　We set LbStrictIndex so that the kernel BPF program filters out priorities other than lbStrictIndex in strict mode.

hzxuzhonghu

I only looked at the go code part

hzxuzhonghu · 2024-09-29T07:07:36Z

pkg/controller/workload/workload_processor.go

@@ -179,6 +181,7 @@ func (p *Processor) removeWorkloadFromBpfMap(uid string) error {
 		wpkDelete = bpf.WorkloadPolicyKey{}
 	)

+	log.Warnf("== removeWorkloadFromBpfMap: workload uid: %#v, backendUid: %#v", uid, p.hashName.Hash(uid))


spammy, please use debug

hzxuzhonghu · 2024-09-29T07:10:03Z

pkg/controller/workload/workload_processor.go

-			ekDelete := bpf.EndpointKey{
-				ServiceId:    serviceId,
-				BackendIndex: i,
+		var j uint32


nit: you donot need to define j here, move below

for j:=0; j <= bpf.MaxPrio; j++ {

hzxuzhonghu · 2024-09-29T07:12:01Z

pkg/controller/workload/workload_processor.go

@@ -290,16 +300,17 @@ func (p *Processor) removeServiceResourceFromBpfMap(svc *workloadapi.Service, na
 }

 // addWorkloadToService update service & endpoint bpf map when a workload has new bound services
-func (p *Processor) addWorkloadToService(sk *bpf.ServiceKey, sv *bpf.ServiceValue, uid uint32) error {
+func (p *Processor) addWorkloadToService(sk *bpf.ServiceKey, sv *bpf.ServiceValue, uid uint32, Prio uint32) error {


Suggested change

func (p *Processor) addWorkloadToService(sk *bpf.ServiceKey, sv *bpf.ServiceValue, uid uint32, Prio uint32) error {

func (p *Processor) addWorkloadToService(sk *bpf.ServiceKey, sv *bpf.ServiceValue, uid uint32, priority uint32) error {

hzxuzhonghu · 2024-09-29T07:13:39Z

pkg/controller/workload/workload_processor.go

@@ -359,6 +382,7 @@ func (p *Processor) updateWorkload(workload *workloadapi.Workload) error {
 	)

 	uid := p.hashName.Hash(workload.GetUid())
+	log.Warnf("=in= updateWorkload: workload uid: %#v, backendUid: %#v", workload.GetUid(), uid)


nit: spammy

pkg/controller/workload/workload_processor.go

hzxuzhonghu · 2024-09-29T07:17:33Z

pkg/controller/workload/workload_processor.go

+	newValue.LbPolicy = uint32(lb.GetMode()) // set loadbalance mode
+	p.locality.SetRoutingPreference(lb.GetRoutingPreference())
+	p.locality.LbPolicy = newValue.LbPolicy
+	log.Debugf("lbPolicy:%#v, routingPreference:%#v, strictIndex:%#v", newValue.LbPolicy, p.locality.RoutingPreference, p.locality.LbStrictIndex)


Suggested change

log.Debugf("lbPolicy:%#v, routingPreference:%#v, strictIndex:%#v", newValue.LbPolicy, p.locality.RoutingPreference, p.locality.LbStrictIndex)

log.Debugf("lbPolicy:%v, routingPreference:%v, strictIndex:%v", newValue.LbPolicy, p.locality.RoutingPreference, p.locality.LbStrictIndex)

pkg/controller/workload/bpfcache/locality_cache.go

derekwin · 2024-10-11T02:37:21Z

After resolving the issue mentioned in #910, functional testing was conducted on the current code, and the functionality was found to be working correctly.

hzxuzhonghu

Will try to review the control plane part later

hzxuzhonghu · 2024-10-11T03:39:31Z

bpf/kmesh/workload/include/workload.h

@@ -10,6 +10,8 @@
 #define MAX_PORT_COUNT            10
 #define MAX_SERVICE_COUNT         10
 #define RINGBUF_SIZE              (1 << 12)
+#define MIN_PRIO                  6


redundant with PRIO_COUNT

hzxuzhonghu · 2024-10-11T03:43:36Z

bpf/kmesh/workload/include/service.h

-    endpoint_k.backend_index = bpf_get_prandom_u32() % service_v->endpoint_count + 1;
+    endpoint_k.prio = MIN_PRIO; // for random handle，all endpoints are saved in MIN_PRIO
+
+    rand_k = bpf_get_prandom_u32() % service_v->prio_endpoint_count[MIN_PRIO] + 1;


we can return fast by checking 0 here since you removed it from the caller

short circuit

hzxuzhonghu · 2024-10-11T03:45:33Z

bpf/kmesh/workload/include/workload.h

+    __u32 prio_endpoint_count[PRIO_COUNT]; // endpoint count of current service with prio
+    __u32 lb_policy;      // load balancing algorithm, currently supports random algorithm, locality loadbalance
+                          // Failover/strict mode
+    __u32 lb_strict_prio; // for failover strict mode


seems this is not needed , LB_POLICY_FAILOVER and LB_POLICY_STRICT values are supported

hzxuzhonghu · 2024-10-11T06:23:58Z

bpf/kmesh/workload/include/service.h

+            }
+            return 0; // find the backend successfully
+        }
+        if (is_strict && match_prio == service_v->lb_strict_prio) { // only match lb strict index


can you explain a little bit more, i am confused about this

bpf/kmesh/workload/include/service.h

hzxuzhonghu · 2024-10-11T07:47:51Z

pkg/status/status_server_test.go

@@ -388,7 +388,7 @@ func TestServer_dumpWorkloadBpfMap(t *testing.T) {
 			{ServiceId: 1}, {ServiceId: 2},
 		}
 		testServiceVals := []bpfcache.ServiceValue{
-			{EndpointCount: 1234}, {EndpointCount: 5678},
+			{EndpointCount: [7]uint32{1234, 1234, 1234, 1234, 1234, 1234, 1234}}, {EndpointCount: [7]uint32{5678, 5678, 5678, 5678, 5678, 5678, 5678}},


not sure i understand

I think this is just a dummy code.

I mean in this test i donot see you set the routing preference, why expect 7 priority groups

hzxuzhonghu · 2024-10-11T07:48:38Z

pkg/controller/workload/workload_processor.go

@@ -179,6 +181,7 @@ func (p *Processor) removeWorkloadFromBpfMap(uid string) error {
 		wpkDelete = bpf.WorkloadPolicyKey{}
 	)

+	log.Debugf("removeWorkloadFromBpfMap: workload %s, backendUid %d", uid, p.hashName.Hash(uid))


nit: move after L185

hzxuzhonghu · 2024-10-11T08:09:53Z

pkg/controller/workload/workload_processor_test.go

We do need a test coverage for service lb policy update

hzxuzhonghu · 2024-10-11T08:15:37Z

pkg/controller/workload/workload_processor.go

-				log.Errorf("addWorkloadToService workload %d service %d failed: %v", workloadId, sk.ServiceId, err)
-				return err
+			if sv.LbPolicy == LbPolicyRandom { // random mode
+				if err = p.addWorkloadToService(&sk, &sv, workloadId, bpf.MinPrio); err != nil { // In random mode, we save all workload to minprio


I would suggest store with highest prio

YaoZengzeng · 2024-10-17T01:45:33Z

pkg/controller/workload/workload_processor.go

+				if p.locality.CanLocalityLB() {
+					prio := p.locality.CalcuLocalityLBPrio(workload)
+					if err = p.addWorkloadToService(&sk, &sv, workloadId, prio); err != nil {
+						log.Errorf("addWorkloadToService workload %d service %d failed: %v", workloadId, sk.ServiceId, err)


Suggested change

log.Errorf("addWorkloadToService workload %d service %d failed: %v", workloadId, sk.ServiceId, err)

log.Errorf("addWorkloadToService workload %d service %d pirority %d failed: %v", workloadId, sk.ServiceId, prio, err)

YaoZengzeng · 2024-10-17T01:51:53Z

pkg/controller/workload/bpfcache/locality_cache.go

+	return MinPrio - rank
+}
+
+func (l *LocalityCache) SaveToWaitQueue(wl *workloadapi.Workload) {


Doesn't need a lock?

Seems not necessary, the workload processor works in serial

Signed-off-by: derekwin <[email protected]>

hzxuzhonghu · 2024-10-28T02:16:31Z

When trying to delete multiple endpoints that all belong to the same service, the BackendIndex dynamically changes, leading to errors.

How could we delete endpoints belong to same service? The caller is actually deleting a workload

When updating a service, it's likely that the associated endpoints will be deleted in batches. we should not rule out the possibility of batch deletions in the future. Additionally, the function name deleteEndpointRecords suggests a general operation and should have a more robust implementation.

How are the endpoints be deleted in batch? I cannot understand

hzxuzhonghu · 2024-10-28T02:23:46Z

pkg/controller/workload/bpfcache/locality_cache.go

+			}
+		}
+	}
+	return uint32(len(rp)) - rank


This method is not right. Keep in mind, we shoud match rp []workloadapi.LoadBalancing_Scope in order, if one doesnot match, we should stop.

So i am super sure this result now is nort right.

There is not test coverage, i will write a test later to test your function. I am feeling this function is still not correct

Signed-off-by: derekwin <[email protected]>

derekwin · 2024-10-28T07:04:14Z

When trying to delete multiple endpoints that all belong to the same service, the BackendIndex dynamically changes, leading to errors.

How could we delete endpoints belong to same service? The caller is actually deleting a workload

When updating a service, it's likely that the associated endpoints will be deleted in batches. we should not rule out the possibility of batch deletions in the future. Additionally, the function name deleteEndpointRecords suggests a general operation and should have a more robust implementation.

How are the endpoints be deleted in batch? I cannot understand

In function func (p *Processor) updateEndpoint(serviceId uint32, toLLb bool) [line 500], we get all endpoints needed to be deleted and delete them in batch using deleteEndpointRecords [line 520].
p.s. In the previous meeting, we agreed on the plan to update the endpoints one by one. However, during the subsequent implementation, I found that deleting one endpoint causes the indices of other endpoints to change, leading to confusion during deletion. Therefore, I believe we should delete all the endpoints to be removed at once and then re-add each new endpoint.

derekwin · 2024-10-28T12:07:19Z

/retest

kmesh-bot · 2024-10-28T12:07:42Z

@derekwin: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hzxuzhonghu · 2024-10-29T01:53:29Z

/ok-to-test

Signed-off-by: derekwin <[email protected]>

hzxuzhonghu · 2024-10-29T11:40:35Z

pkg/controller/workload/workload_processor.go

-	eksDelete := []bpf.EndpointKey{}
-	backendUids := []uint32{}
-	for _, ep := range p.EndpointCache.List(serviceId) {
+func (p *Processor) updateEndpointOneByOne(serviceId uint32, epsDelete []cache.Endpoint, toLLb bool) error {


epsDelete may look confusing, indeed we are trying to re orgnize them

using epsUpdate now

hzxuzhonghu · 2024-10-29T11:46:01Z

pkg/controller/workload/workload_processor.go

+		return nil
+	}
+
+	for i := len(epsDelete) - 1; i >= 0; i-- {


nit: please document why we do thin in reverse order

hzxuzhonghu

LGTM.

Signed-off-by: derekwin <[email protected]>

hzxuzhonghu

/lgtm

kmesh-bot · 2024-10-30T02:55:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kmesh-bot added the kind/enhancement New feature or request label Sep 26, 2024

kmesh-bot requested review from hzxuzhonghu and supercharge-xsy September 26, 2024 07:03

kmesh-bot added do-not-merge/contains-merge-commits size/L labels Sep 26, 2024

derekwin force-pushed the lb-dev-09 branch from 4930126 to 8adb6da Compare September 26, 2024 07:05

kmesh-bot removed the do-not-merge/contains-merge-commits label Sep 26, 2024

derekwin force-pushed the lb-dev-09 branch 6 times, most recently from e37cf23 to aea4eec Compare September 28, 2024 02:36

add locality loadbalance

63920e7

Signed-off-by: derekwin <[email protected]>

derekwin force-pushed the lb-dev-09 branch from aea4eec to 63920e7 Compare September 28, 2024 02:54

derekwin mentioned this pull request Sep 28, 2024

add proposal for Locality LoadBalance #574

Merged

hzxuzhonghu reviewed Sep 29, 2024

View reviewed changes

derekwin force-pushed the lb-dev-09 branch 4 times, most recently from 606db1a to 1be65fb Compare October 11, 2024 02:36

hzxuzhonghu reviewed Oct 11, 2024

View reviewed changes

pkg/controller/workload/workload_processor_test.go Outdated

Copy link

Member

hzxuzhonghu Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need a test coverage for service lb policy update

hzxuzhonghu reviewed Oct 11, 2024

View reviewed changes

YaoZengzeng reviewed Oct 17, 2024

View reviewed changes

fix issues

75b115a

Signed-off-by: derekwin <[email protected]>

derekwin force-pushed the lb-dev-09 branch from 2346ed3 to 75b115a Compare October 24, 2024 07:34

derekwin requested a review from hzxuzhonghu October 27, 2024 03:13

hzxuzhonghu reviewed Oct 28, 2024

View reviewed changes

fix CalcLocalityLBPrio

e069a67

Signed-off-by: derekwin <[email protected]>

kmesh-bot added the ok-to-test label Oct 29, 2024

updateEndpoint OneByOne; add test

5c3d4e2

Signed-off-by: derekwin <[email protected]>

derekwin force-pushed the lb-dev-09 branch from eb77f66 to 5c3d4e2 Compare October 29, 2024 08:53

hzxuzhonghu changed the title ~~WIP: Add locality loadbalance to kmesh workload mode.~~ Add locality loadbalance to kmesh workload mode. Oct 29, 2024

kmesh-bot removed the do-not-merge/work-in-progress label Oct 29, 2024

hzxuzhonghu reviewed Oct 29, 2024

View reviewed changes

hzxuzhonghu previously approved these changes Oct 29, 2024

View reviewed changes

kmesh-bot added the approved label Oct 29, 2024

fix issue

6160b57

Signed-off-by: derekwin <[email protected]>

derekwin dismissed hzxuzhonghu’s stale review via 6160b57 October 29, 2024 14:42

kmesh-bot removed approved labels Oct 29, 2024

derekwin requested a review from hzxuzhonghu October 30, 2024 02:23

hzxuzhonghu approved these changes Oct 30, 2024

View reviewed changes

kmesh-bot assigned hzxuzhonghu Oct 30, 2024

kmesh-bot added the lgtm label Oct 30, 2024

kmesh-bot added the approved label Oct 30, 2024

kmesh-bot merged commit 6e56261 into kmesh-net:main Oct 30, 2024
9 checks passed

	func (p Processor) addWorkloadToService(sk bpf.ServiceKey, sv *bpf.ServiceValue, uid uint32, Prio uint32) error {
	func (p Processor) addWorkloadToService(sk bpf.ServiceKey, sv *bpf.ServiceValue, uid uint32, priority uint32) error {

	log.Debugf("lbPolicy:%#v, routingPreference:%#v, strictIndex:%#v", newValue.LbPolicy, p.locality.RoutingPreference, p.locality.LbStrictIndex)
	log.Debugf("lbPolicy:%v, routingPreference:%v, strictIndex:%v", newValue.LbPolicy, p.locality.RoutingPreference, p.locality.LbStrictIndex)

	log.Errorf("addWorkloadToService workload %d service %d failed: %v", workloadId, sk.ServiceId, err)
	log.Errorf("addWorkloadToService workload %d service %d pirority %d failed: %v", workloadId, sk.ServiceId, prio, err)

Add locality loadbalance to kmesh workload mode. #900

Add locality loadbalance to kmesh workload mode. #900

Uh oh!

Conversation

derekwin commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hzxuzhonghu Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

derekwin commented Oct 11, 2024

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Oct 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwin commented Sep 26, 2024 •

edited

Loading

hzxuzhonghu Sep 29, 2024 •

edited

Loading