kmesh support restart by reload old bpf map and prog by lec-bit · Pull Request #475 · kmesh-net/kmesh

lec-bit · 2024-07-02T13:00:44Z

What type of PR is this?
/kind feature

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-07-03T02:10:14Z

daemon/manager/manager.go

 	defer cniInstaller.Stop()

-	setupCloseHandler()
+	*configs.Status = setupCloseHandler()


Assign an int to a pointer?

Because I want the data in configs.Status and configs.BpfConfig.Status to be the same at the same time

you can change status from pointer to int

I add a KmeshStartStatus in bpf

LiZhenCheng9527 · 2024-07-03T02:18:00Z

daemon/manager/manager.go


-	setupCloseHandler()
+	*configs.Status = setupCloseHandler()
+	log.Printf("bpfLoader.Restart %v", *configs.Status)


Suggested change

log.Printf("bpfLoader.Restart %v", *configs.Status)

log.Printf("Kmesh Restart %d", *configs.Status)

LiZhenCheng9527 · 2024-07-03T02:23:44Z

pkg/bpf/bpf.go


 	"kmesh.net/kmesh/daemon/options"
 	"kmesh.net/kmesh/pkg/logger"
+	pkgversion "kmesh.net/kmesh/pkg/version"


Why not use version directly?

Suggested change

pkgversion "kmesh.net/kmesh/pkg/version"

"kmesh.net/kmesh/pkg/version"

LiZhenCheng9527 · 2024-07-03T02:25:42Z

pkg/bpf/bpf.go


 	obj         *BpfKmesh
 	workloadObj *BpfKmeshWorkload
+	Status      *int


Suggested change

Status *int

Status int

LiZhenCheng9527 · 2024-07-03T02:32:35Z

pkg/bpf/bpf_kmesh_workload.go

