add implicit priority for OP and COP

[chaunceyjiang]

Signed-off-by: chaunceyjiang [email protected]

What type of PR is this? /kind feature

What this PR does / why we need it:

add implicit priority for OP and COP

Which issue(s) this PR fixes:

like https://github.com/karmada-io/karmada/pull/2267

Special notes for your reviewer:

1. get the list of PP(or OP) that matches the workload.
2. sort the list by priority (I assume the list won't be so long, so performance won't be a concern here.)
3. for PP, we just pick the first item from the list, for OP, we apply the whole list by order.

Does this PR introduce a user-facing change?:

add implicit priority for OverridePolicy

[RainbowMango]

/assign

[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 rainbowmango after the PR has been reviewed.

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

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

[RainbowMango]

IMO, sort function could be transformed to pick up a most appropriate one. This function receives a []util.GeneralPolicy as an input and returns a int index as output. This index indicates the most appropriate one, or -1 if mismatches.

+1 I have a similar idea but haven't a clear thought yet.

By the way, I think combining PropagationPolicy and OverridePolicy might not be a good idea, my concern is they are different APIs, and the behavior is not exactly the same(or can't guarantee).

[chaunceyjiang]

sort function could be transformed to pick up a most appropriate one. This function receives a []util.GeneralPolicy as an input and returns a int index as output. This index indicates the most appropriate one, or -1 if mismatches.

🤔 OP (COP) may have several appropriate items

[chaunceyjiang]

I think combining PropagationPolicy and OverridePolicy might not be a good idea, my concern is they are different APIs, and the behavior is not exactly the same(or can't guarantee).

+1

[Garrybest]

Seems like it's not appropriate to combine OP and PP. IIRC, all matched OP should be applied while only one pp will be selected. It's totally different.

[chaunceyjiang]

Seems like it's not appropriate to combine OP and PP. IIRC, all matched OP should be applied while only one pp will be selected. It's totally different.

yes, But the SortPolicy() function in this pr only focuses on sorting, not whether pp(op) is applied

[Garrybest]

Got it. However, I thought we don't actually need a sort function indeed, against PP we pick up the most appropriate one, against OP we pick up all the appropriate ones. The complexity could be optimized from O(NlogN) to O(N).

[chaunceyjiang]

OP we pick up all the appropriate ones. The complexity could be optimized from O(NlogN) to O(N).

🤔 I don't think so.

There are two OP sample-1 and sample-2.

Apply sample-1 first, then sample-2 and sample-2 first, then sample-1.

The final result is different

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
metadata:
  name: sample-1
spec: 
  resourceSelectors: 
  - apiVersion: webapp.my.domain/v1 
    kind: Guestbook
  overrideRules: 
  - targetCluster: 
      clusterNames:
      - member1
    overriders:
      plaintext:
      - path: /spec/size
        operator: replace
        value: 4

apiVersion: policy.karmada.io/v1alpha1
kind: OverridePolicy
metadata:
  name: sample-2
spec: 
  resourceSelectors: 
  - apiVersion: webapp.my.domain/v1 
    kind: Guestbook
  overrideRules: 
  - targetCluster: 
      clusterNames:
      - member1
    overriders:
      plaintext:
      - path: /spec/size
        operator: add
        value: 4

[Garrybest]

Indeed. This is a sort case.

[codecov-commenter]

Codecov Report

Merging #2609 (200a587) into master (b0eb90b) will increase coverage by 0.09%. The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #2609      +/-   ##
==========================================
+ Coverage   37.61%   37.71%   +0.09%     
==========================================
  Files         189      200      +11     
  Lines       17657    18445     +788     
==========================================
+ Hits         6642     6956     +314     
- Misses      10612    11082     +470     
- Partials      403      407       +4     

Flag	Coverage Δ
unittests	37.71% <69.23%> (+0.09%)	: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	23.39% <69.23%> (+1.75%)	:arrow_up:
pkg/util/helper/binding.go	66.50% <0.00%> (-15.60%)	:arrow_down:
pkg/registry/cluster/storage/proxy.go	59.09% <0.00%> (-9.34%)	:arrow_down:
...armadactl/cmdinit/karmada/webhook_configuration.go	83.25% <0.00%> (-5.19%)	:arrow_down:
pkg/util/worker.go	66.66% <0.00%> (-4.77%)	:arrow_down:
pkg/util/serviceaccount.go	87.71% <0.00%> (-2.56%)	:arrow_down:
pkg/search/proxy/store/util.go	93.36% <0.00%> (-0.21%)	:arrow_down:
pkg/descheduler/descheduler.go	22.88% <0.00%> (-0.12%)	:arrow_down:
pkg/util/rbac.go	100.00% <0.00%> (ø)
pkg/util/namespace.go	100.00% <0.00%> (ø)
... and 46 more

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

[chaunceyjiang]

@Garrybest What do you think?

[Garrybest]

Generally LGTM, except this nit. https://github.com/karmada-io/karmada/pull/2609#discussion_r1028045448

[Garrybest]

/lgtm

[chaunceyjiang]

/cc @RainbowMango @XiShanYongYe-Chang

[RainbowMango]

@chaunceyjiang

add implicit priority for OP and COP

I'm asking myself, what benefits can we get from it? What's the possible use case? Can you remind me?

[chaunceyjiang]

I'm asking myself, what benefits can we get from it? What's the possible use case? Can you remind me?

I think it's reasonable, like applying the user's OP first(MatchName), then applying the administrator's OP(MatchAll or MatchLabelSelector). In that case, we might need to build a sorted OP list.

@RainbowMango

[RainbowMango]

That's a reasonable case. Thanks.

[RainbowMango]

I'll take a look it tomorrow and try my best to add this to release 1.4.

[jwcesign]

Generally, it looks good to me, cc @RainbowMango to take a look

[chaunceyjiang]

@RainbowMango @XiShanYongYe-Chang What do you think?

[RainbowMango]

Generally looks good to me. I just updated the release note:

karmada-controller-manager: Now the OverridePolicy and ClusterOverridePolicy will be applied by implicit priority order. The one with lower priority will be applied before the one with higher priority.

Please help to confirm.

[chaunceyjiang]

karmada-controller-manager: Now the OverridePolicy and ClusterOverridePolicy will be applied by implicit priority order. The one with lower priority will be applied before the one with higher priority.

+1

[karmada-bot]

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

• ~~OWNERS~~ [RainbowMango]

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

[RainbowMango]

/lgtm