Support edge nodes report events by llmmqq8023 · Pull Request #5811 · kubeedge/kubeedge

llmmqq8023 · 2024-08-20T09:30:25Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support edge nodes report events

Proposal: #5722

Shelley-BaoYue · 2024-08-27T02:26:41Z

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	appcorev1 "k8s.io/client-go/applyconfigurations/core/v1"
+	fakecorev1 "k8s.io/client-go/kubernetes/typed/core/v1/fake"


Please sort these import.

Shelley-BaoYue · 2024-08-27T02:27:01Z

@@ -0,0 +1,95 @@
+package client


Add a copyright

Shelley-BaoYue · 2024-08-27T02:27:13Z

+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	appcorev1 "k8s.io/client-go/applyconfigurations/core/v1"


ditto, sort the import.

Shelley-BaoYue · 2024-08-27T02:28:31Z


 func (m *metaManager) process(message model.Message) {
 	operation := message.GetOperation()
+	if _, resType, _ := parseResource(&message); resType == model.ResourceTypeEvent {


Could we use processInsert and processUpdate func?

Shelley-BaoYue · 2024-08-27T02:29:42Z

@@ -325,6 +326,8 @@ type EdgeControllerBuffer struct {
 type EdgeControllerLoad struct {
 	// UpdatePodStatusWorkers indicates the load of update pod status workers


You need add a comment for HandleEventWorkers

Shelley-BaoYue · 2024-08-27T02:29:55Z

@@ -252,6 +252,7 @@ type EdgeController struct {
 type EdgeControllerBuffer struct {
 	// UpdatePodStatus indicates the buffer of pod status


You need add a comment for HandleEvent

magichan · 2024-08-28T15:24:00Z

build fail, @llmmqq8023 you may need fix it before merge

Signed-off-by: llmmqq8023 <273337875@qq.com> cloud Signed-off-by: llmmqq8023 <273337875@qq.com> cloud Signed-off-by: llmmqq8023 <273337875@qq.com>

Signed-off-by: llmmqq8023 <273337875@qq.com>

Signed-off-by: llmmqq8023 <273337875@qq.com> Modified comments and other content Signed-off-by: llmmqq8023 <273337875@qq.com> Revert "不需要推送" This reverts commit edcf206. Modified comments and other content Signed-off-by: llmmqq8023 <273337875@qq.com>

llmmqq8023 · 2024-08-29T08:03:10Z

build fail, @llmmqq8023 you may need fix it before merge

Thank you for pointing out the build issue. I have made the necessary fixes， please try it again~

magichan · 2024-08-29T12:28:24Z

+				evt := &v1.Event{}
+				err = json.Unmarshal(data, evt)
+				if err != nil {
+					klog.Errorf("Error marshaling createEvent msg content %v", err)


this is Unmarshal action，not marshal action.

You may need to modify the error description to something like this

magichan · 2024-08-29T12:30:21Z

+		case msg := <-uc.eventChan:
+			data, err := msg.GetContentData()
+			if err != nil {
+				klog.Errorf("Get event data err: %v", err)


printing msg.GetID() in error log is a good practice.

you may need add it in your error log.

magichan · 2024-08-29T12:39:50Z

+				}
+				_, err = uc.kubeClient.CoreV1().Events(evt.Namespace).CreateWithEventNamespace(evt)
+				if err != nil {
+					klog.Errorf("CreateWithEventNamespace error, event: %v, err: %v", evt, err)


Unless you know what you're doing, printing all the information of a structure is a recommended action.

You may need to select the attributes worth printing.

There are several similar issues, if you agree with the review, you may want to check and correct them yourself.

magichan · 2024-08-29T12:40:32Z

+				evt := &v1.Event{}
+				err = json.Unmarshal(data, evt)
+				if err != nil {
+					klog.Errorf("Error marshaling updateEvent msg content %v", err)


don't use %v for err var.

Could you please explain more about why cannot use %v to log err?

After checking, my comment was wrong,

%v works better for nil.

see https://stackoverflow.com/questions/61283248/format-errors-in-go-s-v-or-w

please close it.

magichan · 2024-08-29T13:55:41Z

 	DefaultCertificateSigningRequestWorkers  = 4

 	DefaultUpdatePodStatusBuffer            = 1024
+	DefaultProcessEventBuffer               = 1024


Please allow me to be curious,

@wbc6080 Do we need to keep common/constants/default.go and staging/src/github.com/kubeedge/api/apis/common/constants/default.go consistent?

If not, can the unused variables be removed?

Related work have been done in #5844

magichan · 2024-08-29T14:36:23Z

+}
+
+// Create logs the event creation request.
+func (e *EventsBridge) Create(ctx context.Context, event *corev1.Event, opts metav1.CreateOptions) (*corev1.Event, error) {


about the comment Create logs the event creation request.
I don't think the function is responsible for logging; its responsibility is as a bridge or proxy, forwarding requests to the meta.

magichan · 2024-08-29T15:12:12Z

+}
+
+func (e *events) CreateWithEventNamespace(event *corev1.Event) (*corev1.Event, error) {
+	resource := fmt.Sprintf("%s/%s/%s", e.namespace, model.ResourceTypeEvent, event.Name)


The same confusion, whether to use NameSpace in Core1.Event within functions ending with WithEventNamespace

But unlike the previous one, which was so certain, the decision on which namespace to use here needs to be made on your own.

Yes, it should be changed to:

Suggested change

resource := fmt.Sprintf("%s/%s/%s", e.namespace, model.ResourceTypeEvent, event.Name)

resource := fmt.Sprintf("%s/%s/%s", event.namespace, model.ResourceTypeEvent, event.Name)

@llmmqq8023

magichan · 2024-08-29T15:22:41Z

+	return event, nil
+}
+
+type PatchInfo struct {


I see the repeat define PatchInfo in MetaManager and EdgeController.

Could you check if it's possible to extract this into the API package to reduce duplication?

Shelley-BaoYue · 2024-08-30T09:12:52Z

 	}
 }

+type PatchInfo struct {


Suggested change

type PatchInfo struct {

type EventPatchInfo struct {

Shelley-BaoYue · 2024-08-30T09:29:51Z

+				evt := &v1.Event{}
+				err = json.Unmarshal(data, evt)
+				if err != nil {
+					klog.Errorf("Error marshaling updateEvent msg content %v", err)


Could you please explain more about why cannot use %v to log err?

Shelley-BaoYue · 2024-08-30T09:35:36Z

+}
+
+func (e *events) CreateWithEventNamespace(event *corev1.Event) (*corev1.Event, error) {
+	resource := fmt.Sprintf("%s/%s/%s", e.namespace, model.ResourceTypeEvent, event.Name)


Yes, it should be changed to:

Suggested change

resource := fmt.Sprintf("%s/%s/%s", e.namespace, model.ResourceTypeEvent, event.Name)

resource := fmt.Sprintf("%s/%s/%s", event.namespace, model.ResourceTypeEvent, event.Name)

@llmmqq8023

Shelley-BaoYue · 2024-08-30T09:39:02Z

Please solve the error of golint and UT

Signed-off-by: llmmqq8023 <273337875@qq.com> solve the error of golint and UT Signed-off-by: llmmqq8023 <273337875@qq.com> solve the error of golint and UT Signed-off-by: llmmqq8023 <273337875@qq.com>

wbc6080

Please make the lint check pass

…n "sendEvent". Signed-off-by: llmmqq8023 <273337875@qq.com>

Signed-off-by: llmmqq8023 <273337875@qq.com>

Shelley-BaoYue · 2024-09-19T03:30:50Z

+	// ImagePrePullController
+	DefaultImagePrePullJobStatusBuffer = 1024
+	DefaultImagePrePullJobEventBuffer  = 1
+	DefaultImagePrePullJobWorkers      = 1


These contants need to be removed. It seems a misoperation importing these constants when you rebase the master branch

Shelley-BaoYue · 2024-09-19T03:36:15Z

 // EdgeControllerLoad indicates the EdgeController load
 type EdgeControllerLoad struct {
+	// HandleEventWorkers indicates the load of handle event workers
+	// default 1


https://github.com/kubeedge/kubeedge/pull/5811/files#diff-85964fa94ec623fce7d82bcc2717627f112d94a8649758f9d52544a01af876aeR44 Please keep consistent with DefaultProcessEventWorkers

Shelley-BaoYue · 2024-09-19T03:53:40Z

+	// SendEvents indicates whether to send events to cloud,
+	// if set to false, events will not be sent to cloud.
+	// default true
+	SendEvent bool `json:"sendEvent"`


In order to allow users to smoothly upgrade and adapt to new versions, I think it is more appropriate to disable this feature by default in the next two release.

Signed-off-by: llmmqq8023 <273337875@qq.com>

fisherxu

Overall looks good.

fisherxu · 2024-10-09T07:24:25Z

+	// SendEvents indicates whether to send events to cloud,
+	// if set to true, events will be sent to cloud.
+	// default false
+	SendEvent bool `json:"sendEvent"`


ReportEvent should be better?

cc @wbc6080 @Shelley-BaoYue @WillardHu

ReportEvent should be better?

+1

Signed-off-by: llmmqq8023 <273337875@qq.com>

fisherxu

Plz fix the lint check error, and you can also run make lint in your local host to check, thanks :) @llmmqq8023

Shelley-BaoYue · 2024-10-15T12:18:39Z

+	"k8s.io/client-go/kubernetes/fake"
+	"testing"
+	"time"
+)


sort the import

Shelley-BaoYue · 2024-10-15T12:19:05Z

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	appcorev1 "k8s.io/client-go/applyconfigurations/core/v1"
+	"testing"


Shelley-BaoYue · 2024-10-15T12:20:36Z

 // EdgeControllerLoad indicates the EdgeController load
 type EdgeControllerLoad struct {
+	// ProcessEventWorkers indicates the load of process event workers
+	// default 1


Suggested change

// default 1

// default 4

according your code, the default value should be 4

Signed-off-by: llmmqq8023 <273337875@qq.com> modify imports Signed-off-by: llmmqq8023 <273337875@qq.com> change an annotation of "default" Signed-off-by: llmmqq8023 <273337875@qq.com>

Shelley-BaoYue · 2024-10-16T04:02:38Z

+}
+
+func TestEventReport(t *testing.T) {
+


remove unnecessary blank line

Signed-off-by: llmmqq8023 <273337875@qq.com> add copyright for _test.go Signed-off-by: llmmqq8023 <273337875@qq.com>

Shelley-BaoYue

/lgtm
cc @fisherxu @WillardHu @wbc6080

fisherxu

/approve
Thanks for the great work!

kubeedge-bot · 2024-10-17T09:31:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fisherxu

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~~ [fisherxu]

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

kubeedge-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 20, 2024

kubeedge-bot requested review from Baoqiang-Zhang, GsssC, Shelley-BaoYue, anyushun and subpathdev August 20, 2024 09:30

kubeedge-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 20, 2024

llmmqq8023 changed the title ~~Edge event~~ Support edge nodes report events Aug 20, 2024

llmmqq8023 force-pushed the edge_event branch from b38ea3a to 0efe0c5 Compare August 20, 2024 10:18

kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024

Shelley-BaoYue reviewed Aug 27, 2024

View reviewed changes

Shelley-BaoYue added this to the v1.19 milestone Aug 27, 2024

llmmqq8023 force-pushed the edge_event branch 3 times, most recently from 4c5fb19 to ebc96e6 Compare August 28, 2024 12:23

llmmqq8023 force-pushed the edge_event branch from 1a1f36e to e62c128 Compare August 29, 2024 05:15

llmmqq8023 added 2 commits August 29, 2024 13:18

cloud components for events report

b805e48

Signed-off-by: llmmqq8023 <273337875@qq.com> cloud Signed-off-by: llmmqq8023 <273337875@qq.com> cloud Signed-off-by: llmmqq8023 <273337875@qq.com>

edge components for events report

e559f98

Signed-off-by: llmmqq8023 <273337875@qq.com>

llmmqq8023 force-pushed the edge_event branch from e62c128 to e559f98 Compare August 29, 2024 05:22

magichan reviewed Aug 29, 2024

View reviewed changes

Shelley-BaoYue reviewed Aug 30, 2024

View reviewed changes

llmmqq8023 changed the title ~~Support edge nodes report events~~ WIP: Support edge nodes report events Sep 2, 2024

kubeedge-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 2, 2024

Modify variable names and outputs of klog

8082dd1

Signed-off-by: llmmqq8023 <273337875@qq.com> solve the error of golint and UT Signed-off-by: llmmqq8023 <273337875@qq.com> solve the error of golint and UT Signed-off-by: llmmqq8023 <273337875@qq.com>

Merge remote-tracking branch 'upstream/master' into edge_event

9e1ac42

llmmqq8023 changed the title ~~WIP: Support edge nodes report events~~ Support edge nodes report events Sep 4, 2024

kubeedge-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2024

wbc6080 reviewed Sep 9, 2024

View reviewed changes

kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 10, 2024

llmmqq8023 added 2 commits September 18, 2024 19:54

User can choose whether to report events by changing the configuratio…

b39b5ef

…n "sendEvent". Signed-off-by: llmmqq8023 <273337875@qq.com>

rebase

3877aa9

Signed-off-by: llmmqq8023 <273337875@qq.com>

kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2024

Shelley-BaoYue reviewed Sep 19, 2024

View reviewed changes

kubeedge-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2024

rebase and change the default value of "sendEvent" to false

29f7cd9

Signed-off-by: llmmqq8023 <273337875@qq.com>

kubeedge-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2024

add UT

066812b

Signed-off-by: llmmqq8023 <273337875@qq.com>

kubeedge-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 28, 2024

fisherxu reviewed Oct 9, 2024

View reviewed changes

rename some variables

1fc218a

Signed-off-by: llmmqq8023 <273337875@qq.com>

fisherxu reviewed Oct 15, 2024

View reviewed changes

Shelley-BaoYue reviewed Oct 15, 2024

View reviewed changes

modify some tests

02b5af9

Signed-off-by: llmmqq8023 <273337875@qq.com> modify imports Signed-off-by: llmmqq8023 <273337875@qq.com> change an annotation of "default" Signed-off-by: llmmqq8023 <273337875@qq.com>

llmmqq8023 force-pushed the edge_event branch from 4ff0af6 to 02b5af9 Compare October 15, 2024 13:22

Shelley-BaoYue reviewed Oct 16, 2024

View reviewed changes

add copyright for _test.go

67aa477

Signed-off-by: llmmqq8023 <273337875@qq.com> add copyright for _test.go Signed-off-by: llmmqq8023 <273337875@qq.com>

llmmqq8023 force-pushed the edge_event branch from cd1f75f to 67aa477 Compare October 16, 2024 07:03

Shelley-BaoYue reviewed Oct 17, 2024

View reviewed changes

kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2024

fisherxu reviewed Oct 17, 2024

View reviewed changes

kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2024

kubeedge-bot merged commit 063a213 into kubeedge:master Oct 17, 2024

		@@ -325,6 +326,8 @@ type EdgeControllerBuffer struct {
		type EdgeControllerLoad struct {
		// UpdatePodStatusWorkers indicates the load of update pod status workers

		@@ -252,6 +252,7 @@ type EdgeController struct {
		type EdgeControllerBuffer struct {
		// UpdatePodStatus indicates the buffer of pod status

	resource := fmt.Sprintf("%s/%s/%s", e.namespace, model.ResourceTypeEvent, event.Name)
	resource := fmt.Sprintf("%s/%s/%s", event.namespace, model.ResourceTypeEvent, event.Name)

Conversation

llmmqq8023 commented Aug 20, 2024

Uh oh!

Uh oh!

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!

magichan commented Aug 28, 2024

Uh oh!

llmmqq8023 commented Aug 29, 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shelley-BaoYue commented Aug 30, 2024

Uh oh!

wbc6080 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!

fisherxu 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!

fisherxu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!