Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

execute InterpretHealth even if statusRaw is nil #4453

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

chaosi-zju
Copy link
Member

@chaosi-zju chaosi-zju commented Dec 19, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

continue to execute InterpretHealth even if statusRaw is nil, otherwise, the graceful eviction controller could not parse the service's health status, the EvictionTask will be kept for 10min, details described in #4430

Which issue(s) this PR fixes:

Fixes #4430

Special notes for your reviewer:

Test Report:

status:
  aggregatedStatus:
  - applied: true
    clusterName: member1
    health: Healthy
  - applied: true
    clusterName: member2
    health: Healthy
  - applied: true
    clusterName: member3
    health: Healthy
  conditions:
  - lastTransitionTime: "2023-12-20T07:52:02Z"
    message: Binding has been scheduled successfully.
    reason: Success
    status: "True"
    type: Scheduled
  - lastTransitionTime: "2023-12-20T07:52:05Z"
    message: All works have been successfully applied
    reason: FullyAppliedSuccess
    status: "True"
    type: FullyApplied

image

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Fixed the issue that grace eviction is being blocked due to skipping InterpretHealth for resources without status.

@karmada-bot karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 19, 2023
@chaosi-zju
Copy link
Member Author

CC @XiShanYongYe-Chang @jwcesign

@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 19, 2023
Copy link
Member

@jwcesign jwcesign left a comment

Choose a reason for hiding this comment

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

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2023
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Thanks~
/lgtm
/cc @RainbowMango

@RainbowMango
Copy link
Member

After revisiting the implementation of InterpretHealth:

// InterpretHealth returns the health state of the object.
func (e *DefaultInterpreter) InterpretHealth(object *unstructured.Unstructured) (bool, error) {
klog.V(4).Infof("Get health status of object: %v %s/%s with build-in interpreter.", object.GroupVersionKind(), object.GetNamespace(), object.GetName())
handler, exist := e.healthHandlers[object.GroupVersionKind()]
if exist {
return handler(object)
}
return false, fmt.Errorf("default %s interpreter for %q not found", configv1alpha1.InterpreterOperationInterpretHealth, object.GroupVersionKind())
}

We only provided default implementations for a few workload resource types, which is reasonable as most of configuration resource types like Secret, ConfigMap, RBAC, and so on might not need to be evaluated.

So, let's assume that resources that do not implement the InterpretHealth interface are considered healthy by default.
In addition, the InterpretHealth was designed for fail-over purposes, we shouldn't require all resource types to implement the interface. What do you think? @XiShanYongYe-Chang @chaunceyjiang Does that make sense to you?

@chaunceyjiang
Copy link
Member

So, let's assume that resources that do not implement the InterpretHealth interface are considered healthy by default.
In addition, the InterpretHealth was designed for fail-over purposes, we shouldn't require all resource types to implement the interface

+1

@karmada-bot karmada-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2023
@chaosi-zju
Copy link
Member Author

update: change implemetion to continue to execute InterpretHealth even if statusRaw is nil

@XiShanYongYe-Chang
Copy link
Member

update: change implemetion to continue to execute InterpretHealth even if statusRaw is nil

/lgtm

Please help update the title.

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 20, 2023
@chaosi-zju
Copy link
Member Author

update: re-tested okay.

@chaosi-zju chaosi-zju changed the title interpreter reflectStatus as Healthy when statusRaw is nil execute InterpretHealth even if statusRaw is nil Dec 20, 2023
@jwcesign
Copy link
Member

I think we need release-notes

@chaosi-zju
Copy link
Member Author

I think we need release-notes

done

@RainbowMango
Copy link
Member

update: change implemetion to continue to execute InterpretHealth even if statusRaw is nil

That makes sense to me. No matter whether the status is empty or not, just let InterpretHealth to evaluate the healthy status.

I understand this PR focuses on fixing the issue tracked by #4430, and the improvement discussed above(#4453 (comment)) should be tracked by another issue/PR.

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@karmada-bot
Copy link
Collaborator

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

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2023
@karmada-bot karmada-bot merged commit c6b2ada into karmada-io:master Dec 20, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The service can't be propagated to the newly joined cluster if the cluster is unjoined 1 minute agao
6 participants