-
Notifications
You must be signed in to change notification settings - Fork 1.2k
✨ Add Patch MergeFrom optimistic locking option #969
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,11 @@ limitations under the License. | |
package client | ||
|
||
import ( | ||
"fmt" | ||
|
||
jsonpatch "github.com/evanphx/json-patch" | ||
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/types" | ||
"k8s.io/apimachinery/pkg/util/json" | ||
|
@@ -59,8 +63,39 @@ func ConstantPatch(patchType types.PatchType, data []byte) Patch { | |
return RawPatch(patchType, data) | ||
} | ||
|
||
// MergeFromWithOptimisticLock can be used if clients want to make sure a patch | ||
// is being applied to the latest resource version of an object. | ||
// | ||
// The behavior is similar to what an Update would do, without the need to send the | ||
// whole object. Usually this method is useful if you might have multiple clients | ||
// acting on the same object and the same API version, but with different versions of the Go structs. | ||
// | ||
// For example, an "older" copy of a Widget that has fields A and B, and a "newer" copy with A, B, and C. | ||
// Sending an update using the older struct definition results in C being dropped, whereas using a patch does not. | ||
type MergeFromWithOptimisticLock struct{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to read the code to understand what you meant by I also don't understand how that differs from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 I'll add a godoc |
||
|
||
// ApplyToMergeFrom applies this configuration to the given patch options. | ||
func (m MergeFromWithOptimisticLock) ApplyToMergeFrom(in *MergeFromOptions) { | ||
in.OptimisticLock = true | ||
} | ||
|
||
// MergeFromOption is some configuration that modifies options for a merge-from patch data. | ||
type MergeFromOption interface { | ||
// ApplyToMergeFrom applies this configuration to the given patch options. | ||
ApplyToMergeFrom(*MergeFromOptions) | ||
} | ||
|
||
// MergeFromOptions contains options to generate a merge-from patch data. | ||
type MergeFromOptions struct { | ||
// OptimisticLock, when true, includes `metadata.resourceVersion` into the final | ||
// patch data. If the `resourceVersion` field doesn't match what's stored, | ||
// the operation results in a conflict and clients will need to try again. | ||
OptimisticLock bool | ||
} | ||
|
||
type mergeFromPatch struct { | ||
from runtime.Object | ||
opts MergeFromOptions | ||
} | ||
|
||
// Type implements patch. | ||
|
@@ -80,12 +115,47 @@ func (s *mergeFromPatch) Data(obj runtime.Object) ([]byte, error) { | |
return nil, err | ||
} | ||
|
||
return jsonpatch.CreateMergePatch(originalJSON, modifiedJSON) | ||
data, err := jsonpatch.CreateMergePatch(originalJSON, modifiedJSON) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if s.opts.OptimisticLock { | ||
dataMap := map[string]interface{}{} | ||
if err := json.Unmarshal(data, &dataMap); err != nil { | ||
return nil, err | ||
} | ||
fromMeta, ok := s.from.(metav1.Object) | ||
if !ok { | ||
return nil, fmt.Errorf("cannot use OptimisticLock, from object %q is not a valid metav1.Object", s.from) | ||
} | ||
resourceVersion := fromMeta.GetResourceVersion() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does patch with a RV precondition differ from just doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good question, I think when it comes down to patch is easier to merge only the fields a client has updated. It does keep the scope down to only things that have changes, so it's still possible to retry to patch only things necessary. This particular use case came up from Cluster API conditions work, where we need to make sure that when patching an object we aren't overwriting other controllers. Adding resource version on patch keeps the scope down to the patch and we can still try to do a best-effort merge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But with this precondition, if anything on the object was changed its RV gets increased and this patch fails. Hence, it has the exact same semantics as It seems to me a bit that what you actually want is a precondition on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's the same update semantic, although in our case it'll allow us to only focus on a single field and try merging that field without bothering about the rest |
||
if len(resourceVersion) == 0 { | ||
return nil, fmt.Errorf("cannot use OptimisticLock, from object %q does not have any resource version we can use", s.from) | ||
} | ||
u := &unstructured.Unstructured{Object: dataMap} | ||
u.SetResourceVersion(resourceVersion) | ||
data, err = json.Marshal(u) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
return data, nil | ||
} | ||
|
||
// MergeFrom creates a Patch that patches using the merge-patch strategy with the given object as base. | ||
func MergeFrom(obj runtime.Object) Patch { | ||
return &mergeFromPatch{obj} | ||
return &mergeFromPatch{from: obj} | ||
} | ||
|
||
// MergeFromWithOptions creates a Patch that patches using the merge-patch strategy with the given object as base. | ||
func MergeFromWithOptions(obj runtime.Object, opts ...MergeFromOption) Patch { | ||
options := &MergeFromOptions{} | ||
for _, opt := range opts { | ||
opt.ApplyToMergeFrom(options) | ||
} | ||
return &mergeFromPatch{from: obj, opts: *options} | ||
} | ||
|
||
// mergePatch uses a raw merge strategy to patch the object. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe out of the scope of this PR but why are all these tests async? I thought this was only useful if you were testing asynchronous code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That they accept the
done Done
parameter, it's an optional feature of GinkgoIt
: https://onsi.github.io/ginkgo/#asynchronous-testsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure, I guess they can all run in parallel