Skip to content

Commit

Permalink
Fix error handling when trying to delete remote resources not present…
Browse files Browse the repository at this point in the history 
… in Devfile (#6659)

* Fix condition for checking errors while trying to delete resources not present in Devfile

* Add unit tests

* Fix race condition with error handling when deleting remote resources not present in Devfile

Several Goroutines could potentially modify the same shared error variable,
leading to weird behaviour, caught by the unit tests.
Using errgroup allows simplifying goroutine error handling.

* Remove the user-facing spinner while trying to remove remote resources not present in Devfile

Replace with a klog message, as the spinner does not add much value to the end user.

Plus, it keeps displaying because we are regenerating the adapter which calls this function indefinitely.
  • Loading branch information
@rm3l
rm3l authored Mar 16, 2023
1 parent 8a002e1 commit bffb9b0
Show file tree
Hide file tree
Showing 2 changed files with 208 additions and 22 deletions.
39 changes: 17 additions & 22 deletions pkg/devfile/adapters/kubernetes/component/adapter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@ import (
"fmt"
"reflect"
"strings"
sync2 "sync"

devfilefs "github.com/devfile/library/v2/pkg/testingutil/filesystem"
"golang.org/x/sync/errgroup"
kerrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/utils/pointer"

"github.com/redhat-developer/odo/pkg/binding"
Expand Down Expand Up @@ -693,40 +692,36 @@ func (a Adapter) deleteRemoteResources(objectsToRemove []unstructured.Unstructur
resources = append(resources, fmt.Sprintf("%s/%s", u.GetKind(), u.GetName()))
}

var err error
spinner := log.Spinnerf("Deleting resources not present in the Devfile: %s", strings.Join(resources, ", "))
defer spinner.End(err == nil)

var wg sync2.WaitGroup
// Delete the resources present on the cluster but not in the Devfile
klog.V(3).Infof("Deleting %d resource(s) not present in the Devfile: %s", len(resources), strings.Join(resources, ", "))
g := new(errgroup.Group)
for _, objectToRemove := range objectsToRemove {
wg.Add(1)
// Avoid re-use of the same `objectToRemove` value in each goroutine closure.
// See https://golang.org/doc/faq#closures_and_goroutines for more details.
objectToRemove := objectToRemove
go func() {
defer wg.Done()
var gvr schema.GroupVersionResource
gvr, err = a.kubeClient.GetGVRFromGVK(objectToRemove.GroupVersionKind())
g.Go(func() error {
gvr, err := a.kubeClient.GetGVRFromGVK(objectToRemove.GroupVersionKind())
if err != nil {
err = fmt.Errorf("unable to get information about resource: %s/%s: %s", objectToRemove.GetKind(), objectToRemove.GetName(), err.Error())
return
return fmt.Errorf("unable to get information about resource: %s/%s: %w", objectToRemove.GetKind(), objectToRemove.GetName(), err)
}

err = a.kubeClient.DeleteDynamicResource(objectToRemove.GetName(), gvr, true)
if err != nil {
if !kerrors.IsNotFound(err) || !kerrors.IsMethodNotSupported(err) {
err = fmt.Errorf("unable to delete resource: %s/%s: %s", objectToRemove.GetKind(), objectToRemove.GetName(), err.Error())
return
if !(kerrors.IsNotFound(err) || kerrors.IsMethodNotSupported(err)) {
return fmt.Errorf("unable to delete resource: %s/%s: %w", objectToRemove.GetKind(), objectToRemove.GetName(), err)
}
klog.V(1).Infof("Failed to delete resource: %s/%s; resource not found", objectToRemove.GetKind(), objectToRemove.GetName())
err = nil
klog.V(3).Infof("Failed to delete resource: %s/%s; resource not found or method not supported", objectToRemove.GetKind(), objectToRemove.GetName())
}
}()

return nil
})
}

wg.Wait()
return err
if err := g.Wait(); err != nil {
return err
}

return nil
}

// deleteServiceBindingSecrets takes a list of Service Binding secrets(unstructured) that should be deleted;
Expand Down
191 changes: 191 additions & 0 deletions pkg/devfile/adapters/kubernetes/component/adapter_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
package component

import (
"errors"
"testing"

"github.com/devfile/library/v2/pkg/devfile/generator"
"github.com/devfile/library/v2/pkg/devfile/parser/data"
"github.com/golang/mock/gomock"
"github.com/google/go-cmp/cmp"
kerrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"

"github.com/redhat-developer/odo/pkg/libdevfile"
"github.com/redhat-developer/odo/pkg/preference"
Expand Down Expand Up @@ -244,3 +248,190 @@ func TestAdapter_generateDeploymentObjectMeta(t *testing.T) {
})
}
}

