skip interpret health of resources without a hook #5530

a7i · 2024-09-11T20:23:37Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
In one of our small staging clusters, we get a lot of InterpretHealthFailed events (over 100k per week). After investigations, it seems to be resources such as ClusterRole and ClusterRoleBinding that never have a status, so there is nothing to inspect.

This is also repeated over and over again in karmada-controller-manager logs:

"Interpret health of object(ClusterRole//oncall) failed, err: default InterpretHealth interpreter for \"rbac.authorization.k8s.io/v1, Kind=ClusterRole\" not found." logger="events" type="Warning" object={"kind":"Work","namespace":"karmada-es-REDACTED","name":"oncall-76bd899f7f","uid":"7a93a59e-3196-4192-975c-ccdc3dd5e6f7","apiVersion":"work.karmada.io/v1alpha1","resourceVersion":"6603066766"} reason="InterpretHealthFailed"

**Which issue(s)

is PR fixes**:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: The health status of resources without ResourceInterpreter customization will be treated as healthy by default.

pkg/resourceinterpreter/default/native/default.go

codecov-commenter · 2024-09-11T20:36:03Z

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

Codecov Report

Attention: Patch coverage is 52.94118% with 8 lines in your changes missing coverage. Please review.

Project coverage is 35.22%. Comparing base (62ae95e) to head (dbed854).

Files with missing lines	Patch %	Lines
pkg/controllers/status/work_status_controller.go	52.94%	5 Missing and 3 partials ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5530      +/-   ##
==========================================
+ Coverage   35.21%   35.22%   +0.01%     
==========================================
  Files         645      645              
  Lines       44885    44891       +6     
==========================================
+ Hits        15806    15814       +8     
+ Misses      27847    27842       -5     
- Partials     1232     1235       +3

Flag	Coverage Δ
unittests	`35.22% <52.94%> (+0.01%)`	⬆️

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.

whitewindmills · 2024-09-12T02:07:21Z

@a7i
we cannot stop interpreting health for resources without status. see #4453.
anyway, this is definitely worth optimizing, you might consider skipping it if InterpretHealth is not enabled.

if !c.ResourceInterpreter.HookEnabled(resourceKey.GroupVersionKind(), configv1alpha1.InterpreterOperationInterpretHealth) {
	// skip interpreting health ....
}

XiShanYongYe-Chang

I think it's reasonable.
LGTM

pkg/resourceinterpreter/default/native/default.go

XiShanYongYe-Chang · 2024-09-12T02:30:56Z

we cannot stop interpreting health for resources without status. see #4453.

Sorry, I don't understand. The current modification does not skip the health parsing step, but directly considers health as true when the object does not have status.

whitewindmills · 2024-09-12T06:12:38Z

The current modification does not skip the health parsing step, but directly considers health as true when the object does not have status.

it's fake health. we should avoid modifying the default interpreter if there are no new features or bug fixes, and we should have called HookEnabled before calling InterpretHealth, which would have solved the problem as well, wouldn't it?

whitewindmills · 2024-09-12T06:17:03Z

in addition, it seems that we have no regulations that do not interpret the health of resources without status. maybe there will be such resources in the future.

XiShanYongYe-Chang · 2024-09-12T06:22:28Z

in addition, it seems that we have no regulations that do not interpret the health of resources without status.

That's the point. So logically, this pr modification is correct, but there is an alternative, which is to call the HookEnabled method, which I think is OK. How do you think @a7i ?

chaosi-zju · 2024-09-12T07:26:25Z

we cannot stop interpreting health for resources without status. see #4453.

it seems that we have no regulations that do not interpret the health of resources without status. maybe there will be such resources in the future.

I agree with what @whitewindmills said, it is quite right.

What problem #4453 resolved is, Service kind resource, it has InterpretHealth hook, but its InterpretHealth hook has no chance to run due to its status always reflected as nil by reflectStatus.

Service kind resource is exactly the resource who should interpret the health but without status. If this PR introduced, it will again has no change to run InterpretHealth since it always interpreted as Healthy.

updated:

I made a mistake, Service actually has status field, it is just status reflected as nil (so this PR doesn't affect it).
But that doesn't mean there won't be such resources in the future.
Resource without InterpretHealth hook enabled be treated as healthy is more reasonable.

RainbowMango · 2024-09-12T07:31:13Z

Thanks @a7i for bringing this up, and thanks to @whitewindmills for the reminder that we have revisited the IntepretHealth design at #4453 (comment).

The agreement we made at that time was: The resources which do not implement InterpretHealth should be treated as healthy by default. I was hoping to make some improvement after #4453, but unfortunately I forgot it! (Maybe next time I will require the track PR before approving :))

Speaking of the improvements, I agree with @whitewindmills that we should skip evaluating those resources that don't implement InterpretHealth, treat them as healthy by default, and should avoid unexpected, surprising warnings.

a7i · 2024-09-13T16:55:12Z

Thanks for the feedback. Pushed up a commit, let me know what you think!

pkg/controllers/status/work_status_controller.go

XiShanYongYe-Chang · 2024-09-14T01:34:11Z

pkg/controllers/status/work_status_controller.go

+	} 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())


One question, do we still need to report the event in the skip situation?

Good question, open to feedback. I personally would prefer if we didn't record the event

cc @RainbowMango

