Skip to content

✨ 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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 64 additions & 2 deletions pkg/client/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1210,6 +1210,42 @@ var _ = Describe("Client", func() {
close(done)
})

It("should patch an existing object from a go struct, using optimistic locking", func(done Done) {
Copy link

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

Copy link
Member Author

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?

Copy link

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 Ginkgo It: https://onsi.github.io/ginkgo/#asynchronous-tests

Copy link
Member Author

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

cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
Expect(cl).NotTo(BeNil())

By("initially creating a Deployment")
dep, err := clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{})
Expect(err).NotTo(HaveOccurred())

By("creating a patch from with optimistic lock")
patch := client.MergeFromWithOptions(dep.DeepCopy(), client.MergeFromWithOptimisticLock{})

By("adding a new annotation")
dep.Annotations = map[string]string{
"foo": "bar",
}

By("patching the Deployment")
err = cl.Patch(context.TODO(), dep, patch)
Expect(err).NotTo(HaveOccurred())

By("validating patched Deployment has new annotation")
actual, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expect(actual).NotTo(BeNil())
Expect(actual.Annotations["foo"]).To(Equal("bar"))

By("validating that a patch should fail with conflict, when it has an outdated resource version")
dep.Annotations["should"] = "conflict"
err = cl.Patch(context.TODO(), dep, patch)
Expect(err).To(HaveOccurred())
Expect(apierrors.IsConflict(err)).To(BeTrue())

close(done)
})

It("should patch and preserve type information", func(done Done) {
cl, err := client.New(cfg, client.Options{})
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -2656,8 +2692,9 @@ var _ = Describe("Patch", func() {
BeforeEach(func() {
cm = &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Namespace: metav1.NamespaceDefault,
Name: "cm",
Namespace: metav1.NamespaceDefault,
Name: "cm",
ResourceVersion: "10",
},
}
})
Expand Down Expand Up @@ -2686,6 +2723,31 @@ var _ = Describe("Patch", func() {
By("returning a patch with data only containing the annotation change")
Expect(data).To(Equal([]byte(fmt.Sprintf(`{"metadata":{"annotations":{"%s":"%s"}}}`, annotationKey, annotationValue))))
})

It("creates a merge patch with the modifications applied during the mutation, using optimistic locking", func() {
const (
annotationKey = "test"
annotationValue = "foo"
)

By("creating a merge patch")
patch := client.MergeFromWithOptions(cm.DeepCopy(), client.MergeFromWithOptimisticLock{})

By("returning a patch with type MergePatch")
Expect(patch.Type()).To(Equal(types.MergePatchType))

By("retrieving modifying the config map")
metav1.SetMetaDataAnnotation(&cm.ObjectMeta, annotationKey, annotationValue)

By("computing the patch data")
data, err := patch.Data(cm)

By("returning no error")
Expect(err).NotTo(HaveOccurred())

By("returning a patch with data containing the annotation change and the resourceVersion change")
Expect(data).To(Equal([]byte(fmt.Sprintf(`{"metadata":{"annotations":{"%s":"%s"},"resourceVersion":"%s"}}`, annotationKey, annotationValue, cm.ResourceVersion))))
})
})
})

Expand Down
74 changes: 72 additions & 2 deletions pkg/client/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}
Copy link
Member

@alvaroaleman alvaroaleman May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to read the code to understand what you meant by OptimisticLock. This definitely needs godocs.

I also don't understand how that differs from Update except that you put less data on the wire and trade it for a local computation of the patch which seems like an implementation detail to me that should rarely if ever be relevant.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Expand All @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does patch with a RV precondition differ from just doing Update?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

@alvaroaleman alvaroaleman May 28, 2020

Choose a reason for hiding this comment

The 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 UPDATE unless I am missing something.

It seems to me a bit that what you actually want is a precondition on the .status.conditions and not the RV?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Expand Down