From d1bd5277fb76212d101d9a970390b4b3905dab55 Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Sat, 16 Apr 2022 17:19:06 +0530 Subject: [PATCH] Update comment and declaration of `storage.GuaranteedUpdate` to be clearer. Signed-off-by: Sanskar Jaiswal Kubernetes-commit: f8df26ae803103f82edbf1efe17b1b169801b256 --- pkg/endpoints/handlers/patch.go | 8 ++++---- pkg/registry/generic/registry/dryrun.go | 14 +++++++------- pkg/registry/generic/registry/store_test.go | 4 ++-- pkg/storage/cacher/cacher.go | 6 +++--- pkg/storage/etcd3/store.go | 14 +++++++------- pkg/storage/interfaces.go | 15 ++++++++------- 6 files changed, 31 insertions(+), 30 deletions(-) diff --git a/pkg/endpoints/handlers/patch.go b/pkg/endpoints/handlers/patch.go index 5a2af674c..3cc2c52a8 100644 --- a/pkg/endpoints/handlers/patch.go +++ b/pkg/endpoints/handlers/patch.go @@ -506,11 +506,11 @@ func (p *applyPatcher) createNewObject(requestContext context.Context) (runtime. return p.applyPatchToCurrentObject(requestContext, obj) } -// strategicPatchObject applies a strategic merge patch of to -// and stores the result in . +// strategicPatchObject applies a strategic merge patch of `patchBytes` to +// `originalObject` and stores the result in `objToUpdate`. // It additionally returns the map[string]interface{} representation of the -// and . -// NOTE: Both and are supposed to be versioned. +// `originalObject` and `patchBytes`. +// NOTE: Both `originalObject` and `objToUpdate` are supposed to be versioned. func strategicPatchObject( requestContext context.Context, defaulter runtime.ObjectDefaulter, diff --git a/pkg/registry/generic/registry/dryrun.go b/pkg/registry/generic/registry/dryrun.go index 186537d19..1d9125338 100644 --- a/pkg/registry/generic/registry/dryrun.go +++ b/pkg/registry/generic/registry/dryrun.go @@ -69,28 +69,28 @@ func (s *DryRunnableStorage) GetList(ctx context.Context, key string, opts stora } func (s *DryRunnableStorage) GuaranteedUpdate( - ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, + ctx context.Context, key string, destination runtime.Object, ignoreNotFound bool, preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, dryRun bool, cachedExistingObject runtime.Object) error { if dryRun { - err := s.Storage.Get(ctx, key, storage.GetOptions{IgnoreNotFound: ignoreNotFound}, ptrToType) + err := s.Storage.Get(ctx, key, storage.GetOptions{IgnoreNotFound: ignoreNotFound}, destination) if err != nil { return err } - err = preconditions.Check(key, ptrToType) + err = preconditions.Check(key, destination) if err != nil { return err } - rev, err := s.Versioner().ObjectResourceVersion(ptrToType) + rev, err := s.Versioner().ObjectResourceVersion(destination) if err != nil { return err } - out, _, err := tryUpdate(ptrToType, storage.ResponseMeta{ResourceVersion: rev}) + updated, _, err := tryUpdate(destination, storage.ResponseMeta{ResourceVersion: rev}) if err != nil { return err } - return s.copyInto(out, ptrToType) + return s.copyInto(updated, destination) } - return s.Storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, cachedExistingObject) + return s.Storage.GuaranteedUpdate(ctx, key, destination, ignoreNotFound, preconditions, tryUpdate, cachedExistingObject) } func (s *DryRunnableStorage) Count(key string) (int64, error) { diff --git a/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index 40bfccc44..c7c6bef3c 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -2627,9 +2627,9 @@ type staleGuaranteedUpdateStorage struct { // GuaranteedUpdate overwrites the method with one that always suggests the cachedObj. func (s *staleGuaranteedUpdateStorage) GuaranteedUpdate( - ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, + ctx context.Context, key string, destination runtime.Object, ignoreNotFound bool, preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, _ runtime.Object) error { - return s.Interface.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, s.cachedObj) + return s.Interface.GuaranteedUpdate(ctx, key, destination, ignoreNotFound, preconditions, tryUpdate, s.cachedObj) } func TestDeleteWithCachedObject(t *testing.T) { diff --git a/pkg/storage/cacher/cacher.go b/pkg/storage/cacher/cacher.go index 84940ab0c..e6eab856a 100644 --- a/pkg/storage/cacher/cacher.go +++ b/pkg/storage/cacher/cacher.go @@ -693,7 +693,7 @@ func (c *Cacher) GetList(ctx context.Context, key string, opts storage.ListOptio // GuaranteedUpdate implements storage.Interface. func (c *Cacher) GuaranteedUpdate( - ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, + ctx context.Context, key string, destination runtime.Object, ignoreNotFound bool, preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, _ runtime.Object) error { // Ignore the suggestion and try to pass down the current version of the object // read from cache. @@ -703,10 +703,10 @@ func (c *Cacher) GuaranteedUpdate( // DeepCopy the object since we modify resource version when serializing the // current object. currObj := elem.(*storeElement).Object.DeepCopyObject() - return c.storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, currObj) + return c.storage.GuaranteedUpdate(ctx, key, destination, ignoreNotFound, preconditions, tryUpdate, currObj) } // If we couldn't get the object, fallback to no-suggestion. - return c.storage.GuaranteedUpdate(ctx, key, ptrToType, ignoreNotFound, preconditions, tryUpdate, nil) + return c.storage.GuaranteedUpdate(ctx, key, destination, ignoreNotFound, preconditions, tryUpdate, nil) } // Count implements storage.Interface. diff --git a/pkg/storage/etcd3/store.go b/pkg/storage/etcd3/store.go index 75a6c3b58..2de3192bd 100644 --- a/pkg/storage/etcd3/store.go +++ b/pkg/storage/etcd3/store.go @@ -316,12 +316,12 @@ func (s *store) conditionalDelete( // GuaranteedUpdate implements storage.Interface.GuaranteedUpdate. func (s *store) GuaranteedUpdate( - ctx context.Context, key string, out runtime.Object, ignoreNotFound bool, + ctx context.Context, key string, destination runtime.Object, ignoreNotFound bool, preconditions *storage.Preconditions, tryUpdate storage.UpdateFunc, cachedExistingObject runtime.Object) error { - trace := utiltrace.New("GuaranteedUpdate etcd3", utiltrace.Field{"type", getTypeName(out)}) + trace := utiltrace.New("GuaranteedUpdate etcd3", utiltrace.Field{"type", getTypeName(destination)}) defer trace.LogIfLong(500 * time.Millisecond) - v, err := conversion.EnforcePtr(out) + v, err := conversion.EnforcePtr(destination) if err != nil { return fmt.Errorf("unable to convert output object to pointer: %v", err) } @@ -330,7 +330,7 @@ func (s *store) GuaranteedUpdate( getCurrentState := func() (*objState, error) { startTime := time.Now() getResp, err := s.client.KV.Get(ctx, key) - metrics.RecordEtcdRequestLatency("get", getTypeName(out), startTime) + metrics.RecordEtcdRequestLatency("get", getTypeName(destination), startTime) if err != nil { return nil, err } @@ -418,7 +418,7 @@ func (s *store) GuaranteedUpdate( } // recheck that the data from etcd is not stale before short-circuiting a write if !origState.stale { - return decode(s.codec, s.versioner, origState.data, out, origState.rev) + return decode(s.codec, s.versioner, origState.data, destination, origState.rev) } } @@ -441,7 +441,7 @@ func (s *store) GuaranteedUpdate( ).Else( clientv3.OpGet(key), ).Commit() - metrics.RecordEtcdRequestLatency("update", getTypeName(out), startTime) + metrics.RecordEtcdRequestLatency("update", getTypeName(destination), startTime) if err != nil { return err } @@ -459,7 +459,7 @@ func (s *store) GuaranteedUpdate( } putResp := txnResp.Responses[0].GetResponsePut() - return decode(s.codec, s.versioner, data, out, putResp.Header.Revision) + return decode(s.codec, s.versioner, data, destination, putResp.Header.Revision) } } diff --git a/pkg/storage/interfaces.go b/pkg/storage/interfaces.go index 6e77ea700..812aa412b 100644 --- a/pkg/storage/interfaces.go +++ b/pkg/storage/interfaces.go @@ -198,15 +198,16 @@ type Interface interface { // match 'opts.ResourceVersion' according 'opts.ResourceVersionMatch'. GetList(ctx context.Context, key string, opts ListOptions, listObj runtime.Object) error - // GuaranteedUpdate keeps calling 'tryUpdate()' to update key 'key' (of type 'ptrToType') + // GuaranteedUpdate keeps calling 'tryUpdate()' to update key 'key' (of type 'destination') // retrying the update until success if there is index conflict. // Note that object passed to tryUpdate may change across invocations of tryUpdate() if // other writers are simultaneously updating it, so tryUpdate() needs to take into account // the current contents of the object when deciding how the update object should look. // If the key doesn't exist, it will return NotFound storage error if ignoreNotFound=false - // or zero value in 'ptrToType' parameter otherwise. - // If the object to update has the same value as previous, it won't do any update - // but will return the object in 'ptrToType' parameter. + // else `destination` will be set to the zero value of it's type. + // If the eventual successful invocation of `tryUpdate` returns an output with the same serialized + // contents as the input, it won't perform any update, but instead set `destination` to an object with those + // contents. // If 'cachedExistingObject' is non-nil, it can be used as a suggestion about the // current version of the object to avoid read operation from storage to get it. // However, the implementations have to retry in case suggestion is stale. @@ -215,7 +216,7 @@ type Interface interface { // // s := /* implementation of Interface */ // err := s.GuaranteedUpdate( - // "myKey", &MyType{}, true, + // "myKey", &MyType{}, true, preconditions, // func(input runtime.Object, res ResponseMeta) (runtime.Object, *uint64, error) { // // Before each invocation of the user defined function, "input" is reset to // // current contents for "myKey" in database. @@ -227,10 +228,10 @@ type Interface interface { // // Return the modified object - return an error to stop iterating. Return // // a uint64 to alter the TTL on the object, or nil to keep it the same value. // return cur, nil, nil - // }, + // }, cachedExistingObject // ) GuaranteedUpdate( - ctx context.Context, key string, ptrToType runtime.Object, ignoreNotFound bool, + ctx context.Context, key string, destination runtime.Object, ignoreNotFound bool, preconditions *Preconditions, tryUpdate UpdateFunc, cachedExistingObject runtime.Object) error // Count returns number of different entries under the key (generally being path prefix).