Disable a single resource multi dependency distribution #5743
Conversation
karmada-bot
added
the
kind/feature
karmada-bot
added
the
size/L
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5743 +/- ##
==========================================
- Coverage 42.25% 42.20% -0.06%
==========================================
Files 655 655
Lines 55847 55928 +81
==========================================
+ Hits 23600 23602 +2
- Misses 30734 30811 +77
- Partials 1513 1515 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return err | ||
| } | ||
|
|
||
| if d.DisableMultiDependencyDistribution { |
There was a problem hiding this comment.
Can you remind me what situation this logic is handling?
There was a problem hiding this comment.
A parameter is added to determine whether the Karmada system allows the same resource to be dependent on different resources. By default, the value is true, indicating that multiple dependencies will be prohibited in future evolution, including that a resource is processed by the PropagationPolicy and distributed along with the resource.
There was a problem hiding this comment.
I understand the purpose of your DisableMultiDependencyDistribution parameter. But I haven't grasped what the code you wrote in the detector is meant to handle.
Looking at the e2e scenario, this code seems to be breaking compatibility.
There was a problem hiding this comment.
But I haven't grasped what the code you wrote in the detector is meant to handle.
The logic of this code is incorrect. I will correct it later.
Looking at the e2e scenario, this code seems to be breaking compatibility.
When DisableMultiDependencyDistribution is false, it adds some restrictions, and it really can be understood as break change.
When DisableMultiDependencyDistribution is true, it keeps the original behavior.
There was a problem hiding this comment.
Please rebase and fix the failing tests.
|
|
||
| // DisableMultiDependencyDistribution indicates disable the ability to a resource from being depended on by multiple | ||
| // resources or being distributed by PropagationPolicy/ClusterPropagationPolicy while being depended on. | ||
| // | ||
| // Before v1.12, this capability is allowed by default. If you still wish to enable this capability, you can set | ||
| // this flag to false. However, you will need to bear some side effects that come with it. | ||
| // For example, you can refer to https://github.com/karmada-io/karmada/pull/5717. When the primary resource is deleted, | ||
| // it does not consider other resources that currently depend on the resource or any PropagationPolicy associated with it. | ||
| // | ||
| // It is recommended that you adapt your business accordingly to avoid continued use. | ||
| DisableMultiDependencyDistribution bool |
There was a problem hiding this comment.
Would it be better to have a feature gate than a command flag? Something like: DisableDependencySharing.
For example:
- release-1.12 starts from alpha, which is disabled by default, for backward compatibility concerns.
- release-1.13 move to beta, which is enabled by default, and allows people to disable it as an escape hatch
- release-1.14 move to stable, enabled by default, and don't allow to disable it.
@XiShanYongYe-Chang @chaunceyjiang What do you think?
Signed-off-by: changzhen <[email protected]>
XiShanYongYe-Chang
force-pushed
the
add-limit-for-dependency-resource
branch
from
ea0c003
to
b5e0c44
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
From the majority of user feedback received so far, there are no scenarios where a single resource is depended on by multiple resources, or where a resource is both depended on by other resources and also distributed through PropagationPolicy/ClusterPropagationPolicy. Therefore, we have decided to restrict the above usage in the Karmada system, with the ultimate goal of ensuring that a resource can only be subject to distribution decisions by one PropagationPolicy/ClusterPropagationPolicy.
By imposing such a restriction, we can make the behavior of the Karmada system more explicit and also facilitate the iteration of the follow-on distribution feature.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The current PR is far from the end. In order to complete the entire restriction activity, I plan to carry out the following tasks:
Does this PR introduce a user-facing change?: