diff --git a/changelogs/unreleased/6949-kaovilai b/changelogs/unreleased/6949-kaovilai new file mode 100644 index 0000000000..f148a21a20 --- /dev/null +++ b/changelogs/unreleased/6949-kaovilai @@ -0,0 +1 @@ +On restore, retry Get when IsNotFound after Create fails with AlreadyExists diff --git a/pkg/client/dynamic.go b/pkg/client/dynamic.go index 0e9655b11b..fffc7c8687 100644 --- a/pkg/client/dynamic.go +++ b/pkg/client/dynamic.go @@ -102,6 +102,8 @@ type StatusUpdater interface { UpdateStatus(obj *unstructured.Unstructured, opts metav1.UpdateOptions) (*unstructured.Unstructured, error) } +//go:generate mockery --name Dynamic + // Dynamic contains client methods that Velero needs for backing up and restoring resources. type Dynamic interface { Creator diff --git a/pkg/client/mocks/Dynamic.go b/pkg/client/mocks/Dynamic.go new file mode 100644 index 0000000000..c75460f3db --- /dev/null +++ b/pkg/client/mocks/Dynamic.go @@ -0,0 +1,200 @@ +// Code generated by mockery v2.35.4. DO NOT EDIT. + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + unstructured "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + watch "k8s.io/apimachinery/pkg/watch" +) + +// Dynamic is an autogenerated mock type for the Dynamic type +type Dynamic struct { + mock.Mock +} + +// Create provides a mock function with given fields: obj +func (_m *Dynamic) Create(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + ret := _m.Called(obj) + + var r0 *unstructured.Unstructured + var r1 error + if rf, ok := ret.Get(0).(func(*unstructured.Unstructured) (*unstructured.Unstructured, error)); ok { + return rf(obj) + } + if rf, ok := ret.Get(0).(func(*unstructured.Unstructured) *unstructured.Unstructured); ok { + r0 = rf(obj) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*unstructured.Unstructured) + } + } + + if rf, ok := ret.Get(1).(func(*unstructured.Unstructured) error); ok { + r1 = rf(obj) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// Delete provides a mock function with given fields: name, opts +func (_m *Dynamic) Delete(name string, opts v1.DeleteOptions) error { + ret := _m.Called(name, opts) + + var r0 error + if rf, ok := ret.Get(0).(func(string, v1.DeleteOptions) error); ok { + r0 = rf(name, opts) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Get provides a mock function with given fields: name, opts +func (_m *Dynamic) Get(name string, opts v1.GetOptions) (*unstructured.Unstructured, error) { + ret := _m.Called(name, opts) + + var r0 *unstructured.Unstructured + var r1 error + if rf, ok := ret.Get(0).(func(string, v1.GetOptions) (*unstructured.Unstructured, error)); ok { + return rf(name, opts) + } + if rf, ok := ret.Get(0).(func(string, v1.GetOptions) *unstructured.Unstructured); ok { + r0 = rf(name, opts) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*unstructured.Unstructured) + } + } + + if rf, ok := ret.Get(1).(func(string, v1.GetOptions) error); ok { + r1 = rf(name, opts) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// List provides a mock function with given fields: _a0 +func (_m *Dynamic) List(_a0 v1.ListOptions) (*unstructured.UnstructuredList, error) { + ret := _m.Called(_a0) + + var r0 *unstructured.UnstructuredList + var r1 error + if rf, ok := ret.Get(0).(func(v1.ListOptions) (*unstructured.UnstructuredList, error)); ok { + return rf(_a0) + } + if rf, ok := ret.Get(0).(func(v1.ListOptions) *unstructured.UnstructuredList); ok { + r0 = rf(_a0) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*unstructured.UnstructuredList) + } + } + + if rf, ok := ret.Get(1).(func(v1.ListOptions) error); ok { + r1 = rf(_a0) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// Patch provides a mock function with given fields: name, data +func (_m *Dynamic) Patch(name string, data []byte) (*unstructured.Unstructured, error) { + ret := _m.Called(name, data) + + var r0 *unstructured.Unstructured + var r1 error + if rf, ok := ret.Get(0).(func(string, []byte) (*unstructured.Unstructured, error)); ok { + return rf(name, data) + } + if rf, ok := ret.Get(0).(func(string, []byte) *unstructured.Unstructured); ok { + r0 = rf(name, data) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*unstructured.Unstructured) + } + } + + if rf, ok := ret.Get(1).(func(string, []byte) error); ok { + r1 = rf(name, data) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// UpdateStatus provides a mock function with given fields: obj, opts +func (_m *Dynamic) UpdateStatus(obj *unstructured.Unstructured, opts v1.UpdateOptions) (*unstructured.Unstructured, error) { + ret := _m.Called(obj, opts) + + var r0 *unstructured.Unstructured + var r1 error + if rf, ok := ret.Get(0).(func(*unstructured.Unstructured, v1.UpdateOptions) (*unstructured.Unstructured, error)); ok { + return rf(obj, opts) + } + if rf, ok := ret.Get(0).(func(*unstructured.Unstructured, v1.UpdateOptions) *unstructured.Unstructured); ok { + r0 = rf(obj, opts) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*unstructured.Unstructured) + } + } + + if rf, ok := ret.Get(1).(func(*unstructured.Unstructured, v1.UpdateOptions) error); ok { + r1 = rf(obj, opts) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// Watch provides a mock function with given fields: _a0 +func (_m *Dynamic) Watch(_a0 v1.ListOptions) (watch.Interface, error) { + ret := _m.Called(_a0) + + var r0 watch.Interface + var r1 error + if rf, ok := ret.Get(0).(func(v1.ListOptions) (watch.Interface, error)); ok { + return rf(_a0) + } + if rf, ok := ret.Get(0).(func(v1.ListOptions) watch.Interface); ok { + r0 = rf(_a0) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(watch.Interface) + } + } + + if rf, ok := ret.Get(1).(func(v1.ListOptions) error); ok { + r1 = rf(_a0) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewDynamic creates a new instance of Dynamic. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewDynamic(t interface { + mock.TestingT + Cleanup(func()) +}) *Dynamic { + mock := &Dynamic{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/client/mocks/Factory.go b/pkg/client/mocks/Factory.go index 3bd0160476..35e314c109 100644 --- a/pkg/client/mocks/Factory.go +++ b/pkg/client/mocks/Factory.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.30.1. DO NOT EDIT. +// Code generated by mockery v2.35.4. DO NOT EDIT. package mocks diff --git a/pkg/client/mocks/GenericNamespaceLister.go b/pkg/client/mocks/GenericNamespaceLister.go new file mode 100644 index 0000000000..d72c88336a --- /dev/null +++ b/pkg/client/mocks/GenericNamespaceLister.go @@ -0,0 +1,81 @@ +// Code generated by mockery v2.35.4. DO NOT EDIT. + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + labels "k8s.io/apimachinery/pkg/labels" + + runtime "k8s.io/apimachinery/pkg/runtime" +) + +// GenericNamespaceLister is an autogenerated mock type for the GenericNamespaceLister type +type GenericNamespaceLister struct { + mock.Mock +} + +// Get provides a mock function with given fields: name +func (_m *GenericNamespaceLister) Get(name string) (runtime.Object, error) { + ret := _m.Called(name) + + var r0 runtime.Object + var r1 error + if rf, ok := ret.Get(0).(func(string) (runtime.Object, error)); ok { + return rf(name) + } + if rf, ok := ret.Get(0).(func(string) runtime.Object); ok { + r0 = rf(name) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(runtime.Object) + } + } + + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(name) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// List provides a mock function with given fields: selector +func (_m *GenericNamespaceLister) List(selector labels.Selector) ([]runtime.Object, error) { + ret := _m.Called(selector) + + var r0 []runtime.Object + var r1 error + if rf, ok := ret.Get(0).(func(labels.Selector) ([]runtime.Object, error)); ok { + return rf(selector) + } + if rf, ok := ret.Get(0).(func(labels.Selector) []runtime.Object); ok { + r0 = rf(selector) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]runtime.Object) + } + } + + if rf, ok := ret.Get(1).(func(labels.Selector) error); ok { + r1 = rf(selector) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewGenericNamespaceLister creates a new instance of GenericNamespaceLister. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewGenericNamespaceLister(t interface { + mock.TestingT + Cleanup(func()) +}) *GenericNamespaceLister { + mock := &GenericNamespaceLister{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/client/retry.go b/pkg/client/retry.go index f9674e1edd..9975af76dd 100644 --- a/pkg/client/retry.go +++ b/pkg/client/retry.go @@ -18,12 +18,29 @@ package client import ( "context" + "time" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/wait" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/retry" kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) +// MinuteBackoff is a retry.DefaultBackoff that retries for at least a minute (60000ms) but no more than 2 minutes (120000ms). +var MinuteBackoff = func() wait.Backoff { + mb := retry.DefaultBackoff + // TotalDuration = 0ms + 10ms + 50ms + 250ms + 1250ms + 6250ms + 31250ms + 60000ms = 99,060 ms > 1 minute + // 7 steps + mb.Steps = 7 + mb.Cap = time.Minute + return mb +}() + func CreateRetryGenerateName(client kbclient.Client, ctx context.Context, obj kbclient.Object) error { return CreateRetryGenerateNameWithFunc(obj, func() error { return client.Create(ctx, obj, &kbclient.CreateOptions{}) @@ -42,3 +59,25 @@ func CreateRetryGenerateNameWithFunc(obj kbclient.Object, createFn func() error) return createFn() } } + +func GetRetriableWithCacheLister(lister cache.GenericNamespaceLister, name string, retriable func(error) bool) (runtime.Object, error) { + var clusterObj runtime.Object + getFunc := func() error { + var err error + clusterObj, err = lister.Get(name) + return err + } + err := retry.OnError(MinuteBackoff, retriable, getFunc) + return clusterObj, err +} + +func GetRetriableWithDynamicClient(client Dynamic, name string, getOptions metav1.GetOptions, retriable func(error) bool) (*unstructured.Unstructured, error) { + var clusterObj *unstructured.Unstructured + getFunc := func() error { + var err error + clusterObj, err = client.Get(name, getOptions) + return err + } + err := retry.OnError(MinuteBackoff, retriable, getFunc) + return clusterObj, err +} diff --git a/pkg/client/retry_mocks.go b/pkg/client/retry_mocks.go new file mode 100644 index 0000000000..c3c7309459 --- /dev/null +++ b/pkg/client/retry_mocks.go @@ -0,0 +1,29 @@ +/* +Copyright the Velero contributors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package client + +import ( + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" +) + +// GenericNamespaceLister is a lister skin on a generic Indexer +// +//go:generate mockery --name GenericNamespaceLister +type GenericNamespaceLister interface { + // List will return all objects in this namespace + List(selector labels.Selector) (ret []runtime.Object, err error) + // Get will attempt to retrieve by namespace and name + Get(name string) (runtime.Object, error) +} diff --git a/pkg/client/retry_test.go b/pkg/client/retry_test.go new file mode 100644 index 0000000000..8eaa90581b --- /dev/null +++ b/pkg/client/retry_test.go @@ -0,0 +1,127 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package client + +import ( + "reflect" + "testing" + + 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/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/vmware-tanzu/velero/pkg/client/mocks" +) + +func TestGetRetriableWithDynamicClient(t *testing.T) { + type args struct { + name string + getOptions metav1.GetOptions + retriable func(error) bool + errToRetry error + } + tests := []struct { + name string + args args + want *unstructured.Unstructured + wantErr bool + }{ + { + name: "retries on not found", + args: args{ + name: "foo", + getOptions: metav1.GetOptions{}, + retriable: apierrors.IsNotFound, + errToRetry: apierrors.NewNotFound(schema.GroupResource{}, ""), + }, + want: &unstructured.Unstructured{}, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dc := mocks.NewDynamic(t) + tries := 0 + dc.On("Get", tt.args.name, tt.args.getOptions).Return(func(name string, getOptions metav1.GetOptions) (*unstructured.Unstructured, error) { + tries++ + t.Logf("try %d", tries) + if tries < 3 { + return nil, tt.args.errToRetry + } + return tt.want, nil + }) + + got, err := GetRetriableWithDynamicClient(dc, tt.args.name, tt.args.getOptions, tt.args.retriable) + if (err != nil) != tt.wantErr { + t.Errorf("GetRetriableWithDynamicClient() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetRetriableWithDynamicClient() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestGetRetriableWithCacheLister(t *testing.T) { + type args struct { + name string + retriable func(error) bool + errToRetry error + } + tests := []struct { + name string + args args + want runtime.Object + wantErr bool + }{ + { + name: "retries on not found", + args: args{ + name: "foo", + retriable: apierrors.IsNotFound, + errToRetry: apierrors.NewNotFound(schema.GroupResource{}, ""), + }, + want: &unstructured.Unstructured{}, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lister := mocks.NewGenericNamespaceLister(t) + tries := 0 + lister.On("Get", tt.args.name).Return(func(name string) (runtime.Object, error) { + tries++ + t.Logf("try %d", tries) + if tries < 3 { + return nil, tt.args.errToRetry + } + return tt.want, nil + }) + got, err := GetRetriableWithCacheLister(lister, tt.args.name, tt.args.retriable) + if (err != nil) != tt.wantErr { + t.Errorf("GetRetriableWithCacheLister() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetRetriableWithCacheLister() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 8355533300..24afa48f15 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1068,9 +1068,18 @@ func getResourceID(groupResource schema.GroupResource, namespace, name string) s return fmt.Sprintf("%s/%s/%s", groupResource.String(), namespace, name) } -func (ctx *restoreContext) getResource(groupResource schema.GroupResource, obj *unstructured.Unstructured, namespace, name string) (*unstructured.Unstructured, error) { +// getResource but with retry on retriable error +func (ctx *restoreContext) getResource(groupResource schema.GroupResource, obj *unstructured.Unstructured, namespace, name string, retriable func(error) bool) (*unstructured.Unstructured, error) { lister := ctx.getResourceLister(groupResource, obj, namespace) - clusterObj, err := lister.Get(name) + var ( + err error + clusterObj runtime.Object + ) + if retriable == nil { + clusterObj, err = lister.Get(name) + } else { + clusterObj, err = client.GetRetriableWithCacheLister(lister, name, retriable) + } if err != nil { return nil, errors.Wrapf(err, "error getting resource from lister for %s, %s/%s", groupResource, namespace, name) } @@ -1505,7 +1514,7 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso // new namespace if !ctx.disableInformerCache { ctx.log.Debugf("Checking for existence %s: %v", obj.GroupVersionKind().Kind, name) - fromCluster, err = ctx.getResource(groupResource, obj, namespace, name) + fromCluster, err = ctx.getResource(groupResource, obj, namespace, name, nil) } if err != nil || fromCluster == nil { // couldn't find the resource, attempt to create @@ -1529,9 +1538,9 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso // if so, we will return the 'get' error. // otherwise, we will return the original creation error. if !ctx.disableInformerCache { - fromCluster, err = ctx.getResource(groupResource, obj, namespace, name) + fromCluster, err = ctx.getResource(groupResource, obj, namespace, name, apierrors.IsNotFound) } else { - fromCluster, err = resourceClient.Get(name, metav1.GetOptions{}) + fromCluster, err = client.GetRetriableWithDynamicClient(resourceClient, name, metav1.GetOptions{}, apierrors.IsNotFound) } if err != nil && isAlreadyExistsError { ctx.log.Errorf("Error retrieving in-cluster version of %s: %v", kube.NamespaceAndName(obj), err)