add events for ResourceInterpreter #5541

whitewindmills · 2024-09-13T10:52:00Z

What type of PR is this?
/kind cleanup

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?:

NONE

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

karmada-bot · 2024-09-13T10:52:09Z

[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 chaunceyjiang for approval. For more information see the Kubernetes Code Review Process.

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

Details

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

whitewindmills · 2024-09-13T10:52:24Z

/cc @RainbowMango

codecov-commenter · 2024-09-13T11:04:01Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

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

Project coverage is 34.13%. Comparing base (dc36986) to head (3dd93ec).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/detector/detector.go	0.00%	4 Missing ⚠️
pkg/controllers/status/common.go	0.00%	2 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5541      +/-   ##
==========================================
+ Coverage   33.86%   34.13%   +0.26%     
==========================================
  Files         643      643              
  Lines       44500    44512      +12     
==========================================
+ Hits        15072    15196     +124     
+ Misses      28280    28159     -121     
- Partials     1148     1157       +9

Flag	Coverage Δ
unittests	`34.13% <0.00%> (+0.26%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

XiShanYongYe-Chang · 2024-09-14T01:41:05Z

pkg/controllers/status/common.go

+			eventRecorder.Event(resource, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error())
 			return err
 		}
+		eventRecorder.Event(resource, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, "Aggregated status to object successfully.")


Why do you need to adjust the position of these two events?

I tend to add events after each resource interpretation function to reflect whether the resource interpretation is successful.

kind of justification, but I have some concerns:

the event events.EventReasonAggregateStatusSucceed is used more than this place, and thus it will have so different meaning, whether differentiation or unification is needed?

+1
but I don't have a suitable name to distinguish, do you have one?

I’m looking for references in other interpreters and found that functions like GetDependencies have an event GetDependenciesFailed, ReflectStatus has ReflectStatusFailed, and InterpretHealth has InterpretHealthFailed. These align with the meaning introduced in your PR, so naming this event AggregateStatusFailed in the position of your updated lines seems reasonable.

However, in workstatus.go, there are two place also using this event name which seem to represent successfully collected and updated status of RB. To differentiate, perhaps SyncBindingStatusFailed would be more appropriate? (ps: I'm also poor at naming.)

So you want the AggregateStatusSucceed and AggregateStatusFailed dedicated to indicating resource interpreter status? That means that updating failure after interpreter.AggregateStatus, should not report event.

yes, such update error can be fixed by reconciliation.

ping @RainbowMango

But at the time emitting the success event, the aggregate operation might still not happen, in addition, there might be more than one events due to the retry loop.

I'm afraid it is not acceptable.

other functions of the current resource interpretation framework are also designed in this way, so another purpose of this PR is to be consistent with them.

ReflectStatus:

karmada/pkg/controllers/status/work_status_controller.go

Lines 359 to 366 in 836b878

statusRaw, err := c.ResourceInterpreter.ReflectStatus(clusterObj)

if err != nil {

klog.Errorf("Failed to reflect status for object(%s/%s/%s) with resourceInterpreter, err: %v",

clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err)

c.EventRecorder.Eventf(work, corev1.EventTypeWarning, events.EventReasonReflectStatusFailed, "Reflect status for object(%s/%s/%s) failed, err: %s.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err.Error())

return err

}

c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonReflectStatusSucceed, "Reflect status for object(%s/%s/%s) succeed.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())

InterpretHealth:

karmada/pkg/controllers/status/work_status_controller.go

Lines 371 to 380 in 836b878

healthy, err := c.ResourceInterpreter.InterpretHealth(clusterObj)

if err != nil {

resourceHealth = workv1alpha1.ResourceUnknown

c.EventRecorder.Eventf(work, corev1.EventTypeWarning, events.EventReasonInterpretHealthFailed, "Interpret health of object(%s/%s/%s) failed, err: %s.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err.Error())

} else if healthy {

resourceHealth = workv1alpha1.ResourceHealthy

c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonInterpretHealthSucceed, "Interpret health of object(%s/%s/%s) as healthy.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())

} else {

resourceHealth = workv1alpha1.ResourceUnhealthy

c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonInterpretHealthSucceed, "Interpret health of object(%s/%s/%s) as unhealthy.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())

GetDependencies:

karmada/pkg/dependenciesdistributor/dependencies_distributor.go

Lines 271 to 277 in 836b878

dependencies, err := d.ResourceInterpreter.GetDependencies(workload)

if err != nil {

klog.Errorf("Failed to customize dependencies for %s(%s), %v", workload.GroupVersionKind(), workload.GetName(), err)

d.EventRecorder.Eventf(workload, corev1.EventTypeWarning, events.EventReasonGetDependenciesFailed, err.Error())

return reconcile.Result{}, err

}

d.EventRecorder.Eventf(workload, corev1.EventTypeNormal, events.EventReasonGetDependenciesSucceed, "Get dependencies(%+v) succeed.", dependencies)

add events for ResourceInterpreter

3dd93ec

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

karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 13, 2024

karmada-bot requested review from XiShanYongYe-Chang and jwcesign September 13, 2024 10:52

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 13, 2024

karmada-bot requested a review from RainbowMango September 13, 2024 10:52

XiShanYongYe-Chang reviewed Sep 14, 2024

View reviewed changes

	statusRaw, err := c.ResourceInterpreter.ReflectStatus(clusterObj)
	if err != nil {
	klog.Errorf("Failed to reflect status for object(%s/%s/%s) with resourceInterpreter, err: %v",
	clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err)
	c.EventRecorder.Eventf(work, corev1.EventTypeWarning, events.EventReasonReflectStatusFailed, "Reflect status for object(%s/%s/%s) failed, err: %s.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err.Error())
	return err
	}
	c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonReflectStatusSucceed, "Reflect status for object(%s/%s/%s) succeed.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())

	healthy, err := c.ResourceInterpreter.InterpretHealth(clusterObj)
	if err != nil {
	resourceHealth = workv1alpha1.ResourceUnknown
	c.EventRecorder.Eventf(work, corev1.EventTypeWarning, events.EventReasonInterpretHealthFailed, "Interpret health of object(%s/%s/%s) failed, err: %s.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName(), err.Error())
	} else if healthy {
	resourceHealth = workv1alpha1.ResourceHealthy
	c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonInterpretHealthSucceed, "Interpret health of object(%s/%s/%s) as healthy.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())
	} else {
	resourceHealth = workv1alpha1.ResourceUnhealthy
	c.EventRecorder.Eventf(work, corev1.EventTypeNormal, events.EventReasonInterpretHealthSucceed, "Interpret health of object(%s/%s/%s) as unhealthy.", clusterObj.GetKind(), clusterObj.GetNamespace(), clusterObj.GetName())

	dependencies, err := d.ResourceInterpreter.GetDependencies(workload)
	if err != nil {
	klog.Errorf("Failed to customize dependencies for %s(%s), %v", workload.GroupVersionKind(), workload.GetName(), err)
	d.EventRecorder.Eventf(workload, corev1.EventTypeWarning, events.EventReasonGetDependenciesFailed, err.Error())
	return reconcile.Result{}, err
	}
	d.EventRecorder.Eventf(workload, corev1.EventTypeNormal, events.EventReasonGetDependenciesSucceed, "Get dependencies(%+v) succeed.", dependencies)

add events for ResourceInterpreter #5541

Are you sure you want to change the base?

add events for ResourceInterpreter #5541

Uh oh!

Conversation

whitewindmills commented Sep 13, 2024

Uh oh!

karmada-bot commented Sep 13, 2024

Uh oh!

whitewindmills commented Sep 13, 2024

Uh oh!

codecov-commenter commented Sep 13, 2024

Codecov Report

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!

chaosi-zju Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chaosi-zju Sep 14, 2024 •

edited

Loading