func TestAdapter_deleteRemoteResources(t *testing.T) {
type fields struct {
kubeClientCustomizer func(kubeClient *kclient.MockClientInterface)
}
type args struct {
objectsToRemove []unstructured.Unstructured
}

var u1 unstructured.Unstructured
u1.SetGroupVersionKind(schema.GroupVersionKind{
Group: "metrics.k8s.io",
Version: "v1beta1",
Kind: "PodMetrics",
})
u1.SetName("my-pod-metrics")

var u2 unstructured.Unstructured
u2.SetGroupVersionKind(schema.GroupVersionKind{
Group: "postgresql.k8s.enterprisedb.io",
Version: "v1",
Kind: "Cluster",
})
u2.SetName("my-pg-cluster")
toRemove := []unstructured.Unstructured{u1, u2}

tests := []struct {
name string
fields fields
args args
wantErr bool
}{
{
name: "nothing to delete - nil list",
args: args{
objectsToRemove: nil,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().DeleteDynamicResource(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
},
},
wantErr: false,
},
{
name: "nothing to delete - empty list",
args: args{
objectsToRemove: []unstructured.Unstructured{},
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().DeleteDynamicResource(gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
},
},
wantErr: false,
},
{
name: "error getting information about resource",
args: args{
objectsToRemove: toRemove,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u1.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u2.GroupVersionKind())).Return(schema.GroupVersionResource{}, errors.New("error on GetGVRFromGVK(u2)"))
// Only u1 should be deleted
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u1.GetName()), gomock.Any(), gomock.Any()).Return(nil).Times(1)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u2.GetName()), gomock.Any(), gomock.Any()).Times(0)
},
},
wantErr: true,
},
{
name: "generic error deleting resource",
args: args{
objectsToRemove: toRemove,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u1.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u2.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u1.GetName()), gomock.Any(), gomock.Any()).Return(nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u2.GetName()), gomock.Any(), gomock.Any()).Return(errors.New("generic error while deleting u1"))
},
},
wantErr: true,
},
{
name: "generic error deleting all resources",
args: args{
objectsToRemove: toRemove,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u1.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u2.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u1.GetName()), gomock.Any(), gomock.Any()).
Return(errors.New("generic error while deleting u1"))
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u2.GetName()), gomock.Any(), gomock.Any()).
Return(errors.New("generic error while deleting u2"))
},
},
wantErr: true,
},
{
name: "not found error deleting resource",
args: args{
objectsToRemove: toRemove,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u1.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u2.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u1.GetName()), gomock.Any(), gomock.Any()).Return(nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u2.GetName()), gomock.Any(), gomock.Any()).
Return(kerrors.NewNotFound(schema.GroupResource{}, "u2"))
},
},
wantErr: false,
},
{
name: "method not allowed error deleting resource",
args: args{
objectsToRemove: toRemove,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u1.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u2.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u1.GetName()), gomock.Any(), gomock.Any()).Return(nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u2.GetName()), gomock.Any(), gomock.Any()).
Return(kerrors.NewMethodNotSupported(schema.GroupResource{Resource: "PodMetrics"}, "DELETE"))
},
},
wantErr: false,
},
{
name: "not found error deleting all resources",
args: args{
objectsToRemove: toRemove,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u1.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u2.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u1.GetName()), gomock.Any(), gomock.Any()).
Return(kerrors.NewNotFound(schema.GroupResource{}, "u1"))
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u2.GetName()), gomock.Any(), gomock.Any()).
Return(kerrors.NewNotFound(schema.GroupResource{}, "u2"))
},
},
wantErr: false,
},
{
name: "method not allowed error deleting all resources",
args: args{
objectsToRemove: toRemove,
},
fields: fields{
kubeClientCustomizer: func(kubeClient *kclient.MockClientInterface) {
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u1.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().GetGVRFromGVK(gomock.Eq(u2.GroupVersionKind())).Return(schema.GroupVersionResource{}, nil)
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u1.GetName()), gomock.Any(), gomock.Any()).
Return(kerrors.NewMethodNotSupported(schema.GroupResource{}, "DELETE"))
kubeClient.EXPECT().DeleteDynamicResource(gomock.Eq(u2.GetName()), gomock.Any(), gomock.Any()).
Return(kerrors.NewMethodNotSupported(schema.GroupResource{}, "DELETE"))
},
},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctrl := gomock.NewController(t)
kubeClient := kclient.NewMockClientInterface(ctrl)
if tt.fields.kubeClientCustomizer != nil {
tt.fields.kubeClientCustomizer(kubeClient)
}
a := Adapter{
kubeClient: kubeClient,
}
if err := a.deleteRemoteResources(tt.args.objectsToRemove); (err != nil) != tt.wantErr {
t.Errorf("deleteRemoteResources() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}

0 comments on commit bffb9b0

Please sign in to comment.