Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement propagation policy priority preemption #3837

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

whitewindmills
Copy link
Member

What type of PR is this?
/kind feature

What this PR does / why we need it:
implement propagation policy preemption: high-priority pp/cpp -> low-priority pp/cpp.

Which issue(s) this PR fixes:
part of feature

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 25, 2023
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2023
@whitewindmills
Copy link
Member Author

/cc @Poor12

@karmada-bot karmada-bot requested a review from Poor12 July 25, 2023 11:39
@codecov-commenter
Copy link

Codecov Report

Merging #3837 (3324efa) into master (6ef427a) will decrease coverage by 0.34%.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #3837      +/-   ##
==========================================
- Coverage   55.50%   55.17%   -0.34%     
==========================================
  Files         226      227       +1     
  Lines       21457    21606     +149     
==========================================
+ Hits        11910    11921      +11     
- Misses       8911     9049     +138     
  Partials      636      636              
Flag Coverage Δ
unittests 55.17% <0.00%> (-0.34%) ⬇️

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

Files Changed Coverage Δ
pkg/detector/detector.go 0.00% <0.00%> (ø)
pkg/detector/preemption.go 0.00% <0.00%> (ø)
pkg/util/helper/binding.go 69.32% <0.00%> (-1.99%) ⬇️
pkg/util/names/names.go 91.01% <0.00%> (-5.42%) ⬇️

... and 1 file with indirect coverage changes

pkg/util/names/names.go Show resolved Hide resolved
pkg/detector/preemption.go Outdated Show resolved Hide resolved
pkg/detector/preemption.go Outdated Show resolved Hide resolved
pkg/detector/detector.go Outdated Show resolved Hide resolved
pkg/detector/preemption.go Outdated Show resolved Hide resolved
@whitewindmills
Copy link
Member Author

PTAL @RainbowMango @Poor12

@Poor12
Copy link
Member

Poor12 commented Jul 26, 2023

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2023
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested it? Can you share the test result here?

pkg/detector/detector.go Outdated Show resolved Hide resolved
pkg/detector/preemption.go Outdated Show resolved Hide resolved
@whitewindmills
Copy link
Member Author

Have you tested it? Can you share the test result here?

Yeah, I'll paste it here.

@whitewindmills
Copy link
Member Author

  1. create configmap test-cm
apiVersion: v1
data:
  a: b
kind: ConfigMap
metadata:
  creationTimestamp: "2023-07-26T09:14:00Z"
  name: test-cm
  namespace: default
  resourceVersion: "3256197"
  uid: f7bb95ad-a79a-428e-b25c-f44d474ce4a5
  1. create low-priority pp test-0
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  creationTimestamp: "2023-07-26T10:31:51Z"
  generation: 1
  name: test-0
  namespace: default
  resourceVersion: "3317010"
  uid: 83479ff3-399d-407c-a0f1-6aba1a1babd1
spec:
  conflictResolution: Abort
  placement:
    clusterTolerations:
    - effect: NoExecute
      key: cluster.karmada.io/not-ready
      operator: Exists
      tolerationSeconds: 300
    - effect: NoExecute
      key: cluster.karmada.io/unreachable
      operator: Exists
      tolerationSeconds: 300
    replicaScheduling:
      replicaDivisionPreference: Weighted
      replicaSchedulingType: Duplicated
  preemption: Never
  priority: 0
  resourceSelectors:
  - apiVersion: v1
    kind: ConfigMap
    name: test-cm
    namespace: default
  schedulerName: default-scheduler
  1. configmap test-cm was bound to pp test-0
    image
  2. create high-priority pp test-1
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  creationTimestamp: "2023-07-26T10:32:56Z"
  generation: 1
  name: test-1
  namespace: default
  resourceVersion: "3317883"
  uid: c494a725-87ba-4799-8737-71e66177f6bb
spec:
  conflictResolution: Abort
  placement:
    clusterTolerations:
    - effect: NoExecute
      key: cluster.karmada.io/not-ready
      operator: Exists
      tolerationSeconds: 300
    - effect: NoExecute
      key: cluster.karmada.io/unreachable
      operator: Exists
      tolerationSeconds: 300
    replicaScheduling:
      replicaDivisionPreference: Weighted
      replicaSchedulingType: Divided
  preemption: Always
  priority: 1
  resourceSelectors:
  - apiVersion: v1
    kind: ConfigMap
    name: test-cm
    namespace: default
  schedulerName: default-scheduler
  1. policy test-1 preempts policy test-0
    image

related logs:
image

Signed-off-by: whitewindmills <jayfantasyhjh@gmail.com>
@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2023
@whitewindmills
Copy link
Member Author

for CPP, got the same result.

  1. create low-priority cpp test-1
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  creationTimestamp: "2023-07-26T10:47:23Z"
  generation: 1
  name: test-1
  resourceVersion: "3328889"
  uid: 28b41377-3515-4699-a693-dc527688c5c9
spec:
  conflictResolution: Abort
  placement:
    clusterTolerations:
    - effect: NoExecute
      key: cluster.karmada.io/not-ready
      operator: Exists
      tolerationSeconds: 300
    - effect: NoExecute
      key: cluster.karmada.io/unreachable
      operator: Exists
      tolerationSeconds: 300
    replicaScheduling:
      replicaSchedulingType: Duplicated
  preemption: Never
  priority: 0
  resourceSelectors:
  - apiVersion: rbac.authorization.k8s.io/v1
    kind: ClusterRole
    name: test-role
  schedulerName: default-scheduler
  1. create high-priority cpp test-2 to preempts test-1
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  creationTimestamp: "2023-07-26T10:50:08Z"
  generation: 1
  name: test-2
  resourceVersion: "3331043"
  uid: 4bb52f19-2cf9-41f0-9c41-0ae03efcaf68
spec:
  conflictResolution: Abort
  placement:
    clusterTolerations:
    - effect: NoExecute
      key: cluster.karmada.io/not-ready
      operator: Exists
      tolerationSeconds: 300
    - effect: NoExecute
      key: cluster.karmada.io/unreachable
      operator: Exists
      tolerationSeconds: 300
  preemption: Always
  priority: 1
  resourceSelectors:
  - apiVersion: rbac.authorization.k8s.io/v1
    kind: ClusterRole
    name: test-role
  schedulerName: default-scheduler
  1. preemption result:
    image

related logs:
image

@whitewindmills
Copy link
Member Author

PLAT @Poor12 @RainbowMango

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

Leave LGTM to @Poor12

@karmada-bot
Copy link
Collaborator

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

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2023
@Poor12
Copy link
Member

Poor12 commented Jul 27, 2023

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2023
@karmada-bot karmada-bot merged commit f160ea7 into karmada-io:master Jul 27, 2023
11 checks passed
@whitewindmills whitewindmills deleted the priority_preemption branch July 27, 2023 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants