keep ResourceBinding when removing PropagationPolicy #601
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karmada-bot
added
kind/feature
qianjun1993
force-pushed
the
delete-policy
branch
from
b10bcd9
to
47a83be
Compare
|
I haven't dig into why the E2E failing. Just echo the logs here and try again. |
|
Do you know when the crd of ServiceImport CRD is created. Is it created in advance? I only found the creation of ClusterPropagationPolicy in this test. |
The karmada/hack/deploy-karmada.sh Lines 115 to 116 in 51233e5 |
|
You may need to modify the e2e test: Lines 265 to 296 in 51233e5 the original logic will check whether |
|
So just remove this check? |
I think so. |
karmada-bot
added
size/M
qianjun1993
force-pushed
the
delete-policy
branch
2 times, most recently
from
eb197b2
to
ef67c09
Compare
|
/assign |
| @@ -258,41 +258,6 @@ var _ = ginkgo.Describe("[MCS] Multi-Cluster Service testing", func() { | |||
| err := karmadaClient.PolicyV1alpha1().ClusterPropagationPolicies().Delete(context.TODO(), serviceImportPolicy.Name, metav1.DeleteOptions{}) | |||
| gomega.Expect(err).ShouldNot(gomega.HaveOccurred()) | |||
| }) | |||
There was a problem hiding this comment.
Can you add some comment to record why not wait ServiceExport and ServiceImport disappear on member clusters?
There was a problem hiding this comment.
OK, I will add it.
qianjun1993
force-pushed
the
delete-policy
branch
from
ef67c09
to
bfd8142
Compare
| @@ -866,20 +866,14 @@ func (d *ResourceDetector) HandlePropagationPolicyDeletion(policyNS string, poli | |||
| return err | |||
| } | |||
|
|
|||
| for itemIndex, binding := range rbs.Items { | |||
| for _, binding := range rbs.Items { | |||
There was a problem hiding this comment.
The method's comments should be updated as well, e.g.
// HandlePropagationPolicyDeletion handles PropagationPolicy delete event.
// After a policy is removed, the label marked on relevant resource template will be removed(which gives
// the resource template a change to match another policy).
//
// Note: The relevant ResourceBinding will continue to exist until the resource template is gone.
func (d *ResourceDetector) HandlePropagationPolicyDeletion(policyNS string, policyName string) error {There was a problem hiding this comment.
OK, I will change it.
| @@ -894,48 +888,36 @@ func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyName str | |||
| policyv1alpha1.ClusterPropagationPolicyLabel: policyName, | |||
| } | |||
|
|
|||
| // load and remove the ClusterResourceBindings which labeled with current policy | |||
| // load the ClusterResourceBindings which labeled with current policy | |||
There was a problem hiding this comment.
The method's comments should be updated as well, e.g.:
// HandleClusterPropagationPolicyDeletion handles ClusterPropagationPolicy delete event.
// After a policy is removed, the label marked on relevant resource template will be removed(which gives
// the resource template a change to match another policy).
//
// Note: The relevant ClusterResourceBinding or ResourceBinding will continue to exist until the resource template is gone.
func (d *ResourceDetector) HandleClusterPropagationPolicyDeletion(policyName string) error {
|
I'm considering a case,
Will the deployment be removed from |
Signed-off-by: junqian <junqian@tencent.com>
qianjun1993
force-pushed
the
delete-policy
branch
from
bfd8142
to
bf7f4bf
Compare
The deployment will be removed from member1 |
|
I found this issue twice today. Let's re-trigger it. |
|
/lgtm |
RainbowMango
changed the title
RainbowMango
added
the
approved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
stop delete resourcebinding when the related policy is deleted
Which issue(s) this PR fixes:
Fixes #505
Special notes for your reviewer:
Does this PR introduce a user-facing change?: