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

When Modifying (Cluster)ResourceBinding, Labels Was Merged Only When Binding with PropagationPolicy #3238

Closed
lxtywypc opened this issue Mar 6, 2023 · 5 comments · Fixed by #3239
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@lxtywypc
Copy link
Contributor

lxtywypc commented Mar 6, 2023

What happened:
When the resource-detector modify the (cluster)resourcebinding, whose labels would be merged only when binding with propagationpolicy:

https://github.com/karmada-io/karmada/blob/master/pkg/detector/detector.go#L407

but not when binding with clusterpropagationpolicy:

https://github.com/karmada-io/karmada/blob/master/pkg/detector/detector.go#L482

What you expected to happen:
The labels should be merged no matter which kind of propagationpolicy is binding with.

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

  1. Build a clusterpropagationpolicy and a propagationpolicy for different workload, then apply them.
  2. Add a test label in both bindings of the two workload manually.
  3. Scale the two workload and get their bindings, you will see the test label in bindings with propagationpolicy is still exists, while which in bindings with clusterpropagationpolicy has been already removed.

Anything else we need to know?:
I saw some changes in commit history, which seems that the method to modify the labels in bindings with propagationpolicy was changed from replacing to merging for adapting dependencies propagation. But in the later commits, I saw only some changes to modify the field .spec.propagateDeps, but no more changes to modify the labels. I don't know exactly if it is quite a bug or it is reasonable. If you have some other considering, please tell me.

By the way, I found that the modifing of propagateDeps was missed in clusterresourcebinding:
https://github.com/karmada-io/karmada/blob/master/pkg/detector/detector.go#L512-#L533

was it also a bug? If it was, I could add a commit to fix it. :)

Environment:

  • Karmada version:
  • kubectl-karmada or karmadactl version (the result of kubectl-karmada version or karmadactl version):
  • Others:
@lxtywypc lxtywypc added the kind/bug Categorizes issue or PR as related to a bug. label Mar 6, 2023
@chaunceyjiang
Copy link
Member

As far as I know, the dependency propagation feature is only available in the propagationpolicy, not in the cluster-propagationpolicy.

@lxtywypc
Copy link
Contributor Author

lxtywypc commented Mar 6, 2023

As far as I know, the dependency propagation feature is only available in the propagationpolicy, not in the cluster-propagationpolicy.

But in fact, it would also be modified in resourcebinding with clusterpropagationpolicy:
https://github.com/karmada-io/karmada/blob/master/pkg/detector/detector.go#L488

So I think it is related to the resource is cluster-scoped or namespace-scoped but not to the kind of propagationpolicy.
But I can't understand why the dependency propagation feature is not available for cluster-scoped resource.
After I read the code of dependencies-distributor, I found it might be a large project. It is far away from what I thought earlier.
We could open a new issue to talk about it. Let us just focus on the labels in this issue. :)

@XiShanYongYe-Chang
Copy link
Member

Hi @lxtywypc, thanks for your careful discovery. What you have pointed out is indeed a mistake. In our e2e test and general usage, we use PropagationPolicy to propagate the namespace-scoped resource, so this problem is undetected.

Would you like to send a pr to fix this error?

@lxtywypc
Copy link
Contributor Author

lxtywypc commented Mar 7, 2023

@XiShanYongYe-Chang I'm quite glad to, please have a look at #3239

@RainbowMango
Copy link
Member

/assign @lxtywypc
in favor of #3239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants