search

karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.12k stars 806 forks source link

add implicit priority for OP and COP #2609

Closed chaunceyjiang closed 1 year ago

chaunceyjiang commented 1 year ago

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

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

`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.
RainbowMango commented 1 year ago

/assign

RainbowMango commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Indeed. This is a sort case.

codecov-commenter commented 1 year ago

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 commented 1 year ago

@Garrybest What do you think?

Garrybest commented 1 year ago

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

Garrybest commented 1 year ago

/lgtm

chaunceyjiang commented 1 year ago

/cc @RainbowMango @XiShanYongYe-Chang

RainbowMango commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

That's a reasonable case. Thanks.

RainbowMango commented 1 year ago

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

jwcesign commented 1 year ago

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

chaunceyjiang commented 1 year ago

@RainbowMango @XiShanYongYe-Chang What do you think?

RainbowMango commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

[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](https://github.com/karmada-io/karmada/blob/master/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 commented 1 year ago

/lgtm