From 67c9761623052d147a58807caced1c89262fe30d Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 10 Nov 2020 08:40:25 -0800 Subject: [PATCH] Add BeginCreate and BeginUpdate REST hooks These hooks return a "cleanup" func which is called when the top-level operation completes, with an indicator of which result. This is to enable much simpler handling of allocations in Service's REST implementation, in particular. Some discussion in https://github.com/kubernetes/kubernetes/pull/95967 This also adds tests for the almost totally untested Decorator, AfterCreate, and AfterUpdate hooks. --- .../pkg/registry/generic/registry/BUILD | 1 + .../pkg/registry/generic/registry/store.go | 91 +++ .../registry/generic/registry/store_test.go | 747 +++++++++++++++++- 3 files changed, 829 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD index d948a3c0726..2483440c3ad 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD @@ -46,6 +46,7 @@ go_test( "//staging/src/k8s.io/apiserver/pkg/storage/storagebackend/factory:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/testing:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", + "//vendor/github.com/google/gofuzz:go_default_library", ], ) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index bbc167d3996..e55a0ae5aa6 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -55,6 +55,9 @@ import ( // object. type ObjectFunc func(obj runtime.Object) error +// FinishFunc is a function returned by Begin hooks to complete an operation. +type FinishFunc func(ctx context.Context, success bool) + // GenericStore interface can be used for type assertions when we need to access the underlying strategies. type GenericStore interface { GetCreateStrategy() rest.RESTCreateStrategy @@ -146,14 +149,27 @@ type Store struct { // specific cases where storage of the value is not appropriate, since // they cannot be watched. Decorator ObjectFunc + // CreateStrategy implements resource-specific behavior during creation. CreateStrategy rest.RESTCreateStrategy + // BeginCreate is an optional hook that returns a "transaction-like" + // commit/revert function which will be called at the end of the operation, + // but before AfterCreate and Decorator, indicating via the argument + // whether the operation succeeded. If this returns an error, the function + // is not called. Almost nobody should use this hook. + BeginCreate func(ctx context.Context, obj runtime.Object, options *metav1.CreateOptions) (FinishFunc, error) // AfterCreate implements a further operation to run after a resource is // created and before it is decorated, optional. AfterCreate ObjectFunc // UpdateStrategy implements resource-specific behavior during updates. UpdateStrategy rest.RESTUpdateStrategy + // BeginUpdate is an optional hook that returns a "transaction-like" + // commit/revert function which will be called at the end of the operation, + // but before AfterUpdate and Decorator, indicating via the argument + // whether the operation succeeded. If this returns an error, the function + // is not called. Almost nobody should use this hook. + BeginUpdate func(ctx context.Context, obj, old runtime.Object, options *metav1.UpdateOptions) (FinishFunc, error) // AfterUpdate implements a further operation to run after a resource is // updated and before it is decorated, optional. AfterUpdate ObjectFunc @@ -171,9 +187,11 @@ type Store struct { // If specified, this is checked in addition to standard finalizer, // deletionTimestamp, and deletionGracePeriodSeconds checks. ShouldDeleteDuringUpdate func(ctx context.Context, key string, obj, existing runtime.Object) bool + // ExportStrategy implements resource-specific behavior during export, // optional. Exported objects are not decorated. ExportStrategy rest.RESTExportStrategy + // TableConvertor is an optional interface for transforming items or lists // of items into tabular output. If unset, the default will be used. TableConvertor rest.TableConvertor @@ -335,8 +353,24 @@ func (e *Store) ListPredicate(ctx context.Context, p storage.SelectionPredicate, return list, storeerr.InterpretListError(err, qualifiedResource) } +// finishNothing is a do-nothing FinishFunc. +func finishNothing(context.Context, bool) {} + // Create inserts a new item according to the unique key from the object. func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { + var finishCreate FinishFunc = finishNothing + + if e.BeginCreate != nil { + fn, err := e.BeginCreate(ctx, obj, options) + if err != nil { + return nil, err + } + finishCreate = fn + defer func() { + finishCreate(ctx, false) + }() + } + if err := rest.BeforeCreate(e.CreateStrategy, ctx, obj); err != nil { return nil, err } @@ -381,6 +415,12 @@ func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation } return nil, err } + // The operation has succeeded. Call the finish function if there is one, + // and then make sure the defer doesn't call it again. + fn := finishCreate + finishCreate = finishNothing + fn(ctx, true) + if e.AfterCreate != nil { if err := e.AfterCreate(out); err != nil { return nil, err @@ -500,6 +540,19 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj doUnconditionalUpdate := newResourceVersion == 0 && e.UpdateStrategy.AllowUnconditionalUpdate() if existingResourceVersion == 0 { + var finishCreate FinishFunc = finishNothing + + if e.BeginCreate != nil { + fn, err := e.BeginCreate(ctx, obj, newCreateOptionsFromUpdateOptions(options)) + if err != nil { + return nil, nil, err + } + finishCreate = fn + defer func() { + finishCreate(ctx, false) + }() + } + creating = true creatingObj = obj if err := rest.BeforeCreate(e.CreateStrategy, ctx, obj); err != nil { @@ -517,6 +570,12 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj return nil, nil, err } + // The operation has succeeded. Call the finish function if there is one, + // and then make sure the defer doesn't call it again. + fn := finishCreate + finishCreate = finishNothing + fn(ctx, true) + return obj, &ttl, nil } @@ -544,6 +603,20 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj return nil, nil, apierrors.NewConflict(qualifiedResource, name, fmt.Errorf(OptimisticLockErrorMsg)) } } + + var finishUpdate FinishFunc = finishNothing + + if e.BeginUpdate != nil { + fn, err := e.BeginUpdate(ctx, obj, existing, options) + if err != nil { + return nil, nil, err + } + finishUpdate = fn + defer func() { + finishUpdate(ctx, false) + }() + } + if err := rest.BeforeUpdate(e.UpdateStrategy, ctx, obj, existing); err != nil { return nil, nil, err } @@ -564,6 +637,13 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj if err != nil { return nil, nil, err } + + // The operation has succeeded. Call the finish function if there is one, + // and then make sure the defer doesn't call it again. + fn := finishUpdate + finishUpdate = finishNothing + fn(ctx, true) + if int64(ttl) != res.TTL { return obj, &ttl, nil } @@ -605,6 +685,17 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj return out, creating, nil } +// This is a helper to convert UpdateOptions to CreateOptions for the +// create-on-update path. +func newCreateOptionsFromUpdateOptions(in *metav1.UpdateOptions) *metav1.CreateOptions { + co := &metav1.CreateOptions{ + DryRun: in.DryRun, + FieldManager: in.FieldManager, + } + co.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("CreateOptions")) + return co +} + // Get retrieves the item from storage. func (e *Store) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { obj := e.NewFunc() diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index cf679d066a8..13fcd997545 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -18,6 +18,7 @@ package registry import ( "context" + "encoding/json" "fmt" "path" "reflect" @@ -27,6 +28,7 @@ import ( "testing" "time" + fuzz "github.com/google/gofuzz" apitesting "k8s.io/apimachinery/pkg/api/apitesting" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" @@ -310,11 +312,6 @@ func TestStoreCreate(t *testing.T) { // re-define delete strategy to have graceful delete capability defaultDeleteStrategy := testRESTStrategy{scheme, names.SimpleNameGenerator, true, false, true} registry.DeleteStrategy = testGracefulStrategy{defaultDeleteStrategy} - registry.Decorator = func(obj runtime.Object) error { - pod := obj.(*example.Pod) - pod.Status.Phase = example.PodPhase("Testing") - return nil - } // create the object with denying admission _, err := registry.Create(testContext, podA, denyCreateValidation, &metav1.CreateOptions{}) @@ -328,11 +325,6 @@ func TestStoreCreate(t *testing.T) { t.Errorf("Unexpected error: %v", err) } - // verify the decorator was called - if objA.(*example.Pod).Status.Phase != example.PodPhase("Testing") { - t.Errorf("Decorator was not called: %#v", objA) - } - // get the object checkobj, err := registry.Get(testContext, podA.Name, &metav1.GetOptions{}) if err != nil { @@ -376,6 +368,245 @@ func TestStoreCreate(t *testing.T) { } } +func TestNewCreateOptionsFromUpdateOptions(t *testing.T) { + f := fuzz.New().NilChance(0.0).NumElements(1, 1) + + // The goal here is to trigger when any changes are made to either + // CreateOptions or UpdateOptions types, so we can update the converter. + for i := 0; i < 20; i++ { + in := &metav1.UpdateOptions{} + f.Fuzz(in) + in.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("CreateOptions")) + + out := newCreateOptionsFromUpdateOptions(in) + + // This sequence is intending to elide type information, but produce an + // intermediate structure (map) that can be manually patched up to make + // the comparison work as needed. + + // Convert both structs to maps of primitives. + inBytes, err := json.Marshal(in) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err := json.Marshal(out) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + inMap := map[string]interface{}{} + if err := json.Unmarshal(inBytes, &inMap); err != nil { + t.Fatalf("failed to json.Unmarshal(in): %v", err) + } + outMap := map[string]interface{}{} + if err := json.Unmarshal(outBytes, &outMap); err != nil { + t.Fatalf("failed to json.Unmarshal(out): %v", err) + } + + // Patch the maps to handle any expected differences before we compare + // - none for now. + + // Compare the results. + inBytes, err = json.Marshal(inMap) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err = json.Marshal(outMap) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + if i, o := string(inBytes), string(outBytes); i != o { + t.Fatalf("output != input:\n want: %s\n got: %s", i, o) + } + } +} + +func TestStoreCreateHooks(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + setAnn := func(obj runtime.Object, key string) { + pod := obj.(*example.Pod) + if pod.Annotations == nil { + pod.Annotations = make(map[string]string) + } + pod.Annotations[key] = "true" + } + mile := func(s string) { + milestones = append(milestones, s) + } + + testCases := []struct { + name string + decorator ObjectFunc + beginCreate func(context.Context, runtime.Object, *metav1.CreateOptions) (FinishFunc, error) + afterCreate ObjectFunc + // the TTLFunc is an easy hook to force a failure + ttl func(obj runtime.Object, existing uint64, update bool) (uint64, error) + expectError bool + expectAnnotation string // to test object mutations + expectMilestones []string // to test sequence + }{{ + name: "no hooks", + }, { + name: "Decorator mutation", + decorator: func(obj runtime.Object) error { + setAnn(obj, "DecoratorWasCalled") + return nil + }, + expectAnnotation: "DecoratorWasCalled", + }, { + name: "AfterCreate mutation", + afterCreate: func(obj runtime.Object) error { + setAnn(obj, "AfterCreateWasCalled") + return nil + }, + expectAnnotation: "AfterCreateWasCalled", + }, { + name: "BeginCreate mutation", + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + setAnn(obj, "BeginCreateWasCalled") + return func(context.Context, bool) {}, nil + }, + expectAnnotation: "BeginCreateWasCalled", + }, { + name: "success ordering", + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, + }, { + name: "fail ordering", + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + ttl: func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + }, + expectMilestones: []string{"BeginCreate", "TTLError", "FinishCreate(false)"}, + expectError: true, + }, { + name: "fail Decorator ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return fmt.Errorf("decorator") + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, + }, { + name: "fail AfterCreate ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return fmt.Errorf("after") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate"}, + }, { + name: "fail BeginCreate ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, fmt.Errorf("begin") + }, + expectMilestones: []string{"BeginCreate"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.Decorator = tc.decorator + registry.BeginCreate = tc.beginCreate + registry.AfterCreate = tc.afterCreate + registry.TTLFunc = tc.ttl + + // create the object + milestones = nil + obj, err := registry.Create(testContext, pod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectAnnotation != "" { + out := obj.(*example.Pod) + if v, found := out.Annotations[tc.expectAnnotation]; !found { + t.Errorf("Expected annotation %q not found", tc.expectAnnotation) + } else if v != "true" { + t.Errorf("Expected annotation %q has wrong value: %q", tc.expectAnnotation, v) + } + } + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + func isQualifiedResource(err error, kind, group string) bool { if err.(errors.APIStatus).Status().Details.Kind != kind || err.(errors.APIStatus).Status().Details.Group != group { return false @@ -531,6 +762,502 @@ func TestNoOpUpdates(t *testing.T) { } } +func TestStoreUpdateHooks(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + setAnn := func(obj runtime.Object, key string) { + pod := obj.(*example.Pod) + if pod.Annotations == nil { + pod.Annotations = make(map[string]string) + } + pod.Annotations[key] = "true" + } + mile := func(s string) { + milestones = append(milestones, s) + } + + testCases := []struct { + name string + decorator ObjectFunc + // create-on-update is tested elsewhere, but this proves non-use here + beginCreate func(context.Context, runtime.Object, *metav1.CreateOptions) (FinishFunc, error) + afterCreate ObjectFunc + beginUpdate func(context.Context, runtime.Object, runtime.Object, *metav1.UpdateOptions) (FinishFunc, error) + afterUpdate ObjectFunc + expectError bool + expectAnnotation string // to test object mutations + expectMilestones []string // to test sequence + }{{ + name: "no hooks", + }, { + name: "Decorator mutation", + decorator: func(obj runtime.Object) error { + setAnn(obj, "DecoratorWasCalled") + return nil + }, + expectAnnotation: "DecoratorWasCalled", + }, { + name: "AfterUpdate mutation", + afterUpdate: func(obj runtime.Object) error { + setAnn(obj, "AfterUpdateWasCalled") + return nil + }, + expectAnnotation: "AfterUpdateWasCalled", + }, { + name: "BeginUpdate mutation", + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + setAnn(obj, "BeginUpdateWasCalled") + return func(context.Context, bool) {}, nil + }, + expectAnnotation: "BeginUpdateWasCalled", + }, { + name: "success ordering", + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginUpdate", "FinishUpdate(true)", "AfterUpdate", "Decorator"}, + }, /* fail ordering is covered in TestStoreUpdateHooksInnerRetry */ { + name: "fail Decorator ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return fmt.Errorf("decorator") + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginUpdate", "FinishUpdate(true)", "AfterUpdate", "Decorator"}, + }, { + name: "fail AfterUpdate ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return fmt.Errorf("after") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginUpdate", "FinishUpdate(true)", "AfterUpdate"}, + }, { + name: "fail BeginUpdate ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, fmt.Errorf("begin") + }, + expectMilestones: []string{"BeginUpdate"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.BeginUpdate = tc.beginUpdate + registry.AfterUpdate = tc.afterUpdate + registry.BeginCreate = tc.beginCreate + registry.AfterCreate = tc.afterCreate + + _, err := registry.Create(testContext, pod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + milestones = nil + registry.Decorator = tc.decorator + obj, _, err := registry.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectAnnotation != "" { + out := obj.(*example.Pod) + if v, found := out.Annotations[tc.expectAnnotation]; !found { + t.Errorf("Expected annotation %q not found", tc.expectAnnotation) + } else if v != "true" { + t.Errorf("Expected annotation %q has wrong value: %q", tc.expectAnnotation, v) + } + } + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + +func TestStoreCreateOnUpdateHooks(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + mile := func(s string) { + milestones = append(milestones, s) + } + + testCases := []struct { + name string + decorator ObjectFunc + beginCreate func(context.Context, runtime.Object, *metav1.CreateOptions) (FinishFunc, error) + afterCreate ObjectFunc + beginUpdate func(context.Context, runtime.Object, runtime.Object, *metav1.UpdateOptions) (FinishFunc, error) + afterUpdate ObjectFunc + // the TTLFunc is an easy hook to force a failure + ttl func(obj runtime.Object, existing uint64, update bool) (uint64, error) + expectError bool + expectMilestones []string // to test sequence + }{{ + name: "no hooks", + }, { + name: "success ordering", + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, + }, { + name: "fail ordering", + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + ttl: func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + }, + expectMilestones: []string{"BeginCreate", "TTLError", "FinishCreate(false)"}, + expectError: true, + }, { + name: "fail Decorator ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return fmt.Errorf("decorator") + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, + }, { + name: "fail AfterCreate ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return fmt.Errorf("after") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate"}, + }, { + name: "fail BeginCreate ordering", + expectError: true, + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterCreate: func(obj runtime.Object) error { + mile("AfterCreate") + return nil + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, fmt.Errorf("begin") + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.Decorator = tc.decorator + registry.UpdateStrategy.(*testRESTStrategy).allowCreateOnUpdate = true + registry.BeginUpdate = tc.beginUpdate + registry.AfterUpdate = tc.afterUpdate + registry.BeginCreate = tc.beginCreate + registry.AfterCreate = tc.afterCreate + registry.TTLFunc = tc.ttl + + // NB: did not create it first. + milestones = nil + _, _, err := registry.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + +func TestStoreUpdateHooksInnerRetry(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + mile := func(s string) { + milestones = append(milestones, s) + } + ttlFailDone := false + ttlFailOnce := func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + if ttlFailDone { + mile("TTL") + return existing, nil + } + ttlFailDone = true + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + } + ttlFailAlways := func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + } + + testCases := []struct { + name string + decorator func(runtime.Object) error + beginUpdate func(context.Context, runtime.Object, runtime.Object, *metav1.UpdateOptions) (FinishFunc, error) + afterUpdate func(runtime.Object) error + // the TTLFunc is an easy hook to force an inner-loop retry + ttl func(obj runtime.Object, existing uint64, update bool) (uint64, error) + expectError bool + expectMilestones []string // to test sequence + }{{ + name: "inner retry success", + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + ttl: ttlFailOnce, + expectMilestones: []string{"BeginUpdate", "TTLError", "FinishUpdate(false)", "BeginUpdate", "TTL", "FinishUpdate(true)", "AfterUpdate", "Decorator"}, + }, { + name: "inner retry fail", + decorator: func(obj runtime.Object) error { + mile("Decorator") + return nil + }, + afterUpdate: func(obj runtime.Object) error { + mile("AfterUpdate") + return nil + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + ttl: ttlFailAlways, + expectError: true, + expectMilestones: []string{"BeginUpdate", "TTLError", "FinishUpdate(false)", "BeginUpdate", "TTLError", "FinishUpdate(false)"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.BeginUpdate = tc.beginUpdate + registry.AfterUpdate = tc.afterUpdate + + created, err := registry.Create(testContext, pod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + milestones = nil + registry.Decorator = tc.decorator + ttlFailDone = false + registry.TTLFunc = tc.ttl + registry.Storage.Storage = &staleGuaranteedUpdateStorage{Interface: registry.Storage.Storage, cachedObj: created} + _, _, err = registry.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + // TODO: Add a test to check no-op update if we have object with ResourceVersion // already stored in etcd. Currently there is no easy way to store object with // ResourceVersion in etcd.