add implicit priority for OP and COP #2609

chaunceyjiang · 2022-10-03T05:44:10Z

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 #2267

Special notes for your reviewer:

get the list of PP(or OP) that matches the workload.
sort the list by priority (I assume the list won't be so long, so performance won't be a concern here.)
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 · 2022-10-08T03:11:49Z

/assign

pkg/util/selector.go

Garrybest

Well, this PR can really work but also a little complicated. I suppose we actually don't need to sort this slice, the only thing we need is to find the most appropriate one, right?

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.

How do you think?

RainbowMango · 2022-10-10T03:06:23Z

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 · 2022-10-10T03:45:43Z

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 · 2022-10-10T03:49:35Z

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 · 2022-10-10T03:51:29Z

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 · 2022-10-10T06:16:34Z

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 · 2022-10-10T06:59:29Z

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 · 2022-10-10T12:25:52Z

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 · 2022-10-11T01:48:39Z

Indeed. This is a sort case.

codecov-commenter · 2022-11-15T14:25:40Z

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%)`	⬆️

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%)`	⬆️
pkg/util/helper/binding.go	`66.50% <0.00%> (-15.60%)`	⬇️
pkg/registry/cluster/storage/proxy.go	`59.09% <0.00%> (-9.34%)`	⬇️
...armadactl/cmdinit/karmada/webhook_configuration.go	`83.25% <0.00%> (-5.19%)`	⬇️
pkg/util/worker.go	`66.66% <0.00%> (-4.77%)`	⬇️
pkg/util/serviceaccount.go	`87.71% <0.00%> (-2.56%)`	⬇️
pkg/search/proxy/store/util.go	`93.36% <0.00%> (-0.21%)`	⬇️
pkg/descheduler/descheduler.go	`22.88% <0.00%> (-0.12%)`	⬇️
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 · 2022-11-16T02:01:03Z

@Garrybest What do you think?

pkg/util/overridemanager/overridemanager.go

Garrybest · 2022-11-22T07:56:12Z

Generally LGTM, except this nit. #2609 (comment)

Garrybest · 2022-11-23T00:19:09Z

/lgtm

pkg/util/overridemanager/overridemanager.go

RainbowMango · 2022-11-28T07:41:48Z

@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 · 2022-11-28T08:05:32Z

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 · 2022-11-28T10:11:18Z

That's a reasonable case. Thanks.

RainbowMango · 2022-11-28T10:12:06Z

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

pkg/util/overridemanager/overridemanager.go

pkg/util/selector.go

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

jwcesign · 2022-12-05T07:11:16Z

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

chaunceyjiang · 2022-12-05T13:02:59Z

@RainbowMango @XiShanYongYe-Chang What do you think?

RainbowMango · 2022-12-06T02:09:29Z

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 · 2022-12-06T02:31:41Z

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

RainbowMango

/approve

Thanks for the quick response.

karmada-bot · 2022-12-06T02:32:58Z

