support kmesh manage in pod level #414

weli-l · 2024-06-04T11:10:10Z

What type of PR is this?

What this PR does / why we need it:
support Kmesh manage in pod level
Which issue(s) this PR fixes:
Fixes #325

Special notes for your reviewer:

How to enable kmesh management at pod level

kubectl label pod xxx istio.io/dataplane-mode=kmesh

How to judge whether pod is managed by Kmesh

Due to the feature of the bypass, the impact of the bypass needs to be considered comprehensively

Label priority（From high to low）
- kmesh.net/bypass=enabled
- istio.io/dataplane-mode=Kmesh(pod)
- istio.io/dataplane-mode=Kmesh(namespace)
Annotation priority（From high to low，and it is only be used for user to judge whether pod is managed by kmesh）
- kmesh.net/bypass=enabled
- kmesh.net/redirection=enabled(pod)
- kmesh.net/redirection=enabled(namespace)

Does this PR introduce a user-facing change?:

kmesh-bot · 2024-06-04T11:10:16Z

[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 hzxuzhonghu 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:

pkg/controller/OWNERS

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

hzxuzhonghu

Please add test coverage

codecov-commenter · 2024-06-05T01:31:44Z

Codecov Report

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

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

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

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

Files	Coverage Δ
pkg/controller/bypass/bypass_controller.go	`0.00% <0.00%> (ø)`
pkg/controller/controller.go	`0.00% <0.00%> (ø)`
pkg/controller/netns/netns.go	`0.00% <0.00%> (ø)`
pkg/controller/manage/kmesh_manage.go	`0.00% <0.00%> (ø)`

LiZhenCheng9527 · 2024-06-05T02:06:21Z

pkg/controller/kmesh/kmesh_manage.go

+		`{"metadata":{"annotations":{"%s":null}}}`,
+		KmeshAnnotation,
+	))
+	annotationAddPatch = []byte(fmt.Sprintf(


If this variable is a fixed value, you can use the constant

LiZhenCheng9527 · 2024-06-05T02:14:23Z

pkg/controller/netns/netns.go

+)
+
+var (
+	FS embed.FS


Does this variable have to be a global variable?

hzxuzhonghu · 2024-06-05T11:51:51Z

pkg/controller/kmesh/kmesh_manage.go

move to controller if this is not shared with other components

kwb0523 · 2024-06-07T03:45:14Z

pkg/controller/controller.go

@@ -75,7 +76,17 @@ func (c *Controller) Start() error {
 	if c.client.workloadController != nil {
 		c.client.workloadController.Processor.Sm = secertManager
 	}
+	clientset, err := utils.GetK8sclient()


Can the clientset be reused with bypass? and does the kmesh manager logic need to be moved before the bypass logic.

YaoZengzeng · 2024-06-13T03:35:38Z

pkg/controller/controller.go

@@ -22,6 +22,7 @@ import (
 	"kmesh.net/kmesh/pkg/bpf"
 	"kmesh.net/kmesh/pkg/constants"
 	"kmesh.net/kmesh/pkg/controller/bypass"
+	kmeshmanage "kmesh.net/kmesh/pkg/controller/kmesh"


It's weired to name a sub package as kmesh

YaoZengzeng · 2024-06-13T03:38:15Z

pkg/controller/kmesh/kmesh_manage.go

+
+			if _, ok := namespace.Labels[LabelSelectorKmesh]; ok {
+				log.Infof("namespace %s has label 'istio.io/dataplane-mode=Kmesh', skipping further processing", pod.GetNamespace())
+				return


Use constant defined above to replace 'istio.io/dataplane-mode=Kmesh'

Use constant defined above to replace 'istio.io/dataplane-mode=Kmesh'

defined

YaoZengzeng · 2024-06-13T03:39:36Z

pkg/controller/kmesh/kmesh_manage.go

+			nspath, _ := ns.GetNSpath(pod)
+
+			if err := handleKmeshManage(nspath, true); err != nil {
+				log.Errorf("failed to enable kmesh manage")


Use Kmesh instead of kmesh

Use Kmesh instead of kmesh

Yes, I have modified

YaoZengzeng · 2024-06-13T03:39:56Z

pkg/controller/kmesh/kmesh_manage.go

+
+			if _, ok := namespace.Labels[LabelSelectorKmesh]; ok {
+				log.Infof("namespace %s has label 'istio.io/dataplane-mode=Kmesh', skipping further processing", pod.GetNamespace())
+				return


YaoZengzeng · 2024-06-13T03:40:23Z

pkg/controller/kmesh/kmesh_manage.go

+			}
+
+			if err := patchKmeshAnnotation(client, pod, false); err != nil {
+				log.Errorf("failed to add kmesh annotation, err is %v", err)


ditto

done

YaoZengzeng · 2024-06-13T03:41:12Z

pkg/controller/kmesh/kmesh_manage.go

+			}
+		},
+	}); err != nil {
+		return fmt.Errorf("error adding event handler to podInformer: %v", err)


Suggested change

return fmt.Errorf("error adding event handler to podInformer: %v", err)

return fmt.Errorf("failed to add event handler to podInformer: %v", err)

YaoZengzeng · 2024-06-13T03:42:16Z

pkg/controller/kmesh/kmesh_manage.go

+	return pod.ObjectMeta.DeletionTimestamp != nil
+}
+
+func patchKmeshAnnotation(client kubernetes.Interface, pod *corev1.Pod, oper bool) error {


Suggested change

func patchKmeshAnnotation(client kubernetes.Interface, pod *corev1.Pod, oper bool) error {

func patchKmeshAnnotation(client kubernetes.Interface, pod *corev1.Pod, op bool) error {

oper is weired

oper is weired

I have modified

YaoZengzeng · 2024-06-13T03:44:21Z

pkg/controller/netns/netns.go

+	return true
+}
+
+// copied from istio/cni/pkg/nodeagent/podcgroupns.go


It's better to add url

It's better to add url

done

supercharge-xsy · 2024-06-14T08:11:51Z

pkg/controller/manage/kmesh_manage.go

+				log.Errorf("expected *corev1.Pod but got %T", obj)
+				return
+			}
+			namespace, err := client.CoreV1().Namespaces().Get(context.TODO(), pod.GetNamespace(), metav1.GetOptions{})


This logic of checking if ns has a label can be abstracted into a common function； used by AddFunc and DeleteFunc

This logic of checking if ns has a label can be abstracted into a common function； used by AddFunc and DeleteFunc

I have deleted this logic, because it may affect the data in bpf map kmesh_manage when kmesh restart

supercharge-xsy · 2024-06-14T08:13:20Z

pkg/controller/manage/kmesh_manage.go

+		 * daemon to enable/disable kmesh manage
+		 */
+		simip := net.ParseIP("0.0.0.1")
+		port := 929


magic numbers

magic numbers

I have defined constant

supercharge-xsy · 2024-06-14T08:13:49Z

pkg/controller/manage/kmesh_manage.go

+			return err
+		}
+		errno, ok := err.(syscall.Errno)
+		if ok && errno == 115 { // -EINPROGRESS, Operation now in progress


magic numbers

magic numbers

I have defined the constant

hzxuzhonghu · 2024-06-17T02:18:53Z

pkg/controller/manage/kmesh_manage.go

+	KmeshAnnotation           = "kmesh.net/redirection"
+)
+
+func StartKmeshManageController(client kubernetes.Interface) error {


Suggested change

func StartKmeshManageController(client kubernetes.Interface) error {

func NewKmeshManageController(client kubernetes.Interface) error {

hzxuzhonghu · 2024-06-17T02:19:36Z

pkg/controller/manage/kmesh_manage.go

+	stopChan := make(chan struct{})
+	nodeName := os.Getenv("NODE_NAME")
+
+	informerFactory := informers.NewSharedInformerFactoryWithOptions(client, DefaultInformerSyncPeriod,


nit: we can share one factory with bypass

nit: we can share one factory with bypass

I think it is not necessary to share the factory, because labels selector in factory is different

I donot think this is a blocking issue, file an issue though after merged, we should optimize, otherwise, it will broing double load to k8s apiserver from all nodes. Keep in mind, in a cluster, the node number can be more than 5000

hzxuzhonghu · 2024-06-19T02:46:47Z

pkg/controller/manage/kmesh_manage.go

+	SpecialIpForKmesh         = "0.0.0.1"
+	EnableKmeshPort           = 929
+	DisableKmeshPort          = 930
+	LabelSelectorKmesh        = "istio.io/dataplane-mode=Kmesh"


Suggested change

LabelSelectorKmesh = "istio.io/dataplane-mode=Kmesh"

KmeshManagedLabelSelector = constants.DataPlaneModeLabel + DataPlaneModeKmesh

hzxuzhonghu · 2024-06-19T02:47:41Z

pkg/controller/manage/kmesh_manage.go

+	SpecialIpForKmesh         = "0.0.0.1"
+	EnableKmeshPort           = 929
+	DisableKmeshPort          = 930
+	LabelSelectorKmesh        = "istio.io/dataplane-mode=Kmesh"


BTW, i think we not only support Kmesh, but also kmesh. We are case insensitive, so i donot think this label selector suits here

pkg/controller/manage/kmesh_manage.go

hzxuzhonghu · 2024-06-19T02:50:09Z

pkg/controller/manage/kmesh_manage.go

+		}))
+
+	informerFactory.Start(wait.NeverStop)
+	informerFactory.WaitForCacheSync(wait.NeverStop)


This is blocking, so let;s move this to a Run method, so we can run all the controller's Run async without waiting each other.

hzxuzhonghu

Please add some tests

hzxuzhonghu · 2024-06-24T09:53:52Z

pkg/controller/manage/kmesh_manage.go

+	EnableKmeshPort           = 929
+	DisableKmeshPort          = 930
+	LabelSelectorKmesh        = constants.DataPlaneModeLabel + "=" + constants.DataPlaneModeKmesh
+	KmeshAnnotation           = "kmesh.net/redirection"


KmeshRedirectionAnnotation

KmeshRedirectionAnnotation

done

hzxuzhonghu · 2024-06-24T09:56:23Z

pkg/controller/manage/kmesh_manage.go

+	return pod.ObjectMeta.DeletionTimestamp != nil
+}
+
+func patchKmeshAnnotation(client kubernetes.Interface, pod *corev1.Pod, op bool) error {


Nit: donot use op, i would rather split into two functions

Nit: donot use op, i would rather split into two functions

splited

Signed-off-by: weli-l <[email protected]>

hzxuzhonghu · 2024-06-29T01:36:45Z

pkg/controller/bypass/bypass_controller.go

@@ -88,7 +79,7 @@ func StartByPassController(client kubernetes.Interface) error {
 				return
 			}

-			nspath, _ := getnspath(pod)
+			nspath, _ := ns.GetNSpath(pod)


nit: replace GetNSpath with GetPodNsPath

hzxuzhonghu · 2024-06-29T01:39:55Z

pkg/controller/manage/kmesh_manage.go

+	SpecialIpForKmesh          = "0.0.0.1"
+	EnableKmeshPort            = 929
+	DisableKmeshPort           = 930
+	LabelSelectorKmesh         = constants.DataPlaneModeLabel + "=" + constants.DataPlaneModeKmesh


As i said, we must support both kmesh and Kmesh, should be case insensitive to be consistent with previous behavior

hzxuzhonghu · 2024-06-29T02:01:25Z

pkg/controller/manage/kmesh_manage.go

+
+const (
+	DefaultInformerSyncPeriod  = 30 * time.Second
+	SpecialIpForKmesh          = "0.0.0.1"


Please check with that from CNI, we use 0.0.0.2 now for ipv4 and for v6 ::2

We can reuse TriggerControlCommand function

hzxuzhonghu · 2024-06-29T02:08:39Z

pkg/controller/manage/kmesh_manage_test.go

+
+	done := make(chan struct{})
+	go func() {
+		wg.Wait()


I think you should not wait in a go routine

hzxuzhonghu · 2024-06-29T02:09:37Z

pkg/controller/manage/kmesh_manage_test.go

+	select {
+	case <-done:
+		mu.Lock()
+		assert.True(t, enabled, "expected handleKmeshManage to be called for enabling Kmesh manage")
+		assert.False(t, disabled, "expected handleKmeshManage not to be called for disabling Kmesh manage")
+		mu.Unlock()
+	case <-time.After(1 * time.Second):
+		t.Fatalf("timed out waiting for handleKmeshManage to be called")
+	}


wrap this with a checker function, so you canreuse it below

hzxuzhonghu · 2024-06-29T02:10:14Z

pkg/controller/manage/kmesh_manage_test.go

+	podLister := controller.informerFactory.Core().V1().Pods().Lister()
+	pods, err := podLister.Pods(pod.Namespace).List(labels.Everything())
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(pods), "expected One Pod in the lister after addition")


This can be not tested in out case, this is the implement detail of k8s

hzxuzhonghu · 2024-06-29T02:11:02Z

pkg/controller/manage/kmesh_manage_test.go

+	mu.Lock()
+	enabled = false
+	disabled = false
+	mu.Unlock()


Not sure i understand why you need to lock them. Any race?

hzxuzhonghu · 2024-06-29T02:12:02Z

pkg/controller/manage/kmesh_manage_test.go

+	mu.Unlock()
+
+	wg.Add(1)
+	err = client.CoreV1().Pods("default").Delete(context.TODO(), "test-pod", metav1.DeleteOptions{})


can you add a case of removing the label first

kmesh-bot requested review from kwb0523 and nlgwcy June 4, 2024 11:10

kmesh-bot added the size/XL label Jun 4, 2024

hzxuzhonghu requested changes Jun 4, 2024

View reviewed changes

weli-l force-pushed the dev/kmesh_manage branch from 1e67ec5 to d629c10 Compare June 5, 2024 01:24

LiZhenCheng9527 reviewed Jun 5, 2024

View reviewed changes

hzxuzhonghu reviewed Jun 5, 2024

View reviewed changes

pkg/controller/kmesh/kmesh_manage.go Outdated

Copy link

Member

hzxuzhonghu Jun 5, 2024

Choose a reason for hiding this comment

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

move to controller if this is not shared with other components

weli-l force-pushed the dev/kmesh_manage branch 2 times, most recently from f467e82 to 18eeb4d Compare June 7, 2024 03:43

kwb0523 reviewed Jun 7, 2024

View reviewed changes

weli-l force-pushed the dev/kmesh_manage branch 3 times, most recently from 74928c3 to 8d6d1ba Compare June 12, 2024 02:16

kmesh-bot added size/L and removed size/XL labels Jun 12, 2024

YaoZengzeng reviewed Jun 13, 2024

View reviewed changes

weli-l force-pushed the dev/kmesh_manage branch 3 times, most recently from f8d84ee to c193e52 Compare June 13, 2024 13:40

supercharge-xsy reviewed Jun 14, 2024

View reviewed changes

hzxuzhonghu reviewed Jun 17, 2024

View reviewed changes

weli-l force-pushed the dev/kmesh_manage branch from c193e52 to 9f373f4 Compare June 18, 2024 13:06

kmesh-bot added size/XL and removed size/L labels Jun 18, 2024

weli-l force-pushed the dev/kmesh_manage branch from 9f373f4 to 17e4c40 Compare June 18, 2024 13:18

kmesh-bot added the do-not-merge/contains-merge-commits label Jun 18, 2024

weli-l force-pushed the dev/kmesh_manage branch from 77b110f to 9f373f4 Compare June 18, 2024 13:33

kmesh-bot added do-not-merge/contains-merge-commits and removed do-not-merge/contains-merge-commits labels Jun 18, 2024

weli-l force-pushed the dev/kmesh_manage branch from 62a8c64 to 9f373f4 Compare June 18, 2024 13:37

kmesh-bot removed the do-not-merge/contains-merge-commits label Jun 18, 2024

weli-l force-pushed the dev/kmesh_manage branch 3 times, most recently from d085c6f to f722364 Compare June 19, 2024 01:30

kmesh-bot added size/L and removed size/XL labels Jun 19, 2024

hzxuzhonghu reviewed Jun 19, 2024

View reviewed changes

pkg/controller/manage/kmesh_manage.go Show resolved Hide resolved

hzxuzhonghu reviewed Jun 19, 2024

View reviewed changes

weli-l force-pushed the dev/kmesh_manage branch 3 times, most recently from 88d1a65 to ef91742 Compare June 24, 2024 08:23

hzxuzhonghu requested changes Jun 24, 2024

View reviewed changes

weli-l force-pushed the dev/kmesh_manage branch 3 times, most recently from 2365a44 to 98a37f8 Compare June 27, 2024 12:30

kmesh-bot added size/XL and removed size/L labels Jun 28, 2024

weli-l added 2 commits June 28, 2024 18:00

support kmesh manage in pod level

35bf6e9

Signed-off-by: weli-l <[email protected]>

modify kmesh manage controller

b13f7df

Signed-off-by: weli-l <[email protected]>

weli-l force-pushed the dev/kmesh_manage branch from ed0e965 to b8943b8 Compare June 28, 2024 10:00

add UT for kmesh manage

9d91221

Signed-off-by: weli-l <[email protected]>

weli-l force-pushed the dev/kmesh_manage branch from b8943b8 to 9d91221 Compare June 28, 2024 10:06

hzxuzhonghu reviewed Jun 29, 2024

View reviewed changes

	return fmt.Errorf("error adding event handler to podInformer: %v", err)
	return fmt.Errorf("failed to add event handler to podInformer: %v", err)

	func patchKmeshAnnotation(client kubernetes.Interface, pod *corev1.Pod, oper bool) error {
	func patchKmeshAnnotation(client kubernetes.Interface, pod *corev1.Pod, op bool) error {

	func StartKmeshManageController(client kubernetes.Interface) error {
	func NewKmeshManageController(client kubernetes.Interface) error {

	LabelSelectorKmesh = "istio.io/dataplane-mode=Kmesh"
	KmeshManagedLabelSelector = constants.DataPlaneModeLabel + DataPlaneModeKmesh

support kmesh manage in pod level #414

Are you sure you want to change the base?

support kmesh manage in pod level #414

Conversation

weli-l commented Jun 4, 2024 • edited Loading

How to enable kmesh management at pod level

How to judge whether pod is managed by Kmesh

kmesh-bot commented Jun 4, 2024

hzxuzhonghu left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 5, 2024 • edited Loading

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

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

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

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

weli-l commented Jun 4, 2024 •

edited

Loading

codecov-commenter commented Jun 5, 2024 •

edited

Loading