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.

kmesh-bot · 2024-09-26T07:03:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kevin-wangzefeng for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

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

hzxuzhonghu · 2024-09-29T07:14:51Z

pkg/controller/workload/workload_processor.go

@@ -401,6 +425,11 @@ func (p *Processor) handleWorkload(workload *workloadapi.Workload) error {
 	p.WorkloadCache.AddOrUpdateWorkload(workload)
 	p.storeWorkloadPolicies(workload.GetUid(), workload.GetAuthorizationPolicies())

+	// update kmesh localityCache
+	if p.nodeName == workload.GetNode() {
+		p.locality.SetLocality(p.nodeName, workload.GetClusterId(), workload.GetNetwork(), workload.GetLocality())


not sure i understand why only setLocality when p.nodeName == workload.GetNode()

Because the kmesh process retrieves information about all workloads from xDS, and the locality associated with the kmesh process should be carried by the workloads on the same node, we only update locality information from workloads that have the same nodeName as the current one.

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)

hzxuzhonghu · 2024-09-29T07:23:48Z

pkg/controller/workload/bpfcache/locality_cache.go

+	return l.isLocalityInfoSet && l.isRoutingPreferenceSet
+}
+
+func (l *LocalityCache) CalcuLocalityLBPrio(wl *workloadapi.Workload) uint32 {


not sure i understand this: From my perspective, the workload priority has to do with the original node info.

If the node and the workload both reside in same RoutingPreference [], the workload has highest priority.

Yes, in the earliest design version, it was designed this way. During a kmesh community meeting, you suggested considering the current design format. Given that the kmesh process exists on every node as a DaemonSet, it is possible to pre-calculate the priority of each workload within a service relative to that node on each node. This way, when subsequent traffic arrives and accesses the service, the backend to serve can be selected based on the pre-calculated priorities of the workloads for that service.

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

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

hzxuzhonghu · 2024-10-11T06:25:14Z

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
+            return -ENOENT;


This is not align with ztunnel behavior, this return value would make the traffic goes through like directly accessing the clusterIP, but not blocking the traffic

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

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

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

Open

fix workload_processor_test

f30eaae

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

hzxuzhonghu reviewed Sep 29, 2024

View reviewed changes

derekwin force-pushed the lb-dev-09 branch from 86cc4bd to f3fa74a Compare October 6, 2024 09:11

fix spammy workload_processor.go

07372de

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

derekwin force-pushed the lb-dev-09 branch from f3fa74a to 07372de Compare October 6, 2024 09:30

fix gofmt error

ffc97e8

derekwin force-pushed the lb-dev-09 branch from e3f1e38 to 606db1a Compare October 11, 2024 02:28

change priority from 0 to 6

1be65fb

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

derekwin force-pushed the lb-dev-09 branch 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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add locality loadbalance to kmesh workload mode. #900

Add locality loadbalance to kmesh workload mode. #900

derekwin commented Sep 26, 2024 •

edited

Loading

kmesh-bot commented Sep 26, 2024

hzxuzhonghu Sep 29, 2024 •

edited

Loading

derekwin Sep 30, 2024

hzxuzhonghu left a comment

hzxuzhonghu Sep 29, 2024

derekwin Sep 30, 2024

hzxuzhonghu Sep 29, 2024

derekwin Sep 30, 2024

hzxuzhonghu Sep 29, 2024

derekwin Sep 30, 2024

hzxuzhonghu Sep 29, 2024

derekwin Sep 30, 2024

hzxuzhonghu Sep 29, 2024

derekwin Sep 30, 2024

hzxuzhonghu Sep 29, 2024

derekwin Sep 30, 2024

hzxuzhonghu Sep 29, 2024

derekwin Sep 30, 2024

derekwin commented Oct 11, 2024

hzxuzhonghu left a comment

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

hzxuzhonghu Oct 11, 2024

	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)

Add locality loadbalance to kmesh workload mode. #900

Are you sure you want to change the base?

Add locality loadbalance to kmesh workload mode. #900

Conversation

derekwin commented Sep 26, 2024 • edited Loading

kmesh-bot commented Sep 26, 2024

hzxuzhonghu Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu left a 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

derekwin commented Oct 11, 2024

hzxuzhonghu left a 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

derekwin commented Sep 26, 2024 •

edited

Loading

hzxuzhonghu Sep 29, 2024 •

edited

Loading