+	}
+
+	if err := sc.Link.Pin(sc.Info.BpfFsPath + "sockconn_prog"); err != nil {
+		log.Printf("file exit %v", sc.Info.BpfFsPath+"sockconn_prog")


Suggested change

log.Printf("file exit %v", sc.Info.BpfFsPath+"sockconn_prog")

log.Printf("file exited %s", sc.Info.BpfFsPath+"sockconn_prog")

LiZhenCheng9527 · 2024-07-03T02:35:37Z

pkg/bpf/bpf_restart.go

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2023 The Kmesh Authors.


Suggested change

* Copyright 2023 The Kmesh Authors.

* Copyright 2024 The Kmesh Authors.

LiZhenCheng9527 · 2024-07-03T02:38:40Z

pkg/version/version.go

+
+	err = m.Pin(versionPath + "kmesh_version")
+	if err != nil {
+		log.Errorf("kmesh_version failed to pin: %v", err)


Suggested change

log.Errorf("kmesh_version failed to pin: %v", err)

log.Errorf("kmesh_version pin failed: %v", err)

hzxuzhonghu · 2024-07-04T02:13:15Z

@lec-bit Can you fix and prevent force pushing? So it will be firendly to reviewers

hzxuzhonghu

should we make version map as a global config used by kmesh, in future we could expand it.

In addition, can we store it in the files instead of bpf map, which seems unnecessary.

hzxuzhonghu · 2024-07-05T03:28:23Z

build/docker/start_kmesh.sh

-          umount -t cgroup2 /mnt/kmesh_cgroup2/
-          rm -rf /mnt/kmesh_cgroup2
-          rm -rf /sys/fs/bpf/bpf_kmesh
+        echo "kmesh close"


Suggested change

echo "kmesh close"

echo "kmesh exit"

hzxuzhonghu · 2024-07-05T03:30:41Z

daemon/manager/manager.go

 	defer cniInstaller.Stop()

-	setupCloseHandler()
+	*configs.Status = setupCloseHandler()


you can change status from pointer to int

daemon/manager/manager.go

hzxuzhonghu · 2024-07-05T03:56:13Z

pkg/bpf/bpf_restart.go

+		log.Printf("daemonset err:%v", err)
+		return false
+	}
+	return true


Not right, you should check the kmesh name.

hzxuzhonghu · 2024-07-05T04:08:07Z

pkg/version/version.go

+	if config.AdsEnabled() {
+		versionPath = config.BpfFsPath + "/bpf_kmesh/map/kmesh_version"
+	} else if config.WdsEnabled() {
+		versionPath = config.BpfFsPath + "/bpf_kmesh_workload/map/kmesh_version"


Yoiu can remove the duplicate path construct within NewVersionMap

hzxuzhonghu · 2024-07-05T04:10:45Z

pkg/version/version.go

move the logic of map operation out to bpf loader of bpf.go

hzxuzhonghu · 2024-07-05T04:11:53Z

pkg/version/version.go

+	NewStart = iota
+	Restart
+	Update
+	Reload


what does reload mean

mean we need to reload bpf map, it is restart

hzxuzhonghu · 2024-07-05T04:12:05Z

pkg/version/version.go

+const (
+	NewStart = iota
+	Restart
+	Update


and update means what?

For the future update scenario, if the two gitversions are different after the kmesh is restarted, it indicates that the update scenario is used and the previous configuration is not load.

YaoZengzeng · 2024-07-05T08:30:40Z

daemon/manager/manager.go

 }

-func setupCloseHandler() {
+func setupCloseHandler() int {


This function seems to be used to handle os signal and it's weired to get status and return here.

it doesn‘t matter, i can move func GetDaemonset out

YaoZengzeng · 2024-07-05T08:52:46Z

daemon/manager/manager.go


 	log.Warn("exiting...")
+
+	if bpf.GetDaemonset() {


Why if we get Kmesh daemonset object, we could make sure it's restart?

When Kmesh is create for the first time, it always creates the Kmesh DaemonSet object first and then creates the Kmesh pods.

yes, always creates the Kmesh DaemonSet object first and then creates the Kmesh pods.
and by end, there is some confusion，
although the current I can judge if restart by daemonset in bpfLoader.Stop, there is still a lack of theoretical basis

YaoZengzeng · 2024-07-05T09:04:04Z

pkg/bpf/bpf_restart.go

+	return true
+}
+
+func cleanupMountPath() {


This function is not used?

sorry, forget
now in bpfLoader.Stop

YaoZengzeng · 2024-07-05T09:08:04Z

pkg/bpf/bpf_restart.go

+	NewStart = iota
+	Restart
+	Update
+	Reload


These constants also defined in pkg/version, can they be defined in one place?

codecov · 2024-07-12T02:33:13Z

Codecov Report

Attention: Patch coverage is 65.06849% with 51 lines in your changes missing coverage. Please review.

Project coverage is 48.88%. Comparing base (cc9a829) to head (481937a).
Report is 16 commits behind head on main.

Files	Coverage Δ
pkg/utils/test/bpf_map.go	`43.58% <20.00%> (+0.73%)`	⬆️
pkg/bpf/bpf_kmesh_workload.go	`63.22% <66.66%> (+0.21%)`	⬆️
pkg/bpf/bpf_restart.go	`65.11% <65.11%> (ø)`
pkg/bpf/bpf.go	`41.17% <67.50%> (+20.90%)`	⬆️

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 064614b...481937a. Read the comment docs.

hzxuzhonghu · 2024-07-12T06:41:52Z

@lec-bit ping me when it is ready

lec-bit · 2024-07-22T08:07:31Z

@hzxuzhonghu I have added test cases and fully verified this part of the function, and it can be merged now.

hzxuzhonghu · 2024-07-22T09:14:46Z

Will take a look later

hzxuzhonghu · 2024-07-23T11:48:29Z

deploy/yaml/clusterrole.yaml

  verbs: ["get", "update", "patch", "list", "watch"]
+- apiGroups: ["apps"]
+  resources: ["daemonsets"]
+  verbs: ["list", "get"]


to align with above, i think you donot need list permission

yes, i will delete

hzxuzhonghu · 2024-07-23T11:49:20Z

pkg/bpf/bpf.go

 	return &BpfLoader{
-		config: config,
+		config:    config,
+		StatusMap: NewVersionMap(config),


StatusMap or VersionMap

ok, VersionMap

hzxuzhonghu · 2024-07-23T11:51:25Z

pkg/bpf/bpf_restart.go

+
+const (
+	NewStart = iota
+	Restart


Add some comments

hzxuzhonghu · 2024-07-23T11:51:34Z

pkg/bpf/bpf_restart.go

@@ -0,0 +1,100 @@
+/*
+ * Copyright 2024 The Kmesh Authors.


remove year

hzxuzhonghu · 2024-07-23T11:54:57Z

pkg/bpf/bpf_restart.go

+}
+
+func CleanupBpfMap() {
+	err := syscall.Unmount("/mnt/kmesh_cgroup2", 0)


prevent hardcoding

hzxuzhonghu · 2024-07-23T11:59:42Z

pkg/bpf/bpf_restart.go

+	if GetDaemonset() {
+		SetKmeshStartStatus(Restart)
+	} else {
+		SetKmeshStartStatus(NewStart)


Not sure i understand the intention here, since it is closing

i will check the name

hzxuzhonghu · 2024-07-23T12:02:16Z

pkg/bpf/bpf_restart.go

+	kmeshStartStatus = Status
+}
+
+func GetDaemonset() bool {


To be more clear, we can move this implement detail into a function InferRestartStatus

hzxuzhonghu · 2024-07-23T12:06:16Z

pkg/bpf/bpf_restart.go

+
+var kmeshStartStatus int
+
+func GetKmeshStartStatus() int {


Confusing with GetStartStatus

Signed-off-by: lec-bit <glfhmzmy@126.com>

Signed-off-by: let-bit <glfhmzmy@126.com>

hzxuzhonghu · 2024-07-24T07:19:27Z

pkg/bpf/bpf_restart.go

+// Normal: a normal new start
+// Restart: reusing the previous kmesh configuration
+// Update: upgrading kmesh and reusing part of previous kmesh configuration


better add some indents

hzxuzhonghu · 2024-07-24T07:20:19Z

pkg/bpf/bpf_restart.go

+// Restart: reusing the previous kmesh configuration
+// Update: upgrading kmesh and reusing part of previous kmesh configuration
+// Close:
+// Normal: normal close


clean up all the bpf progs and maps

Signed-off-by: let-bit <glfhmzmy@126.com>

hzxuzhonghu · 2024-07-24T07:23:55Z

pkg/bpf/bpf_restart.go

+	return kmeshStatus
+}
+
+func SetKmeshStatus(Status int) {


Can we remove it, i get confused by it with SetStartStatus

hzxuzhonghu · 2024-07-24T07:26:02Z

pkg/bpf/bpf_restart.go

+
+const (
+	daemonSetName = "kmesh"
+	namespace     = "kmesh-system"


This can be changed, we can get it by
podNamespace := env.Register("POD_NAMESPACE", "", "").Get()

hzxuzhonghu · 2024-07-24T07:31:01Z

pkg/bpf/bpf_restart.go

+// Normal: normal close
+// Restart: not clean kmesh configuration, for next launch
+// Update: not clean part of kmesh configuration, for next launch
+const (


we may define a
type StartType uint8

and

const ( Normal StartType = iota

hzxuzhonghu · 2024-07-24T07:31:24Z

pkg/bpf/bpf_restart.go

+	kmeshStatus = Status
+}
+
+func InferRestartStatus() bool {


make it return StartType

hzxuzhonghu · 2024-07-24T07:32:11Z

pkg/bpf/bpf_restart.go

+	kmeshStatus = Status
+}
+
+func InferRestartStatus() bool {


and use small cased functionname to keep it private

hzxuzhonghu · 2024-07-24T07:37:10Z

pkg/bpf/bpf.go

+	}
+}
+
+func GetMap(m *ebpf.Map, key uint32) [16]byte {


getOldVersionFromMap

hzxuzhonghu · 2024-07-24T07:39:45Z

pkg/bpf/bpf.go

+
+	versionMap, err := ebpf.LoadPinnedMap(versionPath+"kmesh_version", opts)
+	if err != nil {
+		log.Debugf("kmesh version map LoadPinnedMap failed: %v, Start normal", err)


Suggested change

log.Debugf("kmesh version map LoadPinnedMap failed: %v, Start normal", err)

log.Debugf("kmesh version map loadfailed: %v, start normally", err)

hzxuzhonghu · 2024-07-24T07:40:05Z

pkg/bpf/bpf.go

+	return valueBytes
+}
+
+func RecoverVersionMap(config *options.BpfConfig, versionPath string) *ebpf.Map {


Suggested change

func RecoverVersionMap(config *options.BpfConfig, versionPath string) *ebpf.Map {

func recoverVersionMap(config *options.BpfConfig, versionPath string) *ebpf.Map {

hzxuzhonghu · 2024-07-24T07:44:17Z

pkg/bpf/bpf.go

+		return nil
+	}
+
+	err = m.Pin(filepath.Join(versionPath + "kmesh_version"))


If L222 returns err, wouldn't m.Pin panic?

yes，will Panic；
I will add an error handling, generally no error will be returned here unless there are insufficient resources

hzxuzhonghu · 2024-07-24T07:46:20Z

pkg/bpf/bpf_restart_test.go

+}
+
+// Test Kmesh Restart Normal
+func runTestRestart(t *testing.T) {


is there a update case?

Restart, no Update now, there is currently no Update scenario

Signed-off-by: let-bit <glfhmzmy@126.com>

hzxuzhonghu · 2024-07-25T12:38:51Z

pkg/bpf/bpf.go

 var log = logger.NewLoggerField("pkg/bpf")

+var (
+	hash = fnv.New32a()


move to the function

hzxuzhonghu · 2024-07-25T12:41:31Z

pkg/bpf/bpf.go

+		m := recoverVersionMap(config, versionPath)
 		if m != nil {
-			SetStartStatus(m)
+			CompareOldGitVersion(m)


The function name does not reflect the functionality.

hzxuzhonghu · 2024-07-25T12:47:59Z

pkg/bpf/bpf_restart.go

 }

-func SetKmeshStatus(Status int) {
+func SetKmeshStatus(Status StartType) {


SetStartType

hzxuzhonghu · 2024-07-25T12:48:12Z

pkg/bpf/bpf_restart.go

+var kmeshStatus StartType

-func GetKmeshStatus() int {
+func GetKmeshStatus() StartType {


GetStartType

Signed-off-by: let-bit <glfhmzmy@126.com>

hzxuzhonghu · 2024-07-26T02:09:37Z

/lgtm

/approve

kmesh-bot · 2024-07-26T02:09:46Z

[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

Details

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/feature label Jul 2, 2024

kmesh-bot requested review from bitcoffeeiux and hzxuzhonghu July 2, 2024 13:00

kmesh-bot added size/L do-not-merge/contains-merge-commits labels Jul 2, 2024

lec-bit force-pushed the kmesh-build branch from 8954420 to 10b2f14 Compare July 2, 2024 13:10

kmesh-bot added do-not-merge/contains-merge-commits and removed do-not-merge/contains-merge-commits labels Jul 2, 2024

lec-bit force-pushed the kmesh-build branch from c94369f to 4f9f0a8 Compare July 2, 2024 13:41

kmesh-bot added do-not-merge/contains-merge-commits and removed do-not-merge/contains-merge-commits labels Jul 2, 2024

lec-bit force-pushed the kmesh-build branch from 5fc2f54 to 4f9f0a8 Compare July 2, 2024 14:00

kmesh-bot added do-not-merge/contains-merge-commits and removed do-not-merge/contains-merge-commits labels Jul 2, 2024

lec-bit force-pushed the kmesh-build branch from f52c8cf to 4f9f0a8 Compare July 2, 2024 14:12

kmesh-bot added do-not-merge/contains-merge-commits and removed do-not-merge/contains-merge-commits labels Jul 2, 2024

LiZhenCheng9527 reviewed Jul 3, 2024

View reviewed changes

lec-bit force-pushed the kmesh-build branch from 9e1cf57 to 94a6231 Compare July 3, 2024 09:47

kmesh-bot removed the do-not-merge/contains-merge-commits label Jul 3, 2024

kmesh-bot added the do-not-merge/contains-merge-commits label Jul 4, 2024

hzxuzhonghu reviewed Jul 5, 2024

View reviewed changes

YaoZengzeng reviewed Jul 5, 2024

View reviewed changes

lec-bit force-pushed the kmesh-build branch from 906ab58 to 17fe2c8 Compare July 5, 2024 11:50

kmesh-bot removed the do-not-merge/contains-merge-commits label Jul 5, 2024

lec-bit force-pushed the kmesh-build branch from 17fe2c8 to efa0bc2 Compare July 5, 2024 12:28

lec-bit force-pushed the kmesh-build branch 3 times, most recently from 5b9a91a to e72518c Compare July 22, 2024 07:55

hzxuzhonghu reviewed Jul 23, 2024

View reviewed changes

lec-bit force-pushed the kmesh-build branch from 58b147c to 9884dd1 Compare July 23, 2024 13:39

lec-bit added 4 commits July 23, 2024 14:04

kmesh-reboot

21d7beb

Signed-off-by: lec-bit <glfhmzmy@126.com>

Distinguish between restart and upgrade

7f4392a

Signed-off-by: let-bit <glfhmzmy@126.com>

modify some functions and structures

1994ba6

Signed-off-by: let-bit <glfhmzmy@126.com>

add bpf_restart_test.go

28a251a

Signed-off-by: let-bit <glfhmzmy@126.com>

lec-bit force-pushed the kmesh-build branch from 9884dd1 to 18287bd Compare July 23, 2024 14:07

Modify according to community feedback

d7d5aea

Signed-off-by: let-bit <glfhmzmy@126.com>

lec-bit force-pushed the kmesh-build branch from 18287bd to d7d5aea Compare July 23, 2024 14:15

hzxuzhonghu reviewed Jul 24, 2024

View reviewed changes

Note description modification

bc995c8

Signed-off-by: let-bit <glfhmzmy@126.com>

hzxuzhonghu reviewed Jul 25, 2024

View reviewed changes

optimization

f8af16e

Signed-off-by: let-bit <glfhmzmy@126.com>

hzxuzhonghu reviewed Jul 25, 2024

View reviewed changes

Adjust naming convention

481937a

Signed-off-by: let-bit <glfhmzmy@126.com>

kmesh-bot assigned hzxuzhonghu Jul 26, 2024

kmesh-bot added the lgtm label Jul 26, 2024

kmesh-bot added the approved label Jul 26, 2024

kmesh-bot merged commit 7069d8c into kmesh-net:main Jul 26, 2024

lec-bit deleted the kmesh-build branch October 19, 2024 08:54

	log.Printf("bpfLoader.Restart %v", *configs.Status)
	log.Printf("Kmesh Restart %d", *configs.Status)

	pkgversion "kmesh.net/kmesh/pkg/version"
	"kmesh.net/kmesh/pkg/version"

	log.Printf("file exit %v", sc.Info.BpfFsPath+"sockconn_prog")
	log.Printf("file exited %s", sc.Info.BpfFsPath+"sockconn_prog")

	* Copyright 2023 The Kmesh Authors.
	* Copyright 2024 The Kmesh Authors.

	log.Errorf("kmesh_version failed to pin: %v", err)
	log.Errorf("kmesh_version pin failed: %v", err)

	log.Debugf("kmesh version map LoadPinnedMap failed: %v, Start normal", err)
	log.Debugf("kmesh version map loadfailed: %v, start normally", err)

	func RecoverVersionMap(config options.BpfConfig, versionPath string) ebpf.Map {
	func recoverVersionMap(config options.BpfConfig, versionPath string) ebpf.Map {

Conversation

lec-bit commented Jul 2, 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!

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 Jul 4, 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!

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!

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!

codecov bot commented Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hzxuzhonghu commented Jul 12, 2024

Uh oh!

lec-bit commented Jul 22, 2024

Uh oh!

hzxuzhonghu commented Jul 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2024 •

edited

Loading