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

Response to priority change of ClusterPropagationPolicy #3990

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

chaosi-zju
Copy link
Member

@chaosi-zju chaosi-zju commented Aug 24, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR provides a temporary solution for corner case:

After the priority(.spec.priority) of ClusterPropagationPolicy changed from high priority (e.g. 5) to 
low priority(e.g. 3), we should try to check if there is a ClusterPropagationPolicy(e.g. with priority 4) 
that could preempt the targeted resources.

Which issue(s) this PR fixes:
Fixes #
Part of #3787

Special notes for your reviewer:
See #3684 (comment) for the background discussion.

In addition, this PR focuses on dealing with ClusterPropagationPolicy, the PR of PropagationPolicy see #3965.

Test steps:

  1. Apply two policies (foo with priority 5 and bar with priority 4) as follows
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  name: foo
spec:
  priority: 5
  preemption: Always
  resourceSelectors:
    - apiVersion: apps/v1
      kind: Deployment
      name: nginx
      namespace: default
  placement:
    clusterAffinity:
      clusterNames:
        - member1
---
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  name: bar
spec:
  priority: 4
  preemption: Always
  resourceSelectors:
    - apiVersion: apps/v1
      kind: Deployment
      name: nginx
      namespace: default
  placement:
    clusterAffinity:
      clusterNames:
        - member2
  1. Apply a Deployment as this sample.
kubectl apply -f samples/nginx/deployment.yaml
  1. The Deployment should be propagated by foo as it has higher priority
apiVersion: apps/v1
kind: Deployment
metadata:
  generation: 1
  labels:
    app: nginx
    clusterpropagationpolicy.karmada.io/name: foo
  name: nginx
  1. Update the priority of foo from 5 to 3, then the Deployment should be preempted by bar.
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: nginx
    clusterpropagationpolicy.karmada.io/name: bar

meanwhile, I can see the relevant logs:

I0824 09:34:57.032839       1 preemption.go:280] ClusterPropagationPolicy(/foo) priority changed from 5 to 3
I0824 09:34:57.034163       1 preemption.go:314] Enqueuing ClusterPropagationPolicy(/bar) in case of ClusterPropagationPolicy(/foo) priority changes
I0824 09:34:57.042824       1 preemption.go:205] Cluster propagation policy(bar) has preempted another cluster propagation policy(foo).

more test see:

#3990 (comment)

Does this PR introduce a user-facing change?:

NONE(Part of feature #3787)

and enqueue ClusterPropagationPolicy that could repreempte.

Signed-off-by: chaosi-zju <chaosi@zju.edu.cn>
@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 24, 2023
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 24, 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

@chaosi-zju chaosi-zju changed the title Response to priority change of ClusterPropagationPolicy [WIP] Response to priority change of ClusterPropagationPolicy Aug 24, 2023
@karmada-bot karmada-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2023
@chaosi-zju
Copy link
Member Author

add test case, and verified ok


  1. Apply two policies (foo with priority 5 and bar with priority 4) as follows
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  name: foo
spec:
  priority: 5
  preemption: Always
  resourceSelectors:
    - apiVersion: rbac.authorization.k8s.io/v1
      kind: ClusterRole
      name: test-role
  placement:
    clusterAffinity:
      clusterNames:
        - member1
---
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  name: bar
spec:
  priority: 4
  preemption: Always
  resourceSelectors:
    - apiVersion: rbac.authorization.k8s.io/v1
      kind: ClusterRole
      name: test-role
  placement:
    clusterAffinity:
      clusterNames:
        - member2
  1. Apply a ClusterRole
kubectl create clusterrole test-role --verb=get,list,watch --resource=pods
  1. edit policy foo from priority 5 to priority 3

Result:

  1. the ClusterRole should be preempted by bar.
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  creationTimestamp: "2023-08-24T08:57:51Z"
  labels:
    clusterpropagationpolicy.karmada.io/name: bar
  name: test-role
  1. meanwhile, I can see the relevant logs:
I0824 08:58:59.402042       1 preemption.go:280] ClusterPropagationPolicy(/foo) priority changed from 5 to 3
I0824 08:58:59.402179       1 preemption.go:314] Enqueuing ClusterPropagationPolicy(/bar) in case of ClusterPropagationPolicy(/foo) priority changes
I0824 08:58:59.409788       1 preemption.go:205] Cluster propagation policy(bar) has preempted another cluster propagation policy(foo).

@RainbowMango
Copy link
Member

Please update test report as per your testing.

When a PropagationPolicy propagates a namespace-scoped resource, must specify the namespace in resource selector, but for now, we can't force it, let's see how to improve it:

(len(rs.Namespace) > 0 && resource.GetNamespace() != rs.Namespace) {

@chaosi-zju chaosi-zju changed the title [WIP] Response to priority change of ClusterPropagationPolicy Response to priority change of ClusterPropagationPolicy Aug 24, 2023
@karmada-bot karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 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

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 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 Aug 24, 2023
@karmada-bot karmada-bot merged commit 720f51a into karmada-io:master Aug 24, 2023
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/feature Categorizes issue or PR as related to a new feature. 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.

None yet

3 participants