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

Clean up the residual annotations when resources are preempted by pp from cpp #5563

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

zhzhuang-zju
Copy link
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
refer to #3845, pp will preempt cpp regardless of priority. And it will also clean up the labels used to mark cpp during the preemption process. However, currently only the labels are being cleaned up, and the annotations are being left behind. This results in the same resource having both the annotation marking pp and the annotation marking cpp, like

apiVersion: work.karmada.io/v1alpha2
kind: ResourceBinding
metadata:
  annotations:
    clusterpropagationpolicy.karmada.io/name: test-cpp
    policy.karmada.io/applied-placement: '{"clusterTolerations":[{"key":"cluster.karmada.io/not-ready","operator":"Exists","effect":"NoExecute","tolerationSeconds":300},{"key":"cluster.karmada.io/unreachable","operator":"Exists","effect":"NoExecute","tolerationSeconds":300}],"replicaScheduling":{"replicaSchedulingType":"Divided","replicaDivisionPreference":"Aggregated"}}'
    propagationpolicy.karmada.io/name: test-pp
    propagationpolicy.karmada.io/namespace: default

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
From a mechanism standpoint, only pp can preempt cpp. Therefore, there is no need to consider cleaning up pp's labels and annotations when applying ClusterPolicy.

Does this PR introduce a user-facing change?:


@karmada-bot karmada-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 18, 2024
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 18, 2024
@zhzhuang-zju
Copy link
Contributor Author

test report:

  1. create configmap test-cm
apiVersion: v1
data:
  a: b
kind: ConfigMap
metadata:
  name: test-cm
  namespace: default
  1. create cpp test-cpp
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  name: test-cpp
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: 10
  resourceSelectors:
  - apiVersion: v1
    kind: ConfigMap
    name: test-cm
    namespace: default
  schedulerName: default-scheduler
  1. wait util the configmap has bund to the cpp
  2. create a pp test-pp
apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  name: test-pp
  namespace: default
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: Aggregated
      replicaSchedulingType: Divided
  preemption: Always
  priority: 0
  resourceSelectors:
  - apiVersion: v1
    kind: ConfigMap
    name: test-cm
    namespace: default
  schedulerName: default-scheduler

and the marks on rb test-cm-configmap will be:

$ karmadactl get rb test-cm-configmap -oyaml  
apiVersion: work.karmada.io/v1alpha2
kind: ResourceBinding
metadata:
  annotations:
    policy.karmada.io/applied-placement: '{"clusterTolerations":[{"key":"cluster.karmada.io/not-ready","operator":"Exists","effect":"NoExecute","tolerationSeconds":300},{"key":"cluster.karmada.io/unreachable","operator":"Exists","effect":"NoExecute","tolerationSeconds":300}],"replicaScheduling":{"replicaSchedulingType":"Divided","replicaDivisionPreference":"Aggregated"}}'
    propagationpolicy.karmada.io/name: test-pp
    propagationpolicy.karmada.io/namespace: default
  creationTimestamp: "2024-09-18T08:15:36Z"
  finalizers:
  - karmada.io/binding-controller
  generation: 4
  labels:
    propagationpolicy.karmada.io/permanent-id: ede05d0c-0d6c-4bb9-8001-2601a7cacfc5
    resourcebinding.karmada.io/permanent-id: 32fb4ef6-128c-4559-8172-a384cc51ceb4
  name: test-cm-configmap
  namespace: default

@zhzhuang-zju
Copy link
Contributor Author

cc @whitewindmills

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 50.84746% with 29 lines in your changes missing coverage. Please review.

Project coverage is 35.43%. Comparing base (62ae95e) to head (69a07ed).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
pkg/detector/detector.go 0.00% 29 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5563      +/-   ##
==========================================
+ Coverage   35.21%   35.43%   +0.21%     
==========================================
  Files         645      646       +1     
  Lines       44885    44988     +103     
==========================================
+ Hits        15806    15940     +134     
+ Misses      27847    27798      -49     
- Partials     1232     1250      +18     
Flag Coverage Δ
unittests 35.43% <50.84%> (+0.21%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 19, 2024
@zhzhuang-zju zhzhuang-zju force-pushed the preemption branch 2 times, most recently from e2dcc2c to 9d7fdd9 Compare September 19, 2024 07:31
pkg/detector/marks.go Outdated Show resolved Hide resolved
pkg/detector/marks.go Outdated Show resolved Hide resolved
pkg/detector/marks.go Outdated Show resolved Hide resolved
@zhzhuang-zju zhzhuang-zju force-pushed the preemption branch 2 times, most recently from d20e47c to 8c2d31e Compare October 8, 2024 02:28
@zhzhuang-zju
Copy link
Contributor Author

/retest

@zhzhuang-zju
Copy link
Contributor Author

All review comments have been resolved. To facilitate the review process, I will rebase the commits before merging
cc @RainbowMango @whitewindmills

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.

pkg/detector/detector.go Outdated Show resolved Hide resolved
pkg/detector/claim_test.go Outdated Show resolved Hide resolved
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

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2024
@RainbowMango
Copy link
Member

All review comments have been resolved. To facilitate the review process, I will rebase the commits before merging cc @RainbowMango @whitewindmills

Are you going to squash the commits?

@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2024
Copy link
Member

@whitewindmills whitewindmills left a comment

Choose a reason for hiding this comment

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

LGTM

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

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

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2024
@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 Oct 9, 2024
@karmada-bot karmada-bot merged commit 11d5c97 into karmada-io:master Oct 9, 2024
12 checks passed
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/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