Implement Strategic Merge Patch in apiserver #6027

ghodss · 2015-03-26T23:39:01Z

This is an implementation of JSON merge patch custom to Kubernetes, called Kubernetes Merge Patch (update: this is now called Strategic Merge Patch). For a description of the design see #4889 (comment) and for usage see docs/api-conventions.md in this PR.

Currently the patch strategy metadata is listed as struct tags, but we can make the information accessible through swagger (and therefore accessible to clients) at a later point.

@smarterclayton, can you look at api_installer and resthandler? The issue is that I need the versioned object in the PatchResource method to lookup the patchStrategy struct tags figure out what lists of the versioned object should be merged, and I kind of hacked it together. Let me know if there's a better way.

@bgrant0607 @mikedanese

mikedanese · 2015-03-27T00:40:08Z

pkg/client/request.go

@@ -79,6 +79,14 @@ func (r *RequestConstructionError) Error() string {
 	return fmt.Sprintf("request construction error: '%v'", r.Err)
 }

+type PatchType string


This should go in pkg/apiserver/patch/patch.go?

This enum is created and never used (except for once). I'd prefer if it was used in place of all the copy pasted string constants.

I don't think so... pkg/apiserver/patch is only one implementation. The header supports all three implementations.

The bigger question is whether it goes in the server or the client. For a proper API it probably needs to be duplicated in both.

It's lame, but we have a number of such constants in pkg/api/types.go.

I went with the pkg/api/types.go approach. @mikedanese would appreciate your eyes on the way I factored it out.

mikedanese · 2015-03-27T00:48:38Z

Nice! Looks good. I'm going to try to step through the kube-merge-patch logic in the next couple days.

smarterclayton · 2015-03-27T19:03:10Z

pkg/apiserver/patch/patch.go

