Fix uninstall + remove left over resources after test failure #3605
Conversation
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
carlisia
added
kind/changelog-not-required
carlisia
changed the title
Signed-off-by: Carlisia <carlisia@vmware.com>
Signed-off-by: Carlisia <carlisia@vmware.com>
There was a problem hiding this comment.
@carlisia I know that this PR is in draft state.
Considering that this is blocking the release, trying to reduce the latency here. So I made some comments. PTAL.
| } | ||
|
|
||
| rolebinding := install.ClusterRoleBinding(veleroNamespace) | ||
| time.Sleep(time.Second * 60) |
There was a problem hiding this comment.
Using magic periods of sleep makes this very brittle.
Please use WaitForNamespaceDeletion where we wait for a known condition.
Suggest calling client.CoreV1().Namespaces().Delete API and then WaitForNamespaceDeletion
This way we need not recreate a namespace object (Line 78)
There was a problem hiding this comment.
Yes, I not at all had the intention to leave this here :) - it needs to be user configurable.
| Example: `# velero uninstall -n backup`, | ||
| Run: func(c *cobra.Command, args []string) { | ||
|
|
||
| if !cli.GetConfirmation() { |
There was a problem hiding this comment.
This simply presents the user with
fmt.Printf("Are you sure you want to continue (Y/N)? ")
I would suggest printing a message describing the action we are attempting before calling cli.GetConfirmation
There was a problem hiding this comment.
Let's add a --quiet or --force option so this is scriptable
There was a problem hiding this comment.
Good call, thanks.
| crb := install.ClusterRoleBinding(namespace) | ||
| key = kbclient.ObjectKey{Name: crb.Name, Namespace: namespace} | ||
| if err := kbClient.Get(context.Background(), key, crb); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| fmt.Printf("Velero installation rolebinding %q does not exist, skipping.\n", crb.Name) | ||
| } else { | ||
| errs = append(errs, errors.WithStack(err)) | ||
| } | ||
| } else { | ||
| if err := kbClient.Delete(context.Background(), crb); err != nil { | ||
| errs = append(errs, errors.WithStack(err)) | ||
| } |
There was a problem hiding this comment.
Looks like we hard code the name of the cluster rolebinding to "velero"
https://github.com/vmware-tanzu/velero/blob/main/pkg/install/resources.go#L109
Why not simplify this to delete using the kubernetes API for cluster rolebinding instead of using the generic Delete API?
| return errors.Wrap(err, errorMsg) | ||
| } | ||
| } | ||
|
|
||
| fmt.Printf("Velero is installed and ready to be tested in the %s namespace! ⛵ \n", io.Namespace) |
There was a problem hiding this comment.
nit: "Velero is installed ..." is a duplicate message.
There was a problem hiding this comment.
I don't see the duplication. Where is the other one?
| @@ -104,23 +119,23 @@ func RunKibishiiTests(client *kubernetes.Clientset, providerName, veleroCLI, vel | |||
| return errors.Wrap(err, "Failed to generate data") | |||
| } | |||
|
|
|||
| if err := VeleroBackupNamespace(oneHourTimeout, veleroCLI, veleroNamespace, backupName, kibishiiNamespace, backupLocation, useVolumeSnapshots); err != nil { | |||
| VeleroBackupLogs(fiveMinTimeout, veleroCLI, veleroNamespace, backupName) | |||
| if err := veleroBackupNamespace(oneHourTimeout, veleroCLI, veleroNamespace, backupName, kibishiiNamespace, backupLocation, useVolumeSnapshots); err != nil { | |||
There was a problem hiding this comment.
These were all public so we didn't need to put all of the e2e tests in the same package. Eventually I think we want to split this up.
There was a problem hiding this comment.
When we do split these tests into separate packages will be a great time to export what is needed to be exported.
Signed-off-by: Carlisia <carlisia@vmware.com>
Does your change fix a particular issue?
Fixes #3599
Please indicate you've done the following:
/kind changelog-not-required.site/content/docs/main.