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

Add --uninstall and --preserve-uninstall-namespace option to velero install #6155

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kaovilai
Copy link
Contributor

@kaovilai kaovilai commented Apr 16, 2023

Todo

  • Design doc

If you remember the options you used to install, this PR allows you to use the same options with the addition of --uninstall and --preserve-uninstall-namespace to undo the install.

We are explicitly not calling delete on anything created outside install command, that includes BSL/VSL created outside install command defaults. Depending on feedback, this can be added via another flag.

In addition, the normal uninstall command prompt message is now more informative

fmt.Printf("You are about to uninstall Velero from namespace %q. Namespace will be deleted\nTo uninstall from a different namespace, use --namespace flag.", f.Namespace())

Does your change fix a particular issue?

Fixes #5433

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2023

Codecov Report

Merging #6155 (fe1fad3) into main (65f99c1) will decrease coverage by 0.24%.
The diff coverage is 9.52%.

@@            Coverage Diff             @@
##             main    #6155      +/-   ##
==========================================
- Coverage   41.28%   41.05%   -0.24%     
==========================================
  Files         251      252       +1     
  Lines       23399    23724     +325     
==========================================
+ Hits         9661     9740      +79     
- Misses      12979    13210     +231     
- Partials      759      774      +15     
Impacted Files Coverage Δ
pkg/install/install.go 0.00% <0.00%> (ø)
pkg/cmd/cli/install/install.go 26.29% <44.44%> (ø)

... and 2 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.

@kaovilai kaovilai force-pushed the issue5433 branch 2 times, most recently from 51805e5 to cad7e8e Compare April 17, 2023 04:42
@Lyndon-Li Lyndon-Li self-assigned this Apr 17, 2023
@kaovilai kaovilai changed the title uninstall velero without deleting namespace Add --uninstall and --preserve-uninstall-namespace option to velero install Apr 17, 2023
…ero install`

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@Lyndon-Li
Copy link
Contributor

Lyndon-Li commented Apr 26, 2023

@kaovilai
Here we have requirements:

  1. Preserve the namespace, use case is as issue High risk: uninstall velero will delete the namespace #5433
  2. Preserve Velero internal objects, i.e., BSL, VSL, schedules, backup, restore, policies, etc.

The implementation for 1 is clear.
However, for 2, some more details need to be covered:

  • If an object is preserved, after reinstall, what if the CRD changes, as a result, the preserved object doesn't match the new CRD?
  • Not all internal objects need to be preserved, it may even be harmful if we preserve some unnecessary objects. So we need to clarify which ones will be preserved, which ones will not and why

Comparing to 1, 2 is more useful because along with the evolution of Velero, objects may be quite complex and their lifecycle may be better to match the cluster instead of Velero installation, i.e., the schedules and polices.

Therefore, would you like to create a design doc to cover both 1 and 2 and make an overall advanced uninstallation feature?

For 1, personally I would rather regard it as a corner case -- someone has installed Velero into an existing namespace, though the consequence of uninstallation in this case is severe.
Then I would think that it is unnecessary to expose a flag like --preserve-uninstall-namespace. Instead, we just check the current namespace is velero or not, if it is velero, we delete the namespace, otherwise, we don't delete it.
If the namespace is indeed velero, I cannot find any case that we need to preserve it.

Once we have the design doc, we can discuss this further.

cc @reasonerjt @sseago

@kaovilai
Copy link
Contributor Author

Sure thing. Design doc for 1 and 2.

For 2. I think we only have to deal with it at reinstall time so we can check for outdated velero resources if namespace exist already.

Tho I'm not sure if 2 is possible because 1 deletes CRD right now. Maybe I need to add 1 more flag, --preserve-CRD

@Lyndon-Li
Copy link
Contributor

Lyndon-Li commented Apr 26, 2023

@kaovilai

For 2. I think we only have to deal with it at reinstall time so we can check for outdated velero resources if namespace exist already.

Things may be more complicated:

  1. How do we tell the resource is outdated?
  2. Once we find it is outdated during reinstall, what do we do? We cannot delete it since it takes lots of user defined data

Tho I'm not sure if 2 is possible because 1 deletes CRD right now. Maybe I need to add 1 more flag, --preserve-CRD

Yes, for some CRs, if we want to preserve it, the CRD will also be required. However, from users perspective, they don't case about CRDs, they only care whether their defined data along with the CRs have been preserved or not. For sure, we need to design this from users perspective.

@@ -118,6 +120,8 @@ func (o *InstallOptions) BindFlags(flags *pflag.FlagSet) {
flags.StringVar(&o.Features, "features", o.Features, "Comma separated list of Velero feature flags to be set on the Velero deployment and the node-agent daemonset, if node-agent is enabled")
flags.BoolVar(&o.DefaultVolumesToFsBackup, "default-volumes-to-fs-backup", o.DefaultVolumesToFsBackup, "Bool flag to configure Velero server to use pod volume file system backup by default for all volumes on all backups. Optional.")
flags.StringVar(&o.UploaderType, "uploader-type", o.UploaderType, fmt.Sprintf("The type of uploader to transfer the data of pod volumes, the supported values are '%s', '%s'", uploader.ResticType, uploader.KopiaType))
flags.BoolVar(&o.Uninstall, "uninstall", o.Uninstall, "Uninstall Velero from the cluster. Optional.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we add a --uninstall flag under velero install instead of under velero uninstall?
It is hard to understand if the command is like "velero install --uninstall".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because uninstall command do not have the install options.

If we want this functionality in uninstall cmd then the installOptions fields will have to be included with uninstall cmd.

This makes it clear that it'll only work if you know how you installed it.

Perhaps even lookup your bash history and just add --uninstall

Copy link
Contributor

Choose a reason for hiding this comment

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

Because uninstall command do not have the install options

I thinks this is a problem of the code organization, we have multiple ways to solve it. i.e., Add a new UninstallOption struct.

This makes it clear that it'll only work if you know how you installed it.

Maybe, but this will confuse others who don't know the details.

Anyway, as mentioned about, I think even the flag --preserve-uninstall-namespace is unnecessary if we want to fix requirement 1 only.

@kaovilai kaovilai marked this pull request as draft April 26, 2023 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

High risk: uninstall velero will delete the namespace
3 participants