feat: add --purge-namespace flags for `karmadactl deinit`, and default not delete namespace #3326

my-git9 · 2023-03-24T04:19:15Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
karmadactl deinit will delete the namespace where karmda components are located.
In some cases, this processing is too rough, which may delete other resources created by the user under the namespace (the user does not want to be cleaned up together), such as imagepullsecret, Qos settings, etc.
If the user is given the opportunity to choose whether to clean up the entire namespace, it will have a better experience.

Does this PR introduce a user-facing change?:

`karmadactl`: Introduced `--purge-namespace` flag for `deinit` command to skip namespace deletion during uninstallation.

codecov-commenter · 2023-03-24T04:35:52Z

Codecov Report

Merging #3326 (fe6febb) into master (a14d64f) will increase coverage by 2.56%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #3326      +/-   ##
==========================================
+ Coverage   49.13%   51.69%   +2.56%     
==========================================
  Files         209      210       +1     
  Lines       18754    18931     +177     
==========================================
+ Hits         9214     9787     +573     
+ Misses       9039     8618     -421     
- Partials      501      526      +25

Flag	Coverage Δ
unittests	`51.69% <ø> (+2.56%)`	⬆️

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

see 45 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

my-git9 · 2023-03-24T06:24:34Z

The test result:

Init Karmada:

Create secret in karmada-system ns:

Deinit karmada with --ignore-namespace

Check

RainbowMango · 2023-03-24T11:21:32Z

pkg/karmadactl/deinit/deinit.go

@@ -81,6 +82,7 @@ func NewCmdDeInit(parentCommand string) *cobra.Command {
 	flags.StringVar(&opts.Context, "context", "", "The name of the kubeconfig context to use")
 	flags.BoolVar(&opts.DryRun, "dry-run", false, "Run the command in dry-run mode, without making any server requests.")
 	flags.BoolVarP(&opts.Force, "force", "f", false, "Reset cluster without prompting for confirmation.")
+	flags.BoolVar(&opts.IgnoreNamespace, "ignore-namespace", false, "Reset cluster without delete namespace.")


The flag name ignore-namespace might be ambiguous, since deinit is going to uninstall/remove components from a cluster. ignore-namespace can be misinterpreted as if the component in the namespace, then ignore(cancel the process). What do you think?

This is indeed a question, what do you think of the flag name no-delete-namespace?

Before talking about the flag name, I'd like to invite owners(@carlory @lonelyCZ ) to comment if this is a reasonable feature.

--preserve-resources ?

For security and more intuitive, I think we could let users determine if remove the namespace. Like

deinit will not delete etcd data, if the etcd data is persistent, please delete it yourself. the default persistence path for etcd data is '/var/lib/karmada-etcd'

Or we simply don't delete the namespace

A option seems not to prevent mis-deleting for new users, because it is often missed. Based on this point, we shouldn't delete namespace by default.

What do you think?

Mybe we can not delete namespace by default, and delete namespace by specify options --purge-namespace ?

kindly ping @lonelyCZ @my-git9, do we have an executive decision?

For security and more intuitive, I think we could let users determine if remove the namespace. Like

+1, istio does not delete istio-system namespace when it is uninstalled by istioctl

Mybe we can not delete namespace by default, and delete namespace by specify options --purge-namespace ?

I think it is ok.

Mybe we can not delete namespace by default, and delete namespace by specify options --purge-namespace ?

I adjusted PR in this way, and the test result:

calvin0327 · 2023-03-27T10:48:06Z

If there are other workloads in the namespace karmada-system before installing karmada with karmadactl, It,s dangerous to delete the namespace.

`karmada deinit` default not delete namespace Signed-off-by: xin.li <xin.li@daocloud.io>

hzxuzhonghu · 2023-04-06T11:55:26Z

/lgtm

@lonelyCZ can you approve

lonelyCZ · 2023-04-06T12:06:55Z

Thanks @my-git9

/approve

karmada-bot · 2023-04-06T12:07:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu, lonelyCZ

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:

~~pkg/karmadactl/OWNERS~~ [lonelyCZ]

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

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 24, 2023

karmada-bot requested review from carlory and lonelyCZ March 24, 2023 04:19

karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 24, 2023

my-git9 force-pushed the feat/ignorens branch from bbaea79 to 3d3668b Compare March 24, 2023 06:18

RainbowMango reviewed Mar 24, 2023

View reviewed changes

RainbowMango mentioned this pull request Mar 30, 2023

Propose donot delete namespace when karmadactl deinit #3344

Closed

my-git9 force-pushed the feat/ignorens branch from 3d3668b to 1e896a3 Compare April 4, 2023 15:54

my-git9 changed the title ~~feat: add --ignore-namespace flags for karmadactl deinit~~ feat: add --purge-namespace flags for karmadactl deinit, and default not delete namespace Apr 4, 2023

feat: add --purge-namespace flags for karmada deinit

fe6febb

`karmada deinit` default not delete namespace Signed-off-by: xin.li <xin.li@daocloud.io>

my-git9 force-pushed the feat/ignorens branch from 1e896a3 to fe6febb Compare April 4, 2023 15:58

karmada-bot assigned hzxuzhonghu Apr 6, 2023

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023

hzxuzhonghu approved these changes Apr 6, 2023

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2023

karmada-bot merged commit 31b5088 into karmada-io:master Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add --purge-namespace flags for `karmadactl deinit`, and default not delete namespace #3326

feat: add --purge-namespace flags for `karmadactl deinit`, and default not delete namespace #3326

my-git9 commented Mar 24, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Mar 24, 2023 •

edited

Loading

my-git9 commented Mar 24, 2023

RainbowMango Mar 24, 2023

my-git9 Mar 25, 2023

RainbowMango Mar 27, 2023

jwcesign Mar 30, 2023

lonelyCZ Mar 30, 2023

my-git9 Apr 3, 2023

XiShanYongYe-Chang Apr 4, 2023

hzxuzhonghu Apr 4, 2023

lonelyCZ Apr 4, 2023

my-git9 Apr 4, 2023

calvin0327 commented Mar 27, 2023

hzxuzhonghu commented Apr 6, 2023

lonelyCZ commented Apr 6, 2023

karmada-bot commented Apr 6, 2023

feat: add --purge-namespace flags for karmadactl deinit, and default not delete namespace #3326

feat: add --purge-namespace flags for karmadactl deinit, and default not delete namespace #3326

Conversation

my-git9 commented Mar 24, 2023 • edited by RainbowMango Loading

codecov-commenter commented Mar 24, 2023 • edited Loading

Codecov Report

my-git9 commented Mar 24, 2023

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

calvin0327 commented Mar 27, 2023

hzxuzhonghu commented Apr 6, 2023

lonelyCZ commented Apr 6, 2023

karmada-bot commented Apr 6, 2023

feat: add --purge-namespace flags for `karmadactl deinit`, and default not delete namespace #3326

feat: add --purge-namespace flags for `karmadactl deinit`, and default not delete namespace #3326

my-git9 commented Mar 24, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Mar 24, 2023 •

edited

Loading