@@ -0,0 +1,454 @@
+/*


Any reason this couldn't go into pkg/util/patch/*? Feels more generic than apiserver.

Good point. I'm thinking I should rename it from Kube Merge Patch to something more generic as well, since the way it's implemented and its interface doesn't actually have anything to do with Kubernetes. Maybe Strategic Patch or something like that.

I'm not keen on contributing to code sprawl. We currently have code sprinkled all over a very flat directory layout -- #4851, and putting the code into util would make it harder to extract apiserver later #2742. I suggest leaving the code under apiserver until we figure out how we should actually be structuring our codebase.

Clayton's point that this is generic is correct - this is a library that could (and maybe should) live outside of kube. As a result, unless we're going to discourage apiserver from using things outside of itself, I think util is the right place. If it had any dependency or coupling at all to apiserver I'd definitely say it should stay in the apiserver package.

smarterclayton · 2015-04-01T20:13:00Z

The apiserver installer portions look fine to me. I haven't had a chance to look more deeply at merge and probably won't in the next few days.

bgrant0607 · 2015-04-01T20:23:07Z

I will try to at least look through the merge semantics today.

bgrant0607 · 2015-04-01T22:03:48Z

docs/api-conventions.md

+To indicate that a map should not be merged and instead should be taken literally:
+
+```yaml
+__patch: strict  # recursive and applies to all fields of the map it's in


The options for __patch are merge, strict, and delete? Would delete have any meaning at the top level? Can strict be overridden by merge specified in a sublist? Does an explicit __patch annotation override all struct field tags recursively?

Would delete have any meaning at the top level?

Delete is not needed in a map: you just set the key to null.

Can strict be overridden by merge specified in a sublist?

Not at the moment, addressed in other comment.

Does an explicit __patch annotation override all struct field tags recursively?

"strict" does.

bgrant0607 · 2015-04-01T23:43:15Z

Very cool! Looks pretty close for a first cut. I'll try to look at the merge test in more detail, but I've made a pass over the implementation.

bgrant0607 · 2015-04-02T00:28:12Z

pkg/util/strategicpatch/patch_test.go

+    original:
+      simpleMap:
+        name: 1
+        value: 1


key1, key2 would be less confusing than name, value.

bgrant0607 · 2015-04-02T00:41:57Z

I'm pretty happy with the merge test. The main thing it omits is error cases (merging lists of lists, invalid patch types, merge strategy w/o key tags, unspecified merge keys in the data, etc.), though it maybe could use a few more __patch: strict cases with nested structures.

bgrant0607 · 2015-04-02T16:47:44Z

cc @lavalamp

ghodss · 2015-04-03T03:54:51Z

All comments responded to - ready for another pass.

bgrant0607 · 2015-04-03T16:57:14Z

pkg/api/v1beta3/types.go

@@ -718,7 +718,7 @@ type PodSpec struct {
 // state of a system.
 type PodStatus struct {
 	Phase      PodPhase       `json:"phase,omitempty" description:"current condition of the pod."`
-	Conditions []PodCondition `json:"Condition,omitempty" description:"current service state of pod"`
+	Conditions []PodCondition `json:"Condition,omitempty" description:"current service state of pod" patchStrategy:"merge" patchMergeKey:"type"`


Please tag NodeCondition and NodeAddress as well, for consistency.

bgrant0607 · 2015-04-03T16:59:50Z

Thanks for the changes. Other than the NodeStatus flags, LGTM. I'm not sure what to do about the Go library code, but apparently this isn't the only instance of that.

@lavalamp Do you want to take a look?

bgrant0607 · 2015-04-03T22:08:46Z

Test failures are known issues unrelated to this PR. Merging. We can improve more if necessary in subsequent PRs.

Implement Strategic Merge Patch in apiserver

bgrant0607 · 2015-04-03T22:59:42Z

@ghodss I'm seeing some intermittent integration test failures. Could you take a look?

For example:
https://app.shippable.com/builds/551f18ea23bf360c00589947

F0403 22:53:41.895299   31097 integration.go:655] Failed updating patchservice with patch type application/json-patch+json: services "patchservice" not found
!!! Error in ./hack/test-integration.sh:47
  '"${KUBE_OUTPUT_HOSTBIN}/integration" --v=2 --apiVersion="$1"' exited with status 255

bgrant0607 · 2015-04-03T23:10:50Z

I'm guessing the issue may be that the patches don't work with v1beta3. labels have moved to metadata.labels.

bgrant0607 · 2015-04-03T23:13:35Z

cc @nikhiljindal

bgrant0607 · 2015-04-03T23:14:56Z

I0403 22:53:41.894652   31097 integration.go:548] Created atomicService
I0403 22:53:41.894702   31097 integration.go:561] Starting to update (a, z)
I0403 22:53:41.894736   31097 integration.go:561] Starting to update (b, y)
I0403 22:53:41.894770   31097 integration.go:561] Starting to update (c, x)
I0403 22:53:41.894778   31097 integration.go:561] Starting to update (d, w)
I0403 22:53:41.894784   31097 integration.go:561] Starting to update (e, v)
I0403 22:53:41.894789   31097 integration.go:561] Starting to update (foo, bar)
F0403 22:53:41.895299   31097 integration.go:655] Failed updating patchservice with patch type application/json-patch+json: services "patchservice" not found
!!! Error in ./hack/test-integration.sh:47
  '"${KUBE_OUTPUT_HOSTBIN}/integration" --v=2 --apiVersion="$1"' exited with status 255
Call stack:
  1: ./hack/test-integration.sh:47 runTests(...)
  2: ./hack/test-integration.sh:60 main(...)

bgrant0607 · 2015-04-03T23:43:05Z

For others: workaround in #6431

googlebot added the cla: yes label Mar 26, 2015

mikedanese reviewed Mar 27, 2015
View reviewed changes

zmerlynn assigned smarterclayton Mar 27, 2015

smarterclayton reviewed Mar 27, 2015
View reviewed changes

ghodss changed the title ~~Implement kube-merge-patch in apiserver~~ Implement Strategic Merge Patch in apiserver Mar 29, 2015

bgrant0607 mentioned this pull request Apr 1, 2015

Find portalIP automatically while updating a service #5907

Closed

bgrant0607 reviewed Apr 1, 2015
View reviewed changes

bgrant0607 reviewed Apr 2, 2015
View reviewed changes

bgrant0607 mentioned this pull request Apr 3, 2015

Add PodSpec.NodeFailurePolicy = {Reschedule, Delete, Ignore} #6393

Closed

bgrant0607 mentioned this pull request Apr 3, 2015

Bastion take2 csrwng/kubernetes#17

Closed

bgrant0607 reviewed Apr 3, 2015
View reviewed changes

Implement Strategic Merge Patch in apiserver

2c977db

bgrant0607 added a commit that referenced this pull request Apr 3, 2015

Merge pull request #6027 from ghodss/kube-patch

0d95883

Implement Strategic Merge Patch in apiserver

bgrant0607 merged commit 0d95883 into kubernetes:master Apr 3, 2015

ghodss mentioned this pull request Apr 5, 2015

API should differentiate between lists to merge and lists to replace in reconciliation #4889

Closed

ghodss deleted the kube-patch branch April 24, 2015 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Strategic Merge Patch in apiserver #6027

Implement Strategic Merge Patch in apiserver #6027

ghodss commented Mar 26, 2015

mikedanese Mar 27, 2015

mikedanese Mar 27, 2015

ghodss Mar 27, 2015

bgrant0607 Apr 1, 2015

ghodss Apr 3, 2015

mikedanese commented Mar 27, 2015

smarterclayton Mar 27, 2015

ghodss Mar 27, 2015

bgrant0607 Apr 1, 2015

ghodss Apr 3, 2015

smarterclayton commented Apr 1, 2015

bgrant0607 commented Apr 1, 2015

bgrant0607 Apr 1, 2015

ghodss Apr 3, 2015

bgrant0607 commented Apr 1, 2015

bgrant0607 Apr 2, 2015

ghodss Apr 3, 2015

bgrant0607 commented Apr 2, 2015

bgrant0607 commented Apr 2, 2015

ghodss commented Apr 3, 2015

bgrant0607 Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

Implement Strategic Merge Patch in apiserver #6027

Implement Strategic Merge Patch in apiserver #6027

Conversation

ghodss commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Mar 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Apr 1, 2015

bgrant0607 commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Apr 2, 2015

bgrant0607 commented Apr 2, 2015

ghodss commented Apr 3, 2015

Choose a reason for hiding this comment

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015

bgrant0607 commented Apr 3, 2015