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

docs(proposal): add docs for using cascading deletion #5101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CharlesQQ
Copy link
Contributor

@CharlesQQ CharlesQQ commented Jun 26, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
part of #4709

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 26, 2024
@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch from 7d688b9 to 09ad847 Compare June 26, 2024 03:55
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.90%. Comparing base (c426e97) to head (788e44a).
Report is 294 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    #5101      +/-   ##
==========================================
+ Coverage   28.19%   28.90%   +0.71%     
==========================================
  Files         631      632       +1     
  Lines       43482    43862     +380     
==========================================
+ Hits        12258    12680     +422     
+ Misses      30326    30267      -59     
- Partials      898      915      +17     
Flag Coverage Δ
unittests 28.90% <ø> (+0.71%) ⬆️

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.

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @CharlesQQ

Hi, guys @RainbowMango @chaunceyjiang @whitewindmills @zhy76 @buji-code @lcw2
We can discuss here to improve the resource deletion policy.

@whitewindmills
Copy link
Member

/assign

@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch 2 times, most recently from 0f2fb08 to 2653e9d Compare June 27, 2024 08:10
Copy link
Member

@whitewindmills whitewindmills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this annotation have a knock-on effect on dependent resources?―that is, one adds such an annotation to his Deployment, will the annotation affect its dependent resources(like ConfigMap)?

@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch from 2653e9d to 09c6d78 Compare June 28, 2024 03:08
@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch from 09c6d78 to 281dbbf Compare July 1, 2024 03:16
@karmada-bot 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 Jul 8, 2024
@CharlesQQ CharlesQQ changed the title docs(proposal): add docs for deletion policy docs(proposal): add docs for using cascading deletion Jul 8, 2024
@karmada-bot karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jul 8, 2024
@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch from b9171d6 to 4ad9a83 Compare July 8, 2024 07:48
@karmada-bot karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jul 8, 2024
@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch 2 times, most recently from 5315523 to fe04cdf Compare July 11, 2024 10:12
@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch 6 times, most recently from 9c9f156 to 7153d46 Compare July 22, 2024 03:53
Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also ask @RainbowMango to help take a look.
/cc @RainbowMango

docs/proposals/use-cascading-deletion/README.md Outdated Show resolved Hide resolved
docs/proposals/use-cascading-deletion/README.md Outdated Show resolved Hide resolved
@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch from 7153d46 to 604716b Compare August 9, 2024 02:55
@XiShanYongYe-Chang
Copy link
Member

/assign @RainbowMango

Equivalent to supporting both solution one and solution two

### Solution comparison
| Name | Supported control plane resources | Extend API resources | User learning cost |
Copy link
Member

@chaosi-zju chaosi-zju Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get what is the different below the title "Supported control plane resources".

Comment on lines 277 to 280
| Solution One | all resources | None | lowest |
| Solution Two | resource template | PP/CPP/RB/CRB/WORK | lowest |
| Solution Three | all resources | new CRD/WORK | Highest |
| Solution Four | all resources | PP/CPP/RB/CRB/WORK | lower |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can highlight another comparison like solution one can set the orphan attribute at the granularity of each resource template, while solution two can only set the orphan` attribute at the granularity of policy. In 1 policy vs multi resource scene, we cann't execute orphan delete just by per resource.

@CharlesQQ CharlesQQ force-pushed the prevent-removal-managed-resources-docs branch from 604716b to e37b16b Compare August 13, 2024 02:42
@karmada-bot
Copy link
Collaborator

[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:

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

Signed-off-by: chang.qiangqiang <chang.qiangqiang@immomo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants