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

Correct the issue of adding duplicate eviction tasks #3456

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

jwcesign
Copy link
Member

What type of PR is this?
/kind bug

What this PR does / why we need it:
1.apply following yaml:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: nginx
  labels:
    app: nginx
spec:
  replicas: 3
  selector:
    matchLabels:
      app: nginx
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - image: nginx
        name: nginx
---
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  name: nginx-propagation
spec:
  resourceSelectors:
    - apiVersion: apps/v1
      kind: Deployment
      name: nginx
  placement:
    clusterAffinity:
      clusterNames:
        - member1
        - member2
      clusterTolerations:
      - effect: NoExecute
        key: fail-test
        operator: Exists
        tolerationSeconds: 10
    replicaScheduling:
      replicaDivisionPreference: Weighted
      replicaSchedulingType: Divided
      weightPreference:
        staticWeightList:
          - targetCluster:
              clusterNames:
                - member1
            weight: 1
          - targetCluster:
              clusterNames:
                - member2
            weight: 2

2.taint the cluster

karmadactl taint cluster member2 fail-test=:NoExecute

3.check rb

  spec:
    clusters:
    - name: member2
      replicas: 2
    - name: member1
      replicas: 1
    gracefulEvictionTasks:
    - fromCluster: member2
      producer: TaintManager
      reason: TaintUntolerated
      replicas: 2
    - fromCluster: member2
      producer: TaintManager
      reason: TaintUntolerated
      replicas: 2
    - fromCluster: member2
      producer: TaintManager
      reason: TaintUntolerated
      replicas: 2

The tasks will be added multiple times.

Which issue(s) this PR fixes:
Fixes #none

Special notes for your reviewer:
@XiShanYongYe-Chang

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Correct the issue of adding duplicate eviction tasks.

@karmada-bot karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 23, 2023
@karmada-bot karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Apr 23, 2023
@jwcesign
Copy link
Member Author

/remove-kind bug
/kind cleanup

@karmada-bot karmada-bot removed the kind/bug Categorizes issue or PR as related to a bug. label Apr 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2023

Codecov Report

Merging #3456 (e1b9e17) into master (b118a73) will increase coverage by 1.96%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #3456      +/-   ##
==========================================
+ Coverage   51.78%   53.75%   +1.96%     
==========================================
  Files         210      210              
  Lines       18974    19118     +144     
==========================================
+ Hits         9826    10276     +450     
+ Misses       8605     8290     -315     
- Partials      543      552       +9     
Flag Coverage Δ
unittests 53.75% <100.00%> (+1.96%) ⬆️

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

Impacted Files Coverage Δ
pkg/apis/work/v1alpha2/binding_types_helper.go 100.00% <100.00%> (ø)

... and 9 files with indirect coverage changes

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

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 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.

/assign

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.

Generally looks good to me.
It'd be great if you can add a test case.

Signed-off-by: jwcesign <jiangwei115@huawei.com>
@karmada-bot karmada-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 25, 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.

/lgtm
/approve

@XiShanYongYe-Chang
Do you think we need to skip the cluster during scheduling when the cluster already in eviction tasks?

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 26, 2023
@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 Apr 26, 2023
@karmada-bot karmada-bot merged commit 6d4cb3d into karmada-io:master Apr 26, 2023
@jwcesign
Copy link
Member Author

Do you think we need to skip the cluster during scheduling when the cluster already in eviction tasks?

Referring to the logic of K8s native scheduling, I think we need to try to schedule without the clusters in eviction tasks first.
if the scheduling fails, then try to schedule with these clusters.

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/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants