Support cli uninstall #3399

vadasambar · 2021-02-03T06:52:51Z

Attempts to fix #3291.

vadasambar · 2021-02-03T06:56:43Z

Will add the ChangeLog and DCO once I am done with my changes.

vadasambar · 2021-02-03T06:59:41Z

I've re-used parts from install package which adds a dependency on the install package. It'd be great if someone could review this approach. Some of the work that still needs to be done

Use the namespace provided in the cmd
Remove hardcoding to velero
Logs to stdout mentioning which resource is being deleted

The uninstallation deletes resource in the reverse order of their installation.

jenting

Thank you for your contribution, I like the idea.

However, the "velero install" command is going to be deprecated, so I'm not sure is the uninstall command should be added.

nrb · 2021-02-11T18:22:47Z

@jenting I think we'll take this as it's not that major. velero install may be going away, but I like having the the easy command.

@vadasambar This code looks good, but it could be simplified. Take a look at https://velero.io/docs/v1.5/uninstalling/

nrb

@vadasambar A few changes that need to be made, but I think they're fairly simple.

nrb · 2021-02-11T18:23:06Z

pkg/cmd/cli/uninstall/uninstall.go

@@ -0,0 +1,117 @@
+/*
+Copyright 2020 the Velero contributors.


We can leave out the year now

nrb · 2021-02-11T18:32:00Z

pkg/cmd/cli/uninstall/uninstall.go

+		return err
+	}
+
+	crb := install.ClusterRoleBinding(veleroNs)


Everything above this line is unnecessary, as the namespaced resources will be deleted with the namespace.

Also, we need to add the CustomResourceDefinitions. You can select them similarly to this

@nrb , makes sense. I am using an existing helper function to get all the CRDs

velero/pkg/cmd/cli/uninstall/uninstall.go

Line 36 in 827bad0

resources := install.AllCRDs()

Do you think using labels would be a better approach?
PS: Actually, using labels sounds like a better approach. If we want to delete an older velero install with a newer velero cli and there's a skew in the number of CRDs between the old and the new install, we might face issues with the helper function but label based approach would still work.

Yeah, the AllCRDs function was really meant to be used on installation only, not necessarily re-used as a selector.

Like you said, version skew could make this inaccurate.

Yeah. Will make the change then.

Please take a look at the VeleroUninstall in https://github.com/vmware-tanzu/velero/blob/main/test/e2e/velero_utils.go

We can expand on this as necessary, but it is pretty much a straight port of the existing hand uninstall process.
When "velero uninstall" is available in the CLI, the plan is to call that from the test code.

@dsu-igeek , I think we can port the uninstall code directly for the uninstall command.

When "velero uninstall" is available in the CLI, the plan is to call that from the test code.

Is this so that we could test the cli or is it okay to directly call the uninstall function? Asking this because I see we are doing the latter for VeleroInstall in test/e2e/velero_utils.go

We should eventually call the CLI both for install and uninstall. That way we can test the CLI on those paths as part of the testing.

- init fn to uninstall velero - abstract dynamic client creation to a separate fn - creates a separate client per unstructured resource - add delete client for CRDs - export appendUnstructured - add uninstall command to main cmd - export `podTemplateOption` - uninstall resources in the reverse order of installation - fallback to `velero` if no ns is provided during uninstall - skip deletion if the resource doesn't exist - handle resource not found error - match log formatting with cli install logs - add Delete fn to fake client - fix import order - add changelog - add comment doc for CreateClient fn Signed-off-by: Suraj Banakar <[email protected]>

- move helper functions out of test suite - this is to prevent cyclic imports - move uninstall helpers to uninstall cmd - call them from test suite - revert export of variables/fns from install code - because not required anymore Signed-off-by: Suraj Banakar <[email protected]>

Signed-off-by: Suraj Banakar <[email protected]>

vadasambar · 2021-03-02T06:38:07Z

Made most of the requested changes. @dsu-igeek, I have ported uninstall code from the test suite. Can you please take a look at the code whenever you find time?
Also,
https://github.com/vmware-tanzu/velero/pull/3399/files#diff-3ef9006b53436d6a87171de888d0ead4022b7e30b6fc4f29d30e36a1e24643bbR86

jenting · 2021-03-02T06:44:03Z

Please run make update

dsu-igeek

Looks good, can you change VeleroUninstall to call uninstall.Uninstall. Then we can change it to call the CLI later. Thanks!

dsu-igeek · 2021-03-03T00:34:06Z

test/e2e/enable_api_group_versions_test.go

@@ -342,7 +344,7 @@ func RunEnableAPIGroupVersionsTests(ctx context.Context, resource, group string,

 		// Uninstall Velero
 		if installVelero {
-			err = VeleroUninstall(ctx, client, extensionsClient, veleroNamespace)
+			err = uninstall.Uninstall(ctx, client, extensionsClient, veleroNamespace)


Same here, let's continue to call VeleroUninstall.

dsu-igeek · 2021-03-03T00:41:45Z

test/e2e/velero_utils.go

@@ -277,38 +271,6 @@ func VeleroInstall(ctx context.Context, veleroImage string, veleroNamespace stri
 	return nil
 }

-func VeleroUninstall(ctx context.Context, client *kubernetes.Clientset, extensionsClient *apiextensionsclient.Clientset,


Let's change VeleroUninstall to call uninstall.Uninstall for the moment and later we can have it run through the CLI.

@dsu-igeek , not sure if this is the right place to ask this but I can take a look at using cli in Install and Uninstall in the test suite since I have some context on what needs to be done, might as well make that change as well. Do we have any ticket for this or should I create a new one?

- as a wrapper - fix import related errors in test suite Signed-off-by: Suraj Banakar <[email protected]>

* Add uninstall cmd - init fn to uninstall velero - abstract dynamic client creation to a separate fn - creates a separate client per unstructured resource - add delete client for CRDs - export appendUnstructured - add uninstall command to main cmd - export `podTemplateOption` - uninstall resources in the reverse order of installation - fallback to `velero` if no ns is provided during uninstall - skip deletion if the resource doesn't exist - handle resource not found error - match log formatting with cli install logs - add Delete fn to fake client - fix import order - add changelog - add comment doc for CreateClient fn Signed-off-by: Suraj Banakar <[email protected]> * Re-use uninstall code from test suite - move helper functions out of test suite - this is to prevent cyclic imports - move uninstall helpers to uninstall cmd - call them from test suite - revert export of variables/fns from install code - because not required anymore Signed-off-by: Suraj Banakar <[email protected]> * Revert `PodTemplateOption` -> `podTemplateOption` Signed-off-by: Suraj Banakar <[email protected]> * Use uninstall helper under VeleroUninstall - as a wrapper - fix import related errors in test suite Signed-off-by: Suraj Banakar <[email protected]>

vadasambar force-pushed the support-cli-uninstall branch 2 times, most recently from eaac135 to 827bad0 Compare February 10, 2021 06:44

vadasambar marked this pull request as ready for review February 10, 2021 06:51

github-actions bot requested review from ashish-amarnath, carlisia, dsu-igeek, jenting, nrb and zubron February 10, 2021 06:52

github-actions bot assigned vadasambar Feb 10, 2021

jenting reviewed Feb 10, 2021

View reviewed changes

nrb suggested changes Feb 11, 2021

View reviewed changes

Suraj Banakar added 2 commits March 2, 2021 12:01

vadasambar force-pushed the support-cli-uninstall branch from 827bad0 to a796590 Compare March 2, 2021 06:31

github-actions bot requested review from jenting and nrb March 2, 2021 06:31

github-actions bot added has-changelog has-e2e-tests labels Mar 2, 2021

Revert PodTemplateOption -> podTemplateOption

e5d7a0e

Signed-off-by: Suraj Banakar <[email protected]>

dsu-igeek suggested changes Mar 3, 2021

View reviewed changes

Use uninstall helper under VeleroUninstall

9713d35

- as a wrapper - fix import related errors in test suite Signed-off-by: Suraj Banakar <[email protected]>

github-actions bot requested a review from dsu-igeek March 4, 2021 06:49

dsu-igeek approved these changes Mar 4, 2021

View reviewed changes

dsu-igeek added this to In progress in v1.6.0 via automation Mar 4, 2021

dsu-igeek added this to the v1.6.0 milestone Mar 4, 2021

dsu-igeek added the kind/release-blocker Must fix issues for the coming release (milestone) label Mar 4, 2021

v1.6.0 automation moved this from In progress to Reviewer approved Mar 4, 2021

nrb approved these changes Mar 4, 2021

View reviewed changes

nrb merged commit ff1a31d into vmware-tanzu:main Mar 4, 2021

v1.6.0 automation moved this from Reviewer approved to Done Mar 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cli uninstall #3399

Support cli uninstall #3399

vadasambar commented Feb 3, 2021 •

edited

Loading

vadasambar commented Feb 3, 2021

vadasambar commented Feb 3, 2021

jenting left a comment

nrb commented Feb 11, 2021

nrb left a comment •

edited

Loading

nrb Feb 11, 2021

nrb Feb 11, 2021

vadasambar Feb 15, 2021 •

edited

Loading

nrb Feb 22, 2021

vadasambar Feb 23, 2021

dsu-igeek Feb 23, 2021

vadasambar Feb 25, 2021 •

edited

Loading

dsu-igeek Mar 3, 2021

vadasambar commented Mar 2, 2021

jenting commented Mar 2, 2021

dsu-igeek left a comment

dsu-igeek Mar 3, 2021

dsu-igeek Mar 3, 2021

vadasambar Mar 5, 2021

Support cli uninstall #3399

Support cli uninstall #3399

Conversation

vadasambar commented Feb 3, 2021 • edited Loading

vadasambar commented Feb 3, 2021

vadasambar commented Feb 3, 2021

jenting left a comment

Choose a reason for hiding this comment

nrb commented Feb 11, 2021

nrb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadasambar Feb 15, 2021 • edited Loading

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

vadasambar Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadasambar commented Mar 2, 2021

jenting commented Mar 2, 2021

dsu-igeek left a 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

vadasambar commented Feb 3, 2021 •

edited

Loading

nrb left a comment •

edited

Loading

vadasambar Feb 15, 2021 •

edited

Loading

vadasambar Feb 25, 2021 •

edited

Loading