some workload mode bugfix and waypoint optimization #361

kwb0523 · 2024-05-23T12:43:42Z

1.fix potential target port modify error when access backend directly
2.fix a backend that belongs to multi services
3.store waypoint info in service to accelerate waypoint access

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

LiZhenCheng9527 · 2024-05-23T12:59:55Z

pkg/controller/workload/workload_processor.go

+		log.Warnf("current only supprt single ip of a pod")
+	}
+
+	for _, ip := range workload.GetAddresses() { // current only support signle ip, if a pod have multi ips, the last one will be stored finally


Will multiple IPs be supported later?

I'm not sure if support is needed in the future, but currently it's not supported.

LiZhenCheng9527 · 2024-05-23T13:01:14Z

pkg/controller/workload/workload_processor.go

+		bv.Services[bv.ServiceCount] = p.hashName.StrToNum(serviceName)
+		bv.ServiceCount++
+		if bv.ServiceCount >= bpf.MaxServiceNum {
+			log.Warnf("exceed the max service count, currently, a pod can belong to a maximum of 10 services")


Wouldn't it be better to put the serviceName in warnning here?

what you meant don't understand

LiZhenCheng9527 · 2024-05-23T13:02:46Z

Don't use force-push if you don't have to, in case someone else is reviewing it.

hzxuzhonghu

@kwb0523 Please update the proposal doc as well to facilitate review

hzxuzhonghu · 2024-05-24T02:06:29Z

pkg/controller/workload/workload_processor.go

 	)

 	bk.BackendUid = uid
-	if err = p.bpf.BackendLookup(&bk, &bv); err == nil {
+	if err := p.bpf.BackendLookup(&bk, &bv); err == nil {


you'd better log the err

hzxuzhonghu · 2024-05-24T02:07:10Z

pkg/controller/workload/workload_processor.go

-			addr := waypoint.GetAddress().Address
-			bv.WaypointAddr = nets.ConvertIpByteToUint32(addr)
-			bv.WaypointPort = nets.ConvertPortToBigEndian(waypoint.GetHboneMtlsPort())
+	bv.IPv4 = nets.ConvertIpByteToUint32(ip)


what is the byte order now.

little endian, always like this.

nlgwcy · 2024-05-24T07:38:32Z

pkg/controller/workload/bpfcache/service.go

-	LbPolicy      uint32 // load balancing algorithm, currently only supports random algorithm
+	EndpointCount uint32       // endpoint count of current service
+	LbPolicy      uint32       // load balancing algorithm, currently only supports random algorithm
+	ServicePort   ServicePorts // ServicePort[i] and TargetPort[i] are a pair, i starts from 0 and max value is MaxPortNum-1


FYI, it's better to define a PortPair data struct.

currently，we use cilium bpf interface to operate the map, and the interface cannot support self defined data struct.

nlgwcy · 2024-05-24T09:34:06Z

pkg/controller/workload/workload_processor.go

+
+	for i, port := range ports {
+		if i >= bpf.MaxPortNum {
+			log.Warnf("exceed the max port count,current only support maximum of 10 ports")


Pls record detailed log information.

nlgwcy · 2024-05-24T09:51:21Z

pkg/controller/workload/workload_processor.go

+		}
+
+		sv.ServicePort[i] = nets.ConvertPortToBigEndian(port.ServicePort)
+		if strings.Contains(serviceName, "waypoint") {


Maybe incorrect judgment if ServiceName contains waypoint, but actually it is not waypoint service. FYI, it is no need to check each time.

yeah, there is indeed such a problem at present, and we will track the relevant optimization situation through this issue #370 in the future.

nlgwcy · 2024-05-24T11:40:09Z

pkg/controller/workload/workload_processor.go

+
+	if waypoint != nil {
+		sv.WaypointAddr = nets.ConvertIpByteToUint32(waypoint.GetAddress().Address)
+		sv.WaypointPort = nets.ConvertPortToBigEndian(waypoint.GetHboneMtlsPort())


waypoint.GetHboneMtlsPort() -> KmeshWaypointPort

here stored are ip and port of a waypoint service, not the waypoint backend port info.

nlgwcy · 2024-05-24T11:49:56Z

pkg/controller/workload/workload_processor.go

+	bv.IPv4 = nets.ConvertIpByteToUint32(ip)
+	bv.ServiceCount = 0
+	for serviceName := range portList {
+		bv.Services[bv.ServiceCount] = p.hashName.StrToNum(serviceName)


In the code, some Map such as endpointsByService using serviceName(string key) for store, however， call StrToNum when map is read. If the map is directly stored by number, conversion is not required and more efficiency.

This is mainly about the optimization related to endpointsByService, which may be carried out later and is not related to the current PR processing problem.

nlgwcy · 2024-05-24T12:09:30Z

bpf/kmesh/workload/include/backend.h

+            BPF_LOG(DEBUG, BACKEND, "access the backend by service:%d \n", service_id);
+#pragma unroll
+            for (__u32 j = 0; j < MAX_PORT_COUNT; j++) {
+                if (service_v->service_port[j] != 0 && user_port == service_v->service_port[j]) {


service_v->service_port[j] != 0 -- no need to check

nlgwcy · 2024-05-24T12:11:31Z

bpf/kmesh/workload/include/backend.h

        }
    }

    BPF_LOG(ERR, BACKEND, "failed to get the backend\n");
    return -ENOENT;
 }

+// access the backend directly
+static inline int direct_backend_manager(ctx_buff_t *ctx, backend_value *backend_v)


This layer of function encapsulation seems unnecessary.

hzxuzhonghu · 2024-05-24T12:26:09Z

bpf/kmesh/workload/include/backend.h

 {
    int ret;
    address_t target_addr;
    __u32 *sk = (__u32 *)ctx->sk;
    struct bpf_sock_tuple value_tuple = {0};

-    if (backend_v->waypoint_addr != 0 && backend_v->waypoint_port != 0) {
-        BPF_LOG(DEBUG, BACKEND, "find waypoint addr=[%u:%u]\n", backend_v->waypoint_addr, backend_v->waypoint_port);
+    if (addr != 0 && port != 0) {


I would suggest checking this before calling waypoint_manager, it would be more readable

hzxuzhonghu · 2024-05-24T12:27:22Z

bpf/kmesh/workload/include/backend.h

-    for (unsigned int i = 0; i < backend_v->port_count; i++) {
-        if (i >= MAX_PORT_COUNT) {
+    for (__u32 i = 0; i < backend_v->service_count; i++) {
+        if (i >= MAX_SERVICE_COUNT) {
            BPF_LOG(WARN, BACKEND, "exceed the max port count\n");


should update the log, max service count

hzxuzhonghu · 2024-05-24T12:28:24Z

bpf/kmesh/workload/include/backend.h

-            BPF_LOG(DEBUG, BACKEND, "get the backend addr=[%u:%u]\n", target_addr.ipv4, target_addr.port);
-            return 0;
+        if (service_id == backend_v->service[i]) {
+            BPF_LOG(DEBUG, BACKEND, "access the backend by service:%d \n", service_id);


\n is not needed

hzxuzhonghu · 2024-05-24T12:37:46Z

bpf/kmesh/workload/include/frontend.h

@@ -50,7 +50,7 @@ static inline int frontend_manager(ctx_buff_t *ctx, frontend_value *frontend_v)
    }

    if (direct_backend) {
-        ret = backend_manager(ctx, backend_v);
+        ret = direct_backend_manager(ctx, backend_v);


please add some comments, this is actually pod direct access, if a pod has watpoint captured, we will redirect to waypoint here. If it is not waypoint captured, we do nothing.

1.fix potential target port modify error when access backend directly 2.fix a backend that belongs to multi services 3.store waypoint info in service to accelerate waypoint access Signed-off-by: kwb0523 <kwb0523@163.com>

codecov-commenter · 2024-05-27T09:41:11Z

Codecov Report

Attention: Patch coverage is 0% with 46 lines in your changes are missing coverage. Please review.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Flag	Coverage Δ
unittests	`31.21% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
pkg/controller/workload/bpfcache/backend.go	`0.00% <ø> (ø)`
pkg/controller/workload/bpfcache/service.go	`0.00% <ø> (ø)`
pkg/controller/workload/workload_processor.go	`13.79% <0.00%> (ø)`

hzxuzhonghu · 2024-05-27T11:52:05Z

bpf/kmesh/workload/include/backend.h

-            BPF_LOG(WARN, BACKEND, "exceed the max port count\n");
+    for (__u32 i = 0; i < backend_v->service_count; i++) {
+        if (i >= MAX_SERVICE_COUNT) {
+            BPF_LOG(WARN, BACKEND, "exceed the max port count:%d\n", MAX_SERVICE_COUNT);


wrong log, it is max service number

hzxuzhonghu · 2024-05-27T11:52:24Z

bpf/kmesh/workload/include/backend.h

-                DEBUG, BACKEND, "get the backend addr=[%pI4h:%u]\n", &target_addr.ipv4, bpf_ntohs(target_addr.port));
-            return 0;
+        if (service_id == backend_v->service[i]) {
+            BPF_LOG(DEBUG, BACKEND, "access the backend by service:%d\n", service_id);


It seems that all logs have been added with ‘\n’. Do we need to delete them all together in this PR.

hzxuzhonghu · 2024-05-27T11:52:35Z

bpf/kmesh/workload/include/backend.h

+                    BPF_LOG(
+                        DEBUG,
+                        BACKEND,
+                        "get the backend addr=[%pI4h:%u]\n",


hzxuzhonghu · 2024-05-27T11:53:24Z

bpf/kmesh/workload/include/frontend.h

+            BPF_LOG(
+                DEBUG,
+                FRONTEND,
+                "find waypoint addr=[%pI4h:%u]\n",


hzxuzhonghu · 2024-05-27T11:53:52Z

bpf/kmesh/workload/include/service.h

+        BPF_LOG(
+            DEBUG,
+            SERVICE,
+            "find waypoint addr=[%pI4h:%u]\n",


Signed-off-by: kwb0523 <kwb0523@163.com>

hzxuzhonghu

/lgtm

kmesh-bot · 2024-05-27T12:45:42Z

[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 requested review from hzxuzhonghu and nlgwcy May 23, 2024 12:43

kmesh-bot added the size/L label May 23, 2024

kwb0523 force-pushed the workload-dev branch 3 times, most recently from c2d6b8e to 3d897b5 Compare May 23, 2024 12:58

LiZhenCheng9527 reviewed May 23, 2024

View reviewed changes

kwb0523 force-pushed the workload-dev branch from 3d897b5 to eb1332e Compare May 23, 2024 13:14

hzxuzhonghu reviewed May 24, 2024

View reviewed changes

nlgwcy reviewed May 24, 2024

View reviewed changes

hzxuzhonghu reviewed May 24, 2024

View reviewed changes

kwb0523 force-pushed the workload-dev branch from 142babb to 1eb233d Compare May 27, 2024 09:20

smoe workload mode bugfix and waypoint optimization

37b847f

1.fix potential target port modify error when access backend directly 2.fix a backend that belongs to multi services 3.store waypoint info in service to accelerate waypoint access Signed-off-by: kwb0523 <kwb0523@163.com>

kwb0523 force-pushed the workload-dev branch from 1eb233d to 37b847f Compare May 27, 2024 09:33

hzxuzhonghu reviewed May 27, 2024

View reviewed changes

comment fix

f9d3a9b

Signed-off-by: kwb0523 <kwb0523@163.com>

hzxuzhonghu approved these changes May 27, 2024

View reviewed changes

kmesh-bot assigned hzxuzhonghu May 27, 2024

kmesh-bot added the lgtm label May 27, 2024

kmesh-bot added the approved label May 27, 2024

kmesh-bot merged commit 03bb5a4 into kmesh-net:main May 27, 2024
3 checks passed

some workload mode bugfix and waypoint optimization #361

some workload mode bugfix and waypoint optimization #361

Conversation

kwb0523 commented May 23, 2024

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

LiZhenCheng9527 commented May 23, 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

nlgwcy May 24, 2024 • edited

Choose a reason for hiding this comment

kwb0523 May 27, 2024 • edited

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

Choose a reason for hiding this comment

codecov-commenter commented May 27, 2024 • edited

Codecov Report

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

hzxuzhonghu left a comment

Choose a reason for hiding this comment

kmesh-bot commented May 27, 2024

nlgwcy May 24, 2024 •

edited

kwb0523 May 27, 2024 •

edited

codecov-commenter commented May 27, 2024 •

edited