support PodDisruptionBudget resource in default interpreter

[a7i]

Signed-off-by: Amir Alavi amiralavi7@gmail.com

What type of PR is this?

What this PR does / why we need it:
/kind feature
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

support PodDisruptionBudget resource in default interpreter

[codecov-commenter]

Codecov Report

Merging #2997 (1eb8738) into master (c4428d6) will increase coverage by 0.09%.
The diff coverage is 65.75%.

@@            Coverage Diff             @@
##           master    #2997      +/-   ##
==========================================
+ Coverage   38.58%   38.68%   +0.09%     
==========================================
  Files         206      206              
  Lines       18820    18865      +45     
==========================================
+ Hits         7261     7297      +36     
- Misses      11129    11134       +5     
- Partials      430      434       +4     

Flag	Coverage Δ
unittests	38.68% <65.75%> (+0.09%)	⬆️

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

Impacted Files	Coverage Δ
.../resourceinterpreter/defaultinterpreter/healthy.go	65.76% <50.00%> (-1.23%)	⬇️
...einterpreter/defaultinterpreter/aggregatestatus.go	71.20% <55.26%> (-1.73%)	⬇️
...rceinterpreter/defaultinterpreter/reflectstatus.go	18.47% <85.18%> (+11.47%)	⬆️
pkg/scheduler/core/division_algorithm.go	85.71% <0.00%> (-0.38%)	⬇️
pkg/scheduler/core/assignment.go	79.76% <0.00%> (ø)
...scheduler/core/spreadconstraint/select_clusters.go	86.04% <0.00%> (+0.33%)	⬆️
pkg/search/proxy/store/util.go	93.83% <0.00%> (+0.47%)	⬆️
.../scheduler/core/spreadconstraint/group_clusters.go	95.20% <0.00%> (+2.01%)	⬆️
pkg/util/binding.go	100.00% <0.00%> (+3.89%)	⬆️
pkg/util/helper/binding.go	72.33% <0.00%> (+7.76%)	⬆️

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

[jwcesign]

Thanks for your PR @a7i , I have one question, for pdb, there is spec.minAvailable like follows:

apiVersion: policy/v1
kind: PodDisruptionBudget
metadata:
  name: zk-pdb
spec:
  minAvailable: 10
  selector:
    matchLabels:
      app: zookeeper

So this pdp will be created in all member clusters with minAvailable: 10? or will be split into different numbers for different member clusters?

[RainbowMango]

Hi @a7i, thanks for doing this.
Can you help to share your use case here?

[a7i]

Thanks for your PR @a7i , I have one question, for pdb, there is spec.minAvailable like follows:

apiVersion: policy/v1
kind: PodDisruptionBudget
metadata:
  name: zk-pdb
spec:
  minAvailable: 10
  selector:
    matchLabels:
      app: zookeeper

So this pdp will be created in all member clusters with minAvailable: 10? or will be split into different numbers for different member clusters?

@jwcesign As far as I know, that is how it is today. With or without this change. We are doing more testing on PDBs this week and I can circle back and let you know how it works with different propagation policies.

[a7i]

Hi @a7i, thanks for doing this. Can you help to share your use case here?

@RainbowMango We are conducting experiments on how Karmada treats PDBs. First blocker was that PDB status is not aggregated. A kubectl get pdb .. will yield missing or N/A results today.

[chaunceyjiang]

So this pdp will be created in all member clusters with minAvailable: 10? or will be split into different numbers for different member clusters?

At present, kamrada can only create this pdp(minAvailable: 10) in all member clusters, right? @jwcesign

[jwcesign]

At present, kamrada can only create this pdp(minAvailable: 10) in all member clusters, right? @jwcesign

I think yes, if want to make pdp work perfectly, minAvailable needs to be scheduled based on pp, the logic is like what we talk about FederationHPA. @chaunceyjiang , what u think?

[a7i]

At present, kamrada can only create this pdp(minAvailable: 10) in all member clusters, right? @jwcesign

I think yes, if want to make pdp work perfectly, minAvailable needs to be scheduled based on pp, the logic is like what we talk about FederationHPA. @chaunceyjiang , what u think?

Confirming that this is the case using v1.3.3
Currently, the percentage or number (min or max) basically creates the same resource in all member clusters, regardless of the weighted/divided configuration of pp.

[jwcesign]

Hi @a7i , in your use case, which behavior is expected? keep the same with the original pdb, or it should be scheduled based on the pp configuration?

[chaunceyjiang]

I think yes, if want to make pdp work perfectly, minAvailable needs to be scheduled based on pp, the logic is like what we talk about FederationHPA. @chaunceyjiang , what u think?

Good idea, agree with you.

[a7i]

@jwcesign My organization almost exclusively uses maxUnavailable with a percentage (e.g. 20%). Minus a few exceptions like Kafka where replica count is always fixed.

I think that for percentages, keeping the same value is valid but there are edge cases to that even:

• member-1: 2 replica Deployment
• member-2: 2 replica Deployment
• PDB maxUnavailable: 50%

Deleting a single Pod from member-1 will signal no more disruption allowed (even though at a global level, one more is still tolerable). We could report that Disruption is allowed in aggregatestatus but need to take into account what that means for operations that still happen at the member cluster level (e.g. draining a node and how it could get blocked).

I agree that handling PDB requires its own proposal (separate from HPA) but still feel that this PR can be reviewed since it's only aggregating status.

I (or someone from my org) would be more than happy to engage in discussions/designs for the proposal.

[jwcesign]

Yeah, it should work fine with percentages.

I am ok with this PR(only aggregating status), cc @XiShanYongYe-Chang for checking

[XiShanYongYe-Chang]

Hi @a7i, thanks for your contribution!
Can you help add an E2E test in the https://github.com/karmada-io/karmada/blob/master/test/e2e/resource_test.go file for the PodDisruptionBudget resource?

[a7i]

Just requested another review from @XiShanYongYe-Chang , didn't intend to remove.

cc: @RainbowMango

[XiShanYongYe-Chang]

Just some nits, other parts look good to me, thanks a lot~

[karmada-bot]

@a7i: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[XiShanYongYe-Chang]

Thanks a lot.
/lgtm
Ask @RainbowMango for a review.

[RainbowMango]

/approve

[RainbowMango]

nice finding.

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

I just opened a task(karmada-io/website#287) to track the document updation.