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

Fix uninstall + remove left over resources after test failure #3605

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pkg/client/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ import (

"github.com/pkg/errors"
"github.com/spf13/pflag"
apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/kubernetes"
k8scheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"

velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
Expand All @@ -45,7 +47,8 @@ type Factory interface {
// DynamicClient returns a Kubernetes dynamic client. It uses the following priority to specify the cluster
// configuration: --kubeconfig flag, KUBECONFIG environment variable, in-cluster configuration.
DynamicClient() (dynamic.Interface, error)
// KubebuilderClient returns a Kubernetes dynamic client. It uses the following priority to specify the cluster
// KubebuilderClient returns a client for the controller runtime framework. It adds Kubernetes and Velero
// types to its scheme. It uses the following priority to specify the cluster
// configuration: --kubeconfig flag, KUBECONFIG environment variable, in-cluster configuration.
KubebuilderClient() (kbclient.Client, error)
// SetBasename changes the basename for an already-constructed client.
Expand Down Expand Up @@ -151,6 +154,8 @@ func (f *factory) KubebuilderClient() (kbclient.Client, error) {

scheme := runtime.NewScheme()
velerov1api.AddToScheme(scheme)
k8scheme.AddToScheme(scheme)
apiextv1beta1.AddToScheme(scheme)
kubebuilderClient, err := kbclient.New(clientConfig, kbclient.Options{
Scheme: scheme,
})
Expand Down
5 changes: 1 addition & 4 deletions pkg/cmd/cli/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,10 +255,7 @@ func (o *InstallOptions) Run(c *cobra.Command, f client.Factory) error {
return err
}

resources, err = install.AllResources(vo)
if err != nil {
return err
}
resources = install.AllResources(vo)
}

if _, err := output.PrintWithFormat(c, resources); err != nil {
Expand Down
144 changes: 95 additions & 49 deletions pkg/cmd/cli/uninstall/uninstall.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,57 +19,126 @@ package uninstall
import (
"context"
"fmt"
"strings"
"time"

"github.com/pkg/errors"
"github.com/spf13/cobra"
corev1 "k8s.io/api/core/v1"

// apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
kubeerrs "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"

apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
kbclient "sigs.k8s.io/controller-runtime/pkg/client"

"github.com/vmware-tanzu/velero/pkg/client"
"github.com/vmware-tanzu/velero/pkg/cmd"
"github.com/vmware-tanzu/velero/pkg/cmd/cli"
"github.com/vmware-tanzu/velero/pkg/cmd/util/output"
"github.com/vmware-tanzu/velero/pkg/install"
"github.com/vmware-tanzu/velero/pkg/util/kube"
)

// Uninstall uninstalls all components deployed using velero install command
func Uninstall(ctx context.Context, client *kubernetes.Clientset, extensionsClient *apiextensionsclientset.Clientset, veleroNamespace string) error {
if veleroNamespace == "" {
veleroNamespace = "velero"
// NewCommand creates a cobra command.
func NewCommand(f client.Factory) *cobra.Command {
c := &cobra.Command{
Use: "uninstall",
Short: "Uninstall Velero",
Long: `Uninstall Velero along with the CRDs.

The '--namespace' flag can be used to specify the namespace where velero is installed (default: velero).
`,
Example: `# velero uninstall -n backup`,
Run: func(c *cobra.Command, args []string) {

if !cli.GetConfirmation() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a --quiet or --force option so this is scriptable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, thanks.

// Don't do anything unless we get confirmation
return
}

kbClient, err := f.KubebuilderClient()
cmd.CheckError(err)
cmd.CheckError(Run(kbClient, f.Namespace()))
},
}
err := DeleteNamespace(ctx, client, veleroNamespace)
if err != nil {
return errors.WithMessagef(err, "Uninstall failed removing Velero namespace %s", veleroNamespace)

output.BindFlags(c.Flags())
output.ClearOutputFlagDefault(c)
return c
}

// Run removes all components that were deployed using the Velero install command
func Run(kbClient kbclient.Client, namespace string) error {
var errs []error

// namespace
ns := &corev1.Namespace{}
key := kbclient.ObjectKey{Name: namespace}
if err := kbClient.Get(context.Background(), key, ns); err != nil {
if apierrors.IsNotFound(err) {
fmt.Printf("Velero installation namespace %q does not exist, skipping.\n", namespace)
} else {
errs = append(errs, errors.WithStack(err))
}
} else {
if ns.Status.Phase == corev1.NamespaceTerminating {
fmt.Printf("Velero installation namespace %q is terminating.\n", namespace)
} else {
if err := kbClient.Delete(context.Background(), ns); err != nil {
errs = append(errs, errors.WithStack(err))
}
}
}

rolebinding := install.ClusterRoleBinding(veleroNamespace)
time.Sleep(time.Second * 60)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I not at all had the intention to leave this here :) - it needs to be user configurable.


err = client.RbacV1().ClusterRoleBindings().Delete(ctx, rolebinding.Name, metav1.DeleteOptions{})
if err != nil {
return errors.WithMessagef(err, "Uninstall failed removing Velero cluster role binding %s", rolebinding)
// rolebinding
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))
}
Comment on lines +99 to +110
Copy link
Member

Choose a reason for hiding this comment

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

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?

}
veleroLabels := labels.FormatLabels(install.Labels())

crds, err := extensionsClient.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{
LabelSelector: veleroLabels,
})
if err != nil {
return errors.WithMessagef(err, "Uninstall failed listing Velero crds")
// CRDs
veleroLabels := labels.FormatLabels(install.Labels())
crdList := apiextv1beta1.CustomResourceDefinitionList{}
opts := kbclient.ListOptions{
Namespace: namespace,
Raw: &metav1.ListOptions{
LabelSelector: veleroLabels,
},
}
for _, removeCRD := range crds.Items {
err = extensionsClient.ApiextensionsV1().CustomResourceDefinitions().Delete(ctx, removeCRD.ObjectMeta.Name, metav1.DeleteOptions{})
if err != nil {
return errors.WithMessagef(err, "Uninstall failed removing CRD %s", removeCRD.ObjectMeta.Name)
if err := kbClient.List(context.Background(), &crdList, &opts); err != nil {
errs = append(errs, errors.WithStack(err))
} else {
if len(crdList.Items) == 0 {
fmt.Print("Velero CRDs do not exist, skipping.\n")
} else {
for _, crd := range crdList.Items {
if err := kbClient.Delete(context.Background(), &crd); err != nil {
errs = append(errs, errors.WithStack(err))
}
}
}
}

fmt.Println("Uninstalled Velero")
if kubeerrs.NewAggregate(errs) != nil {
fmt.Printf("Errors while attempting to uninstall Velero: %q", kubeerrs.NewAggregate(errs))
return kubeerrs.NewAggregate(errs)
}

fmt.Println("Velero uninstalled ⛵")
return nil
}

Expand All @@ -89,26 +158,3 @@ func DeleteNamespace(ctx context.Context, client *kubernetes.Clientset, namespac
return false, nil
})
}

// NewCommand creates a cobra command.
func NewCommand(f client.Factory) *cobra.Command {
c := &cobra.Command{
Use: "uninstall",
Short: "Uninstall Velero",
Long: `Uninstall Velero along with the CRDs.

The '--namespace' flag can be used to specify the namespace where velero is installed (default: velero).
`,
Example: `# velero uninstall -n backup`,
Run: func(c *cobra.Command, args []string) {
veleroNs := strings.TrimSpace(f.Namespace())
cl, extCl, err := kube.GetClusterClient()
cmd.CheckError(err)
cmd.CheckError(Uninstall(context.Background(), cl, extCl, veleroNs))
},
}

output.BindFlags(c.Flags())
output.ClearOutputFlagDefault(c)
return c
}
11 changes: 7 additions & 4 deletions pkg/install/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,16 +234,19 @@ func createResource(r *unstructured.Unstructured, factory client.DynamicFactory,
format := strings.Join([]string{id, ": ", f, "\n"}, "")
fmt.Fprintf(w, format, a...)
}
log("attempting to create resource")

c, err := CreateClient(r, factory, w)
if err != nil {
return err
}

if _, err := c.Create(r); apierrors.IsAlreadyExists(err) {
log("already exists, proceeding")
} else if err != nil {
log("attempting to create resource")
_, err = c.Create(r)
if err != nil {
if apierrors.IsAlreadyExists(err) {
log("already exists, proceeding")
return nil
}
return errors.Wrapf(err, "Error creating resource %s", id)
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/install/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ func AllCRDs() *unstructured.UnstructuredList {

// AllResources returns a list of all resources necessary to install Velero, in the appropriate order, into a Kubernetes cluster.
// Items are unstructured, since there are different data types returned.
func AllResources(o *VeleroOptions) (*unstructured.UnstructuredList, error) {
func AllResources(o *VeleroOptions) *unstructured.UnstructuredList {
resources := AllCRDs()

ns := Namespace(o.Namespace)
Expand Down Expand Up @@ -317,5 +317,5 @@ func AllResources(o *VeleroOptions) (*unstructured.UnstructuredList, error) {
appendUnstructured(resources, ds)
}

return resources, nil
return resources
}
26 changes: 0 additions & 26 deletions pkg/util/kube/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,12 @@ import (
"github.com/pkg/errors"
corev1api "k8s.io/api/core/v1"
apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
corev1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/client-go/tools/clientcmd"
)

// NamespaceAndName returns a string in the format <namespace>/<name>
Expand Down Expand Up @@ -220,26 +217,3 @@ func IsUnstructuredCRDReady(crd *unstructured.Unstructured) (bool, error) {

return (isEstablished && namesAccepted), nil
}

// GetClusterClient instantiates and returns a client for the cluster.
func GetClusterClient() (*kubernetes.Clientset, *apiextensionsclientset.Clientset, error) {
loadingRules := clientcmd.NewDefaultClientConfigLoadingRules()
configOverrides := &clientcmd.ConfigOverrides{}
kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides)
clientConfig, err := kubeConfig.ClientConfig()
if err != nil {
return nil, nil, errors.WithStack(err)
}

client, err := kubernetes.NewForConfig(clientConfig)
if err != nil {
return nil, nil, errors.WithStack(err)
}

extensionClientSet, err := apiextensionsclientset.NewForConfig(clientConfig)
if err != nil {
return nil, nil, errors.WithStack(err)
}

return client, extensionClientSet, nil
}
11 changes: 11 additions & 0 deletions test/e2e/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,14 @@ For example, E2E tests can be run from Velero repository roots using the command
Velero E2E tests uses [Ginkgo](https://onsi.github.io/ginkgo/) testing framework which allows a subset of the tests to be run using the [`-focus` and `-skip`](https://onsi.github.io/ginkgo/#focused-specs) flags to ginkgo.

The `-focus` flag is passed to ginkgo using the `GINKGO_FOCUS` make variable. This can be used to focus on specific tests.

## Adding tests

### API clients
When adding a test, aim to instantiate an API client only once at the beginning of the test. There is a constructor `newTestClient` that facilitates the configuration and instantiation of clients. Also, please use the `kubebuilder` runtime controller client for any new test, as we will phase out usage of `client-go` API clients.

### Test cleanup
If your test creates resources, be sure it is removing these resources everywhere where there is a potential failure point so even if the test ends with a failure, the environemnt will be cleaned up for the next test.

### Tips
Look for the ⛵ emoji printed at the end of each install and uninstall log. There should not be two install/unintall in a row, and there should be tests between an install and an uninstall.
Loading