[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 · 2022-12-06T02:33:32Z

/lgtm

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 3, 2022

karmada-bot requested review from Garrybest and XiShanYongYe-Chang October 3, 2022 05:44

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 3, 2022

karmada-bot assigned RainbowMango Oct 8, 2022

RainbowMango added this to the v1.4 milestone Oct 8, 2022

RainbowMango reviewed Oct 8, 2022

View reviewed changes

pkg/util/selector.go Outdated Show resolved Hide resolved

Garrybest suggested changes Oct 10, 2022

View reviewed changes

chaunceyjiang force-pushed the implicit branch from dc992f3 to 7bc59ba Compare November 15, 2022 14:06

karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2022

chaunceyjiang force-pushed the implicit branch from 7bc59ba to c7e27a0 Compare November 15, 2022 14:10

chaunceyjiang force-pushed the implicit branch from c7e27a0 to 20321ed Compare November 15, 2022 15:00

chaunceyjiang requested review from Garrybest and removed request for XiShanYongYe-Chang November 16, 2022 02:01

Garrybest reviewed Nov 21, 2022

View reviewed changes

pkg/util/overridemanager/overridemanager.go Outdated Show resolved Hide resolved

pkg/util/overridemanager/overridemanager.go Show resolved Hide resolved

chaunceyjiang force-pushed the implicit branch from 20321ed to f41df14 Compare November 22, 2022 08:05

karmada-bot requested review from RainbowMango and XiShanYongYe-Chang November 23, 2022 08:34

jwcesign reviewed Nov 28, 2022

View reviewed changes

pkg/util/overridemanager/overridemanager.go Show resolved Hide resolved

jwcesign reviewed Nov 28, 2022

View reviewed changes

pkg/util/overridemanager/overridemanager.go Outdated Show resolved Hide resolved

chaunceyjiang force-pushed the implicit branch from f41df14 to 1727742 Compare November 28, 2022 11:36

karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2022

chaunceyjiang commented Nov 28, 2022

View reviewed changes

pkg/util/overridemanager/overridemanager.go Show resolved Hide resolved

chaunceyjiang force-pushed the implicit branch from 1727742 to ace6f3c Compare November 28, 2022 13:02

karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 28, 2022

Garrybest reviewed Nov 29, 2022

View reviewed changes

pkg/util/selector.go Outdated Show resolved Hide resolved

add implicit priority for OP and COP

200a587

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

chaunceyjiang force-pushed the implicit branch from ace6f3c to 200a587 Compare November 29, 2022 03:27

RainbowMango modified the milestones: v1.4, v1.5 Nov 29, 2022

RainbowMango approved these changes Dec 6, 2022

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2022

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2022

karmada-bot merged commit 9784ed3 into karmada-io:master Dec 6, 2022

chaunceyjiang deleted the implicit branch December 19, 2022 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add implicit priority for OP and COP #2609

add implicit priority for OP and COP #2609

chaunceyjiang commented Oct 3, 2022 •

edited by RainbowMango

Loading

RainbowMango commented Oct 8, 2022

Garrybest left a comment

RainbowMango commented Oct 10, 2022

chaunceyjiang commented Oct 10, 2022

chaunceyjiang commented Oct 10, 2022

Garrybest commented Oct 10, 2022

chaunceyjiang commented Oct 10, 2022 •

edited

Loading

Garrybest commented Oct 10, 2022 •

edited

Loading

chaunceyjiang commented Oct 10, 2022

Garrybest commented Oct 11, 2022

codecov-commenter commented Nov 15, 2022 •

edited

Loading

chaunceyjiang commented Nov 16, 2022

Garrybest commented Nov 22, 2022

Garrybest commented Nov 23, 2022

RainbowMango commented Nov 28, 2022

chaunceyjiang commented Nov 28, 2022 •

edited

Loading

RainbowMango commented Nov 28, 2022

RainbowMango commented Nov 28, 2022

jwcesign commented Dec 5, 2022

chaunceyjiang commented Dec 5, 2022 •

edited

Loading

RainbowMango commented Dec 6, 2022

chaunceyjiang commented Dec 6, 2022

RainbowMango left a comment

karmada-bot commented Dec 6, 2022

RainbowMango commented Dec 6, 2022

add implicit priority for OP and COP #2609

add implicit priority for OP and COP #2609

Conversation

chaunceyjiang commented Oct 3, 2022 • edited by RainbowMango Loading

RainbowMango commented Oct 8, 2022

Garrybest left a comment

Choose a reason for hiding this comment

RainbowMango commented Oct 10, 2022

chaunceyjiang commented Oct 10, 2022

chaunceyjiang commented Oct 10, 2022

Garrybest commented Oct 10, 2022

chaunceyjiang commented Oct 10, 2022 • edited Loading

Garrybest commented Oct 10, 2022 • edited Loading

chaunceyjiang commented Oct 10, 2022

Garrybest commented Oct 11, 2022

codecov-commenter commented Nov 15, 2022 • edited Loading

Codecov Report

chaunceyjiang commented Nov 16, 2022

Garrybest commented Nov 22, 2022

Garrybest commented Nov 23, 2022

RainbowMango commented Nov 28, 2022

chaunceyjiang commented Nov 28, 2022 • edited Loading

RainbowMango commented Nov 28, 2022

RainbowMango commented Nov 28, 2022

jwcesign commented Dec 5, 2022

chaunceyjiang commented Dec 5, 2022 • edited Loading

RainbowMango commented Dec 6, 2022

chaunceyjiang commented Dec 6, 2022

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Dec 6, 2022

RainbowMango commented Dec 6, 2022

chaunceyjiang commented Oct 3, 2022 •

edited by RainbowMango

Loading

chaunceyjiang commented Oct 10, 2022 •

edited

Loading

Garrybest commented Oct 10, 2022 •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading

chaunceyjiang commented Nov 28, 2022 •

edited

Loading

chaunceyjiang commented Dec 5, 2022 •

edited

Loading