refactor resourceinterpreter framework

[chaunceyjiang]

Signed-off-by: chaunceyjiang chaunceyjiang@gmail.com

What type of PR is this? /kind cleanup

What this PR does / why we need it:

refactor resourceinterpreter framework

Which issue(s) this PR fixes: Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[codecov-commenter]

Codecov Report

Merging #2766 (e9d9e6a) into master (31f97ac) will increase coverage by 0.01%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2766      +/-   ##
==========================================
+ Coverage   33.46%   33.48%   +0.01%     
==========================================
  Files         200      200              
  Lines       19636    19642       +6     
==========================================
+ Hits         6572     6577       +5     
- Misses      12669    12671       +2     
+ Partials      395      394       -1     

Flag	Coverage Δ
unittests	33.48% <ø> (+0.01%)	:arrow_up:

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

Impacted Files	Coverage Δ
pkg/util/overridemanager/overridemanager.go	21.63% <0.00%> (-0.54%)	:arrow_down:
pkg/karmadactl/cordon.go	0.00% <0.00%> (ø)
pkg/search/proxy/store/util.go	94.73% <0.00%> (+1.16%)	:arrow_up:
pkg/util/worker.go	71.42% <0.00%> (+4.76%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[chaunceyjiang]

/cc @XiShanYongYe-Chang @jameszhangyukun @ikaven1024

[karmada-bot]

@chaunceyjiang: GitHub didn't allow me to request PR reviews from the following users: jameszhangyukun.

Note that only karmada-io members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/karmada-io/karmada/pull/2766#issuecomment-1308284207): >/cc @XiShanYongYe-Chang @jameszhangyukun @ikaven1024 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[XiShanYongYe-Chang]

/assign

[chaunceyjiang]

Can you have a look? @XiShanYongYe-Chang @RainbowMango @ikaven1024

[ikaven1024]

Aha, I am also doing this refactor. Except the changes in your PR, I have other changes:

1. Remove the HookEnabled function. I found it's unnecessary.
2. Use Chain of responsibility DP, simplify the code in customResourceInterpreter

Give it for your reference： https://github.com/karmada-io/karmada/compare/master...ikaven1024:karmada:refactor-resourceintepret-framework

[chaunceyjiang]

Remove the HookEnabled function. I found it's unnecessary.

I think HookEnabled can be used to reduce unnecessary function calls.

[XiShanYongYe-Chang]

1. Remove the HookEnabled function. I found it's unnecessary.

IMOP, In terms of code readability and interface coupling, I think it's necessary to keep HookEnable.

2. Use Chain of responsibility DP, simplify the code in customResourceInterpreter

I agree with this point! It is also possible to continue reconstruction later.

[ikaven1024]

I think HookEnabled can be used to reduce unnecessary function calls.

workflow1:

hookEnabled =  A.HookEnabled() && B.HookEnabled() && C.HookEnabled()
if  hookEnabled {
    if A.HookEnabled() {
        A.Xxx()
        return
    }

    if B.HookEnabled() {
        B.Xxx()
        return
    }

    if C.HookEnabled() {
        C.Xxx()
        return
    }
}

workflow2:

if A.HookEnabled() {
    A.Xxx()
    return
}

if B.HookEnabled() {
    B.Xxx()
    return
}

if C.HookEnabled() {
    C.Xxx()
    return
}

The later is simple, no additional calling.

[chaunceyjiang]

The refactor logic should be as follow.

hookEnabled =  A.HookEnabled() || B.HookEnabled() || C.HookEnabled()
if  hookEnabled {
    if A.HookEnabled() {       
        A.Xxx()
        return
    }

    if B.HookEnabled() {
        B.Xxx()
        return
    }

    if C.HookEnabled() {
        C.Xxx()
        return
    }

    throw an exception.
}

[ikaven1024]

The refactor logic should be as follow.

So why need you check HookEnabled twice?

What's wrong with below code? It avoids throw an exception

if A.HookEnabled() {
    A.Xxx()
    return
}

if B.HookEnabled() {
    B.Xxx()
    return
}

if C.HookEnabled() {
    C.Xxx()
    return
}

[chaunceyjiang]

https://github.com/karmada-io/karmada/blob/5a767f4f219c8181aaffeaa0b5fbfb732bd38c55/pkg/detector/detector.go#L623-L634

If the GVK does not implement GetReplicas and you delete the code in line 623 as you said, the replicas in line 624 will be ambiguous.

[ikaven1024]

replicas, replicaRequirements, err := d.ResourceInterpreter.GetReplicas(object)

So I change it to

        replicas, replicaRequirements, handled, err := d.ResourceInterpreter.GetReplicas(object) 
        if err != nil { ... }
        if handled {
               propagationBinding.Spec.Replicas = replicas 
           propagationBinding.Spec.ReplicaRequirements = replicaRequirements 
        }
        return propagationBinding, nil 

[chaunceyjiang]

replicas, replicaRequirements, err := d.ResourceInterpreter.GetReplicas(object)

So I change it to

        replicas, replicaRequirements, handled, err := d.ResourceInterpreter.GetReplicas(object) 
        if err != nil { ... }
        if handled {
               propagationBinding.Spec.Replicas = replicas 
         propagationBinding.Spec.ReplicaRequirements = replicaRequirements 
        }
        return propagationBinding, nil 

So do you understand what I mean? I think my pr may be better to read. 😊

[ikaven1024]

I think my pr may be better to read. 😊

When call HookEnabled, configurableInterpreter returns true. Then the rule is deleted. Then call configurableInterpreter's GetReplicas, configurableInterpreter can't handle it, and will throw an error.

While this error will not occur in my code.

[chaunceyjiang]

When call HookEnabled, configurableInterpreter returns true. Then the rule is deleted.

Yes, That's why I wrote throw an exception.

will panic

where?

[ikaven1024]

will panic

where?

Sorry, my expression is not clear. I update my comment.

[chaunceyjiang]

Sounds good, You can submit a pr and let's have a look

[chaunceyjiang]

Give it for your reference： https://github.com/karmada-io/karmada/compare/master...ikaven1024:karmada:refactor-resourceintepret-framework

I get it, I'll study it

[XiShanYongYe-Chang]

Hi @chaunceyjiang , maybe we need to wait for #2794.

[RainbowMango]

@chaunceyjiang I moved this to the next release, if you still think it is needed, please solve the conflicts and push again.

[karmada-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please ask for approval from xishanyongye-chang and additionally assign ikaven1024 after the PR has been reviewed. You can assign the PR to them by writing /assign @ikaven1024 in a comment when ready.

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

Needs approval from an approver in each of these files: - **[pkg/karmadactl/interpret/OWNERS](https://github.com/karmada-io/karmada/blob/master/pkg/karmadactl/interpret/OWNERS)** - **[pkg/resourceinterpreter/OWNERS](https://github.com/karmada-io/karmada/blob/master/pkg/resourceinterpreter/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[ikaven1024]

LGTM

[chaunceyjiang]

I'll think about what we are going to do to the resource interpreter feature, my concern is what if we are going to add features only to the configurable interpreter?

Get it. I think your concern is correct.

By the way, if we introduce this feature and do not refactor resourceinterpreter framework, the current implementation of ResourceInterpreter will be more complex.

[ikaven1024]

It literally simplified the implementation of ResourceInterpreter but also requires each interpreter to keep the same behavior/interface.

I'll think about what we are going to do to the resource interpreter feature, my concern is what if we are going to add features only to the configurable interpreter?

IMO, the behaviors is determined by implement of our controllers, rather the ways of interpreting, by far all the interpreter shall have the same behaviors.

[RainbowMango]

@chaunceyjiang Could you please confirm if we still need this PR?

[RainbowMango]

/close Please reopen it if we still need this.

[karmada-bot]

@RainbowMango: Closed this PR.

In response to [this](https://github.com/karmada-io/karmada/pull/2766#issuecomment-1607411914): >/close >Please reopen it if we still need this. Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.