No evidence shows we need it yet, we can skip the event for this PR.

a7i · 2024-09-17T19:03:34Z

Hi all, any feedback?

whitewindmills · 2024-09-18T02:13:26Z

lgtm
/retest

chaosi-zju · 2024-09-18T02:14:24Z

LGTM

RainbowMango

/assign

RainbowMango

Generally looks good to me!
I like the idea that have a dedicated methods for healthy assessment very much! Thank you!

RainbowMango · 2024-09-18T02:59:02Z

pkg/controllers/status/work_status_controller.go

+	} 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())


No evidence shows we need it yet, we can skip the event for this PR.

pkg/controllers/status/work_status_controller.go

Signed-off-by: Amir Alavi <amiralavi7@gmail.com>

RainbowMango

/lgtm
/approve

karmada-bot · 2024-10-08T08:40:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~pkg/controllers/OWNERS~~ [RainbowMango]

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

karmada-bot requested review from chaunceyjiang and iawia002 September 11, 2024 20:23

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 11, 2024

a7i commented Sep 11, 2024

View reviewed changes

pkg/resourceinterpreter/default/native/default.go Outdated Show resolved Hide resolved

XiShanYongYe-Chang reviewed Sep 12, 2024

View reviewed changes

pkg/resourceinterpreter/default/native/default.go Outdated Show resolved Hide resolved

a7i force-pushed the skip-interpret-health branch from c8f45f0 to 8e50532 Compare September 13, 2024 16:54

a7i changed the title ~~skip interpret health of resources without .status~~ skip interpret health of resources without a hook Sep 13, 2024

a7i commented Sep 13, 2024

View reviewed changes

pkg/controllers/status/work_status_controller.go Show resolved Hide resolved

a7i force-pushed the skip-interpret-health branch from 8e50532 to ba77642 Compare September 13, 2024 18:11

XiShanYongYe-Chang reviewed Sep 14, 2024

View reviewed changes

RainbowMango reviewed Sep 18, 2024

View reviewed changes

karmada-bot assigned RainbowMango Sep 18, 2024

RainbowMango reviewed Sep 18, 2024

View reviewed changes

skip interpret health of resources without a hook

dbed854

Signed-off-by: Amir Alavi <amiralavi7@gmail.com>

a7i force-pushed the skip-interpret-health branch from ba77642 to dbed854 Compare October 5, 2024 15:10

a7i requested review from RainbowMango and XiShanYongYe-Chang October 5, 2024 15:10

RainbowMango approved these changes Oct 8, 2024

View reviewed changes

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

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

karmada-bot merged commit f656d9a into karmada-io:master Oct 8, 2024
12 checks passed

a7i deleted the skip-interpret-health branch October 8, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip interpret health of resources without a hook #5530

skip interpret health of resources without a hook #5530

a7i commented Sep 11, 2024 •

edited by RainbowMango

Loading

codecov-commenter commented Sep 11, 2024 •

edited

Loading

whitewindmills commented Sep 12, 2024

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang commented Sep 12, 2024

whitewindmills commented Sep 12, 2024 •

edited

Loading

whitewindmills commented Sep 12, 2024

XiShanYongYe-Chang commented Sep 12, 2024

chaosi-zju commented Sep 12, 2024 •

edited

Loading

RainbowMango commented Sep 12, 2024

a7i commented Sep 13, 2024

XiShanYongYe-Chang Sep 14, 2024

a7i Sep 14, 2024

XiShanYongYe-Chang Sep 18, 2024

RainbowMango Sep 18, 2024

a7i commented Sep 17, 2024

whitewindmills commented Sep 18, 2024

chaosi-zju commented Sep 18, 2024

RainbowMango left a comment

RainbowMango left a comment

RainbowMango Sep 18, 2024

RainbowMango left a comment

karmada-bot commented Oct 8, 2024

skip interpret health of resources without a hook #5530

skip interpret health of resources without a hook #5530

Conversation

a7i commented Sep 11, 2024 • edited by RainbowMango Loading

codecov-commenter commented Sep 11, 2024 • edited Loading

Codecov Report

whitewindmills commented Sep 12, 2024

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented Sep 12, 2024

whitewindmills commented Sep 12, 2024 • edited Loading

whitewindmills commented Sep 12, 2024

XiShanYongYe-Chang commented Sep 12, 2024

chaosi-zju commented Sep 12, 2024 • edited Loading

RainbowMango commented Sep 12, 2024

a7i commented Sep 13, 2024

XiShanYongYe-Chang Sep 14, 2024

Choose a reason for hiding this comment

a7i Sep 14, 2024

Choose a reason for hiding this comment

XiShanYongYe-Chang Sep 18, 2024

Choose a reason for hiding this comment

RainbowMango Sep 18, 2024

Choose a reason for hiding this comment

a7i commented Sep 17, 2024

whitewindmills commented Sep 18, 2024

chaosi-zju commented Sep 18, 2024

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

RainbowMango Sep 18, 2024

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Oct 8, 2024

a7i commented Sep 11, 2024 •

edited by RainbowMango

Loading

codecov-commenter commented Sep 11, 2024 •

edited

Loading

whitewindmills commented Sep 12, 2024 •

edited

Loading

chaosi-zju commented Sep 12, 2024 •

edited

Loading