Add annotation to the the resource template to prevent removal managed resources #4788

CharlesQQ · 2024-04-01T10:26:30Z

What type of PR is this?

What this PR does / why we need it:
Extend the API field orphaningDeletion to prevent removal managed resources

Which issue(s) this PR fixes:
Fixes #4709

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

codecov-commenter · 2024-04-01T10:51:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.34%. Comparing base (d676996) to head (2e44070).
Report is 6 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4788      +/-   ##
==========================================
+ Coverage   53.31%   53.34%   +0.02%     
==========================================
  Files         252      253       +1     
  Lines       20539    20565      +26     
==========================================
+ Hits        10951    10971      +20     
- Misses       8862     8868       +6     
  Partials      726      726

Flag	Coverage Δ
unittests	`53.34% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

XiShanYongYe-Chang · 2024-04-23T09:47:48Z

pkg/apis/policy/v1alpha1/propagation_types.go

+
+ // OrphaningDeletionIncomplete means that work object and the resource willed be kept.
+ // But if work object deleted manually, cleanup the labels/annotations from the resource on member clusters
+ OrphaningDeletionIncomplete OrphaningDeletion = "Incomplete"


Is there a usage scenario for this type?

Is there a usage scenario for this type?

Refer to #3004

After reading the description and discussion in this issue, it seems that the work is not required to be retained. In terms of use, what can be done to retain work?

We can first just implement Complete and Never, and discuss the other options later once we have more specific requirements.

We can first just implement Complete and Never, and discuss the other options later once we have more specific requirements.

All right!

chaunceyjiang · 2024-04-24T08:59:38Z

pkg/controllers/binding/binding_controller.go

- if err := helper.DeleteWorks(c.Client, req.Namespace, req.Name, binding.Labels[workv1alpha2.ResourceBindingPermanentIDLabel]); err != nil {
- klog.Errorf("Failed to delete works related to %s/%s: %v", binding.GetNamespace(), binding.GetName(), err)
- return controllerruntime.Result{}, err
+ if binding.Spec.OrphaningDeletion == policyv1alpha1.OrphaningDeletionNever || binding.Spec.OrphaningDeletion == policyv1alpha1.OrphaningDeletionComplete {


I'd suggest using a switch here, as it will make the logic clearer. It will also make it easier for us to add comments for each case.

I'd suggest using a switch here, as it will make the logic clearer. It will also make it easier for us to add comments for each case.

Only one case now, it's not necessary

chaunceyjiang · 2024-04-28T03:52:02Z

pkg/apis/policy/v1alpha1/propagation_types.go

+ // +kubebuilder:default="Never"
+ // +kubebuilder:validation:Enum=Never;Complete
+ // +optional
+ OrphaningDeletionPolicy OrphaningDeletionPolicyType `json:"orphaningDeletionPolicy,omitempty"`


I believe we only need to add this field in PP. The work does not require this field.

In addition, I don't think the current implementation can fully handle all situations.

Here's a new idea:

When users set orphaningDeletionPolicy of PP as Complete.

The specific implementation should be adding an annotation on the workloadTemplate of the control plane, similar to orphaningDeletionPolicy.karmada.io: "Complete"

Then when deleting workloadTemplate in execution_controller, check if workloadTemplate has annotation orphaningDeletionPolicy.karmada.io. If it is Complete, do not delete it.

The benefits are:

The personnel creating the workloadTemplate can also control whether this application needs to be retained in member clusters, rather than only those who create PP.

Resources like namespace that do not have PP can also be deleted on the control plane without being deleted from member clusters.

Then when deleting workloadTemplate in execution_controller, check if workloadTemplate has annotation orphaningDeletionPolicy.karmada.io. If it is Complete, do not delete it.

Execution controller must watch all origin resource such as resourceTemlate 、namespace and so on, which make too complicated, I don't think that's a good idea.
I think for namespace, can add annotation like orphaningDeletionPolicy.karmada.io: "Complete"; FederatedResourceQuota those create work object direct, by adding field orphaningDeletionPolicy: Complete if necessary

Execution controller must watch all origin resource such as resourceTemlate 、namespace and so on

There's no need, this is existing logic.

This is no different from directly adding annotations to the resource template. In addition, users can directly skip pp to control the annotation value on the resource template.

This is no different from directly adding annotations to the resource template. In addition, users can directly skip pp to control the annotation value on the resource template.

Yes, I think that's reasonable.

I believe we only need to add this field in PP. The work does not require this field.

+1, but I prefer to decouple from PP/CPP, cause I'm afraid this is very unfriendly to administrators if the system also has the PropagationPolicyPreemption feature turned on. in addition, I feel that propagation policies are becoming more and more complex, and almost all new features must consider adding APIs to it.

but I prefer to decouple from PP/CPP, cause I'm afraid this is very unfriendly to administrators if the system also has the PropagationPolicyPreemption feature turned on.

Can you give an example of this?

if decoupled from PP/CPP, the administrator doesn't care about it.

Thanks, I get your meaning. How do you think @CharlesQQ @chaunceyjiang @RainbowMango

but I prefer to decouple from PP/CPP, cause I'm afraid this is very unfriendly to administrators if the system also has the PropagationPolicyPreemption feature turned on.

I do not quite understand how unfriendly here, I think the preemption is clear that after a PropagationPolicy(with higher priority) successfully preempted a resource, then the resource should be propagated as per the PropagationPolicy.

XiShanYongYe-Chang · 2024-05-10T03:13:34Z

Hi @CharlesQQ @chaunceyjiang Is there any blockage in this PR now? Can we move forward?

XiShanYongYe-Chang · 2024-05-10T03:25:24Z

pkg/apis/policy/v1alpha1/propagation_types.go

+
+ // OrphaningDeletionComplete means that the resource in member clusters completely independent of karmada
+ // Not only cleanup the labels/annotations from the resource on member clusters, but also delete work object in control plane
+ OrphaningDeletionComplete OrphaningDeletionPolicyType = "Complete"


Complete sounds a bit confusing. Is it possible to call it CompleteDelete?

ArgoCD uses the term "cascade"

It seems that using "cascade" would be better, as it can reduce the learning cost for users.

+1

It seems that using "cascade" would be better, as it can reduce the learning cost for users.

chaunceyjiang · 2024-05-10T03:25:59Z

Hi @CharlesQQ @chaunceyjiang Is there any blockage in this PR now? Can we move forward?

#4788 (comment) Regarding this plan, it seems that there is no conclusion yet?

XiShanYongYe-Chang · 2024-05-10T03:38:48Z

Let's invite @RainbowMango and @whitewindmills give some opinion.
/cc @RainbowMango @whitewindmills

whitewindmills · 2024-05-10T06:24:04Z

pkg/controllers/binding/binding_controller.go

- if err := helper.DeleteWorks(c.Client, req.Namespace, req.Name, binding.Labels[workv1alpha2.ResourceBindingPermanentIDLabel]); err != nil {
- klog.Errorf("Failed to delete works related to %s/%s: %v", binding.GetNamespace(), binding.GetName(), err)
- return controllerruntime.Result{}, err
+ if binding.Spec.OrphaningDeletionPolicy == policyv1alpha1.OrphaningDeletionNever || binding.Spec.OrphaningDeletionPolicy == policyv1alpha1.OrphaningDeletionComplete {


feel unnecessary to add such a statement.

RainbowMango · 2024-05-10T06:29:17Z

/assign

CharlesQQ · 2024-05-13T02:01:04Z

Regarding this plan, it seems that there is no conclusion yet?

Yes, To sum up, it seems inappropriate to put fields in pp/cpp

chaunceyjiang · 2024-05-15T03:22:59Z

@XiShanYongYe-Chang @RainbowMango @CharlesQQ @whitewindmills
Do we have a consensus on the current proposal?

XiShanYongYe-Chang · 2024-05-15T03:40:57Z

We may not reached a unanimous conclusion yet. Work suspension might also can learn from this discussion. They are both processing policies for resource templates and can be decoupled from PP.

There should be three options at this point,

Solution 1: Define a resource deletion policy in the PP.
- disadvantage:
  - Coupled with the PP, the PP becomes larger and larger with the evolution.
Solution 2: Add an annotation to the resource template to describe the deletion policy of the target resource.
- disadvantage:
  - Poor API evolution.
Solution 3: A new CRD is defined to describe a deletion policy of one or more resources.
- disadvantage:
  - Increased learning costs and CRs

You can publish it, or we can have a meeting to discuss and decide.

XiShanYongYe-Chang · 2024-05-16T03:50:28Z

@XiShanYongYe-Chang @RainbowMango @CharlesQQ @whitewindmills Do we have a consensus on the current proposal?

Can we set up a time to discuss this plan? When would be a good time for you, or maybe next Tuesday at the community meeting?

buji-code · 2024-05-22T19:03:12Z

Any update on this one?

XiShanYongYe-Chang · 2024-05-23T01:16:50Z

@buji-code Yeah, after discussion at the community meeting, we decided to try out solution 2 and 3. This work will start in the next release, early next month.

XiShanYongYe-Chang · 2024-05-23T14:05:07Z

Hi guys @CharlesQQ @whitewindmills @chaunceyjiang @RainbowMango @buji-code

Can we proceed with solution 2: Add an annotation to the resource template to describe the deletion policy of the target resource first, and then consider designing and adding a CRD to describe the deletion policy as the scenario develops? (Of course, the use of annotations is not excluded as the ideal end use.)

karmada-bot · 2024-05-29T07:09:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rainbowmango. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…vent removal managed resources Signed-off-by: chang.qiangqiang <[email protected]>

chaunceyjiang · 2024-05-31T02:52:32Z

pkg/controllers/execution/execution_controller.go

@@ -152,11 +152,21 @@ func (c *Controller) tryDeleteWorkload(clusterName string, work *workv1alpha1.Wo
 klog.Errorf("Failed to unmarshal workload, error is: %v", err)
 return err
 }
-


I think we can write an e2e to test this PR.

Check whether the resources in the member cluster have cleared the unique label of karmada.

Detect whether the resource still exists in the member cluster after the control plane deletes it.

The resources tested should include namespace resources.

karmada-bot requested review from chaunceyjiang, whitewindmills and XiShanYongYe-Chang April 1, 2024 10:26

karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 1, 2024

CharlesQQ changed the title ~~fix(controllers): Extend the API field orphaningDeletion to prevent r…~~ Extend the API field orphaningDeletion to prevent removal managed resources Apr 1, 2024

CharlesQQ force-pushed the prevent-removal-managed-resources branch from c0d0ad6 to a874076 Compare April 1, 2024 10:39

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 1, 2024

CharlesQQ force-pushed the prevent-removal-managed-resources branch from a874076 to c5994f0 Compare April 2, 2024 02:43

karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Apr 2, 2024

CharlesQQ force-pushed the prevent-removal-managed-resources branch from 7884969 to c5994f0 Compare April 2, 2024 03:16

karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Apr 2, 2024

CharlesQQ force-pushed the prevent-removal-managed-resources branch 3 times, most recently from 7c42d71 to 97504f8 Compare April 3, 2024 08:22

CharlesQQ force-pushed the prevent-removal-managed-resources branch 4 times, most recently from 6411574 to a302c57 Compare April 19, 2024 10:16

karmada-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 19, 2024

CharlesQQ force-pushed the prevent-removal-managed-resources branch from a302c57 to 0a5d11d Compare April 19, 2024 10:46

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 19, 2024

CharlesQQ force-pushed the prevent-removal-managed-resources branch 5 times, most recently from 9c6f601 to fb5afd1 Compare April 22, 2024 03:03

XiShanYongYe-Chang reviewed Apr 23, 2024

View reviewed changes

karmada-bot requested review from RainbowMango and zhy76 April 24, 2024 06:35

XiShanYongYe-Chang mentioned this pull request Apr 24, 2024

How to prevent deletion of resources for member clusters? #3004

Open

chaunceyjiang reviewed Apr 24, 2024

View reviewed changes

CharlesQQ force-pushed the prevent-removal-managed-resources branch from 4f30d77 to ed29102 Compare April 26, 2024 07:08

chaunceyjiang reviewed Apr 28, 2024

View reviewed changes

XiShanYongYe-Chang reviewed May 10, 2024

View reviewed changes

whitewindmills reviewed May 10, 2024

View reviewed changes

karmada-bot assigned RainbowMango May 10, 2024

XiShanYongYe-Chang mentioned this pull request May 10, 2024

Cascading policy for propagated resources #4925

Closed

XiShanYongYe-Chang mentioned this pull request May 17, 2024

feat: ability to suspend Work #4838

Open

CharlesQQ force-pushed the prevent-removal-managed-resources branch from ed29102 to ac35a37 Compare May 29, 2024 07:09

CharlesQQ changed the title ~~Extend the API field orphaningDeletion to prevent removal managed resources~~ Add annotation to the the resource template to prevent removal managed resources May 29, 2024

CharlesQQ force-pushed the prevent-removal-managed-resources branch from ac35a37 to ea6e382 Compare May 29, 2024 07:17

feat(controllers): Add annotation to the the resource template to pre…

2e44070

…vent removal managed resources Signed-off-by: chang.qiangqiang <[email protected]>

CharlesQQ force-pushed the prevent-removal-managed-resources branch from ea6e382 to 2e44070 Compare May 29, 2024 07:37

chaunceyjiang reviewed May 31, 2024

View reviewed changes

Add annotation to the the resource template to prevent removal managed resources #4788

Are you sure you want to change the base?

Add annotation to the the resource template to prevent removal managed resources #4788

Conversation

CharlesQQ commented Apr 1, 2024 • edited

codecov-commenter commented Apr 1, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitewindmills May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaunceyjiang commented May 10, 2024

XiShanYongYe-Chang commented May 10, 2024

Choose a reason for hiding this comment

RainbowMango commented May 10, 2024

CharlesQQ commented May 13, 2024

chaunceyjiang commented May 15, 2024

XiShanYongYe-Chang commented May 15, 2024 • edited

XiShanYongYe-Chang commented May 16, 2024

buji-code commented May 22, 2024

XiShanYongYe-Chang commented May 23, 2024

XiShanYongYe-Chang commented May 23, 2024

karmada-bot commented May 29, 2024

Choose a reason for hiding this comment

CharlesQQ commented Apr 1, 2024 •

edited

codecov-commenter commented Apr 1, 2024 •

edited

whitewindmills May 10, 2024 •

edited

XiShanYongYe-Chang commented May 15, 2024 •

edited