karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.44k stars 880 forks source link

don't reconcile resourcebinding when change resourceselector of propagation policy #3681

Open olderTaoist opened 1 year ago

olderTaoist commented 1 year ago

What happened: change the resourceselector of propagation policy, the resourcebinding of old match resource doesn't be deleted

apiVersion: policy.karmada.io/v1alpha1
kind: PropagationPolicy
metadata:
  name: nginx-propagation
spec:
  resourceSelectors:
    - apiVersion: v1
      kind: Service
      labelSelector:
        matchLabels:
          app: nginx
  placement:
    clusterAffinity:
      clusterNames:
        - paas-federal-user1-master1
        - paas-federal-user2-master1

What you expected to happen: the resourcebinding of old match resource be deleted

How to reproduce it (as minimally and precisely as possible):

propagaion policy is mentioned above. the resource as fellow:

apiVersion: v1
kind: Service
metadata:
  name: nginx-1
  labels:
    app: nginx-1
spec:
  ports:
  - port: 80
    name: web
  selector:
    app: nginx

Anything else we need to know?:

Environment:

olderTaoist commented 1 year ago

/assign

chaunceyjiang commented 1 year ago

the resourcebinding of old match resource be deleted

Hi, @olderTaoist Can you provide more detailed information?

weilaaa commented 1 year ago

IIRC, karmada allowed update ResourceSelector in one version, but it seems did not delete related resourcebinding/clusterresourcebinding when removing selected resources from ResourceSelector

weilaaa commented 1 year ago

I found this pr #2562 , and I did't found anywhere to delete related resourcebinding/clusterresourcebinding. It seems only cleaned up the labels of binding.

Like the title as @olderTaoist wrote, if we should reconcile binding to do clean up action when labels is removed from binding.

weilaaa commented 1 year ago

the resourcebinding of old match resource be deleted

Hi, @olderTaoist Can you provide more detailed information?

I guess the meaning of @olderTaoist is that rb should be deleted when the resource was never matched by any pp or cpp, so that related work would be deleted and the spread resource would be deleted either.

RainbowMango commented 1 year ago

invite #2562 author here to comment. @XiShanYongYe-Chang

XiShanYongYe-Chang commented 1 year ago

Hi, @olderTaoist @weilaaa Thanks for your feedback and understanding of the problem.

Currently, in karmada, the life cycle of rb/crb is consistent with that of resource template. As discussed in the issue, after the resourcesselector of pp/cpp is modified, the original resource template is no longer matched, but the rb still exists. This is because the resource template still exists. When a new PP policy is used to name the resource template, the resource template is modified in the rb/crb. After a resource template is deleted, the resource template is also deleted. In fact, when a pp/cpp is deleted, if the original resource template still exists, the rb/crb also still exists. The two behaviors are essentially the same, that is, the life cycle of the rb/crb must be consistent with that of the resource template.

weilaaa commented 1 year ago

Hi, @olderTaoist @weilaaa Thanks for your feedback and understanding of the problem.

Currently, in karmada, the life cycle of rb/crb is consistent with that of resource template. As discussed in the issue, after the resourcesselector of pp/cpp is modified, the original resource template is no longer matched, but the rb still exists. This is because the resource template still exists. When a new PP policy is used to name the resource template, the resource template is modified in the rb/crb. After a resource template is deleted, the resource template is also deleted. In fact, when a pp/cpp is deleted, if the original resource template still exists, the rb/crb also still exists. The two behaviors are essentially the same, that is, the life cycle of the rb/crb must be consistent with that of the resource template.

Hi, @XiShanYongYe-Chang ,Thanks for your explanation. I understand you're trying to state that the life cycle of rb and resource template are consistent, but I don't understand why their life cycle need to be consistent. As an api user, when he removes the resource template from the resourceselector in pp, according to the user's intuition, does he want to cancel this propagation behavior? Can you explain more?

XiShanYongYe-Chang commented 1 year ago

Hi @weilaaa, I understand your confusion, because I have also thought about this issue during the implementation process. Let me briefly explain the process that led to this issue, here's what happened.

In the beginning, Karmada also supported synchronously deleting rb when pp was deleted. However, later on (I may not be able to specify exactly when), someone suggested that if a pp is fundamental and hits many resource templates, deleting this pp at this time could have catastrophic consequences. Therefore, we modified the behavior to what it is now: rb follows the lifecycle of resource templates.

Considering the current situation is confusing, I'm thinking if we can take a step forward and consider how to make this behavior more in line with user expectations. For example, can we give control of this behavior to users and let them decide whether rb needs to be synchronized and deleted?

Hi @weilaaa @olderTaoist We can continue to discuss how to handle this issue, treating it as a feature evolution. What do you think?

weilaaa commented 1 year ago

/kind feature

Thanks a lot for explaining the ins and outs, @XiShanYongYe-Chang. I think treating it as a feature evolution is ok. Since @olderTaoist discovered this issue first, please focus on this issue, how do you think? :)

XiShanYongYe-Chang commented 1 year ago

For PP deletion or modification of ResourceSelector, I think these two need to be consistent. I believe we can now discuss directly how users can decide how to handle whether RB needs cleaning.

weilaaa commented 1 year ago

For PP deletion or modification of ResourceSelector

Totally agree.

How users can decide how to handle whether RB needs cleaning.

I prefer to add a new gate in pp to determine this behavior, and the default is consistent with the current one to be compatible with the existing data. User can open new gate in pp to change this behavior if he knows what he is doing.

RainbowMango commented 1 year ago

I prefer to add a new gate in pp to determine this behavior, and the default is consistent with the current one to be compatible with the existing data. User can open new gate in pp to change this behavior if he knows what he is doing.

+1 Yes, we need a way for users to clearly know and control what's gonna happen when removing/(edit resource selector). Adding a field(or some fields) might be an alternative.

XiShanYongYe-Chang commented 1 year ago

Hi @olderTaoist @weilaaa Can you help come up with a relatively complete solution? If it's convenient, I think we can also use a proposal to handle it.

weilaaa commented 1 year ago

Hi @olderTaoist @weilaaa Can you help come up with a relatively complete solution? If it's convenient, I think we can also use a proposal to handle it.

Yes,to have a proposal is good,waiting for @olderTaoist to comment first.

weilaaa commented 1 year ago

I guess @olderTaoist should add proposal here , if you are willing to.

olderTaoist commented 1 year ago

Hi @olderTaoist @weilaaa Can you help come up with a relatively complete solution? If it's convenient, I think we can also use a proposal to handle it.

ok, invite you to view when finished writing the proposal

XiShanYongYe-Chang commented 1 year ago

Kindly ping @olderTaoist @weilaaa

Furthermore, I also invite you to take a look at this issue: #3873

weilaaa commented 1 year ago

Kindly ping @olderTaoist @weilaaa

Furthermore, I also invite you to take a look at this issue: #3873

Replied in other side.

olderTaoist commented 1 year ago

Hi @olderTaoist @weilaaa Can you help come up with a relatively complete solution? If it's convenient, I think we can also use a proposal to handle it.

i add a proposal here @weilaaa @XiShanYongYe-Chang