Let crd support strategic merge patch on metadata #56606

caesarxuchao · 2017-11-30T04:49:52Z

cc @liggitt @nikhita @enisoc @mengqiy @ironcladlou

The size of this fix is +13/1- (e6d7af1). Comparing it with converting the garbage collector to user JSON merge patch(#56595 +125/−20), I prefer this fix. I'll add unit tests if we can reach consensus.

The major criticism on this approach was that we shouldn't support strategic merge patch for CR partially. I'd like to defend this approach once more:

From the perspective of the evolution of CRD, supporting strategic merge patch is a goal. And no matter how we achieve that goal, the schema of ObjectMeta should be defined by the APIServer, like what's done in this PR. So I regard this PR as a small step towards a larger goal, rather than a one-time hack.
From the perspective of API consistency , ObjectMeta should be homogeneous across all resources stored on apiservers, including the supported operations on ObjectMeta.
From users' perspective, they will receive a clear error message if they send a strategic merge patch for fields other than the ObjectMeta.

Another point raised by @ironcladlou was that we should "promote the use of one of the IETF PATCH standards" (#56348 (comment)). My perspective is that the strategic merge patch is the only patch type that can safely patch list in a singe roundtrip, it should be supported by all resources. Currently only CRD doesn't support it.

Custom resources now accept strategic merge patch against its `metadata` field. The patch will be rejected if it contains fields other than `metadata`.

liggitt · 2017-11-30T04:55:42Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/helpers.go

@@ -439,6 +439,9 @@ func (UnstructuredObjectConverter) Convert(in, out, context interface{}) error {
 func (UnstructuredObjectConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
 	if kind := in.GetObjectKind().GroupVersionKind(); !kind.Empty() {
 		gvk, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{kind})
+		if gvk.GroupVersion().Version == runtime.APIVersionInternal {


cc @smarterclayton

liggitt · 2017-11-30T04:58:54Z

cc @kubernetes/sig-api-machinery-pr-reviews

sttts · 2017-11-30T10:09:43Z

hack/local-up-cluster.sh

@@ -220,7 +220,7 @@ KUBELET_HOST=${KUBELET_HOST:-"127.0.0.1"}
 # By default only allow CORS for requests on localhost
 API_CORS_ALLOWED_ORIGINS=${API_CORS_ALLOWED_ORIGINS:-/127.0.0.1(:[0-9]+)?$,/localhost(:[0-9]+)?$}
 KUBELET_PORT=${KUBELET_PORT:-10250}
-LOG_LEVEL=${LOG_LEVEL:-3}
+LOG_LEVEL=${LOG_LEVEL:-5}


debug change

sttts · 2017-11-30T10:12:49Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go

@@ -866,7 +871,11 @@ func StrategicMergeMapPatch(original, patch JSONMap, dataStruct interface{}) (JS
 	// each field in a strategic merge patch, we can't find the go struct tags. Hence, we can't easily  do a strategic merge
 	// for custom resources. So we should fail fast and return an error.
 	if _, ok := dataStruct.(*unstructured.Unstructured); ok {
-		return nil, mergepatch.ErrUnsupportedStrategicMergePatchFormat
+		schema, err = NewPatchMetaFromStruct(MetaOnly{})


What does this mean for patches again fields outside of meta? Will they apply like to a native type without any tags?

If yes, I would like to see a test case. If not, it must be rejected.

only supporting a patch type on part of an object is bizarre to me

liggitt · 2017-11-30T14:24:09Z

My perspective is that the strategic merge patch is the only patch type that can safely patch list in a singe roundtrip, it should be supported by all resources. Currently only CRD doesn't support it.

Is our position that all aggregated API servers must support strategic merge patch? I don't see a compelling reason to require that to participate in GC. What was the measured impact of simply doing a JSON merge patch on the expected field with the resourceVersion?

From the perspective of the evolution of CRD, supporting strategic merge patch is a goal.

I'm not sure I agree. Experience with strategic merge patch from apply has not been good. Why do we want to expand it?

caesarxuchao · 2017-11-30T18:35:32Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go

@@ -866,7 +871,14 @@ func StrategicMergeMapPatch(original, patch JSONMap, dataStruct interface{}) (JS
 	// each field in a strategic merge patch, we can't find the go struct tags. Hence, we can't easily  do a strategic merge
 	// for custom resources. So we should fail fast and return an error.
 	if _, ok := dataStruct.(*unstructured.Unstructured); ok {
-		return nil, mergepatch.ErrUnsupportedStrategicMergePatchFormat
+		if _, ok := patch["metadata"]; !ok || len(patch) != 1 {


@sttts I forgot to push this check. This will prevent strategic merge patch outside of "metadata".

is an empty patch ever allowed?

@jennybuckley An empty strategic merge patch on custom resources is not allowed (It wasn't allowed before as well).

lavalamp · 2017-11-30T21:16:55Z

cc @lavalamp

lavalamp · 2017-11-30T21:17:04Z

cc @jennybuckley

lavalamp · 2017-11-30T22:12:43Z

Is our position that all aggregated API servers must support strategic merge patch?

Not thus far.

I don't see a compelling reason to require that to participate in GC.

I tend to agree, it seems good to keep the bar low for participating.

enisoc · 2017-11-30T22:13:34Z

@sttts wrote:

only supporting a patch type on part of an object is bizarre to me

In my opinion it's weird that we don't already support strategic merge patch on metadata inside CRs. We have all the tags we need via ObjectMeta, so why forbid it? As evidenced by this use case for GC, it's a useful thing to be able to do; I would say we just didn't get around to implementing it yet. Apparently the authors and reviewers of GC support for CRs also made the (IMO) reasonable assumption that patching metadata should work the same for any object that has ObjectMeta (which we enforce for everything made by CRD).

@liggitt wrote:

Is our position that all aggregated API servers must support strategic merge patch?

We aren't necessarily taking that position by doing this. We're already in the state where GC doesn't work for anything that doesn't support SMP. Our goal right now is fixing GC specifically for CRs hosted through CRD. Broadening GC support to other things that don't support SMP is beyond the scope of this bugfix.

On a side note, I would expect SMP to come "free" for any aggregated API server that uses our libraries and uses compiled-in Go types. Is that not the case?

@liggitt wrote:

What was the measured impact of simply doing a JSON merge patch on the expected field with the resourceVersion?

We haven't measured it yet because Chao's PR (#56595) shows there is already significant impact on the core architecture of the GC. Currently everything runs off of a dependency graph that only changes when object relationships change (not when arbitrary fields in those objects change). In order to use anything other than SMP, GC would need to know the latest state of the object fields so it can avoid clobbering other changes, which is a significant change in the algorithm that we would prefer not to cherry-pick to 1.8.x and 1.9.1.

@liggitt wrote:

Experience with strategic merge patch from apply has not been good. Why do we want to expand it?

Strategic merge patch/diff is currently our best known solution for the problem of reconciling mutations made by automation (e.g. controllers, admission) with changes to the original source of truth (e.g. YAML in source control). Kubectl apply was an early proving ground for this and we expect to use SMP more in things like Declarative App Management. So for example, some parts of DAM might require strategic merge diff, which will require non-compiled-in CRs (such as those from CRD) to provide the SMP tags in some form (e.g. OpenAPI).

lavalamp · 2017-11-30T23:52:24Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/helpers.go

@@ -439,6 +439,9 @@ func (UnstructuredObjectConverter) Convert(in, out, context interface{}) error {
 func (UnstructuredObjectConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
 	if kind := in.GetObjectKind().GroupVersionKind(); !kind.Empty() {
 		gvk, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{kind})
+		if gvk.GroupVersion().Version == runtime.APIVersionInternal {
+			return in, nil


What is the purpose of this?

This should be impossible - an internal unstructed is a coding error, and should never be allowed

lavalamp · 2017-12-01T00:06:36Z

Just to clarify, Chao's claim is that CRD is the only apiserver that would ever need this fix, assuming others use the generic apiserver library. But this is equivalent to mandating all apiservers support SMP on metadata. (Otherwise we have to fix GC too, which would render this change redundant.)

I agree it's weird to generally allow patching arbitrary subsets of objects, but allowing patching of metadata even if you don't understand the rest of the object seems very powerful. So I don't object to that.

So I really think it comes down to whether we want to mandate support for (at least metadata) SMP.

Currently I am inclined to say:

Let's take this patch.
Let's not set a precedent, and be 100% willing to reevaluate this if someone comes up with a valid use case.

smarterclayton · 2017-12-01T02:01:29Z

Given that strategic merge patch is unversioned this means we can never change the merge patch strategy on metadata ever again. I’m not confident enough in SMR to say that is accurate.

I don’t think we should require all api servers support strategic merge patch without a lot more discussion - that statement above chills me to the bone...

smarterclayton · 2017-12-01T02:05:45Z

Also, what happens to an existing valid api server extension today that doesn’t support this?

caesarxuchao · 2017-12-01T04:22:28Z

Also, what happens to an existing valid api server extension today that doesn’t support this?

Do we have apiservers that are not based on the generic apiserver library? If they don't support SMP, they can still use background GC. Foreground and orphan will stuck.

caesarxuchao · 2017-12-01T22:02:54Z

Can we agree on accepting this patch for 1.9 (obviously I'll need to add more tests)? It's making CRD more powerful, and fixes 99% GC use cases.

I can continue working on my other fix (#56595) that converts GC to use JSON merge patch, but that one requires more soak time.

lavalamp · 2017-12-04T19:29:12Z

Given that strategic merge patch is unversioned this means we can never change the merge patch

Ugh, this is seriously still true? We need to replace it with something more principled ASAP.

lavalamp · 2017-12-04T19:32:46Z

hack/local-up-cluster.sh

@@ -115,16 +110,6 @@ if [ "${CLOUD_PROVIDER}" == "openstack" ]; then
    fi
 fi

-#set feature gates if using ipvs mode


Why are there so many changes to this file? It seems unrelated?

Must be a rebase error. I'll finalize this PR.

caesarxuchao · 2017-12-06T00:07:57Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/helpers.go

@@ -439,6 +439,9 @@ func (UnstructuredObjectConverter) Convert(in, out, context interface{}) error {
 func (UnstructuredObjectConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
 	if kind := in.GetObjectKind().GroupVersionKind(); !kind.Empty() {
 		gvk, ok := target.KindForGroupVersionKinds([]schema.GroupVersionKind{kind})
+		// if gvk.GroupVersion().Version == runtime.APIVersionInternal {
+		// 	return in, nil


@smarterclayton I wanted to treat the original version as the internal version of a custom resource. Otherwise we need special handling for Unstructured at https://github.com/kubernetes/kubernetes/pull/56606/files#diff-15bdfc59ec78ff09b509d4a347560a1bR244 and https://github.com/kubernetes/kubernetes/pull/56606/files#diff-15bdfc59ec78ff09b509d4a347560a1bR346.

Looks much better without these lines. But we should delete instead of comment out :)

caesarxuchao · 2017-12-06T00:10:36Z

@lavalamp @liggitt it's ready for review. PTAL. Thank you.

caesarxuchao · 2017-12-06T00:29:36Z

/retest

k8s-ci-robot · 2017-12-06T01:07:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue: #56348

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [caesarxuchao]
staging/src/k8s.io/apiserver/OWNERS
test/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

caesarxuchao · 2017-12-06T01:09:47Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go

@@ -6746,3 +6747,100 @@ func TestUnknownField(t *testing.T) {
 		}
 	}
 }
+
+func TestStrategicMergePatchUnstructured(t *testing.T) {


When writing this test, I didn't notice we already had a unit test at staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go. Anyway, it doesn't hurt to have extra tests at different layers.

lavalamp · 2017-12-12T21:11:43Z

@liggitt So did I convince you we should take this at kubecon? :)

liggitt · 2017-12-12T21:27:52Z

@liggitt: Is our position that all aggregated API servers must support strategic merge patch?

@enisoc: We aren't necessarily taking that position by doing this.

We are by refusing to make GC work with a resource unless it supports SMP

@lavalamp: So did I convince you we should take this at kubecon? :)

I still think the semantics of only supporting a patch type on part of an object are bizarre. Would openapi claim to accept strategic merge patch content-type for CRDs? That would lead clients like kubectl apply to compute SMP patches, rather than jsonmerge patches, and fail.

Also, the typecasts in patch handling smell particularly bad to me.

I'd still be more inclined to switch GC to a patch type that is more reasonable to support, rather than do this.

sttts · 2017-12-13T08:58:43Z

I'd still be more inclined to switch GC to a patch type that is more reasonable to support, rather than do this.

I agree with @liggitt. I would rather see a fixed GC in 1.9.x with non-strategic patches than taking this shortcut.

lavalamp · 2017-12-14T20:31:34Z

@liggitt: Is our position that all aggregated API servers must support strategic merge patch?

@enisoc: We aren't necessarily taking that position by doing this.

@liggitt: We are by refusing to make GC work with a resource unless it supports SMP

I disagree. I don't think we are "refusing" permanently. This is just an expedient way to solve the only existing problem case.

k8s-github-robot · 2018-02-06T08:26:53Z

@caesarxuchao PR needs rebase

liggitt · 2018-05-03T14:21:59Z

closing based on discussion in #58414 (comment) and removal of the need for SMP support for patch retry in #63146

/close

Automatic merge from submit-queue (batch tested with PRs 63386, 64624, 62297, 64847). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Let the garbage collector use json merge patch when SMP is not supported **What this PR does / why we need it**: Let garbage collector fallback to use json merge patch when strategic merge patch returns 415. This enables orphan delete on custom resources. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #56348 **Special notes for your reviewer**: This PR is developed based on #56595. Ref #56606 for more information. **Release note**: ```release-note Orphan delete is now supported for custom resources ``` /sig api-machinery

caesarxuchao self-assigned this Nov 30, 2017

caesarxuchao changed the title ~~Let crd support strategic merge patch on metadata~~ [WIP] Let crd support strategic merge patch on metadata Nov 30, 2017

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2017

liggitt reviewed Nov 30, 2017

View reviewed changes

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 30, 2017

sttts assigned nikhita and unassigned nikhita Nov 30, 2017

sttts reviewed Nov 30, 2017

View reviewed changes

caesarxuchao force-pushed the crd-strategic-metadata branch from e6d7af1 to e91d23c Compare November 30, 2017 18:21

caesarxuchao commented Nov 30, 2017

View reviewed changes

mengqiy mentioned this pull request Nov 30, 2017

Kubectl apply and strategic merge patch using openapi #51321

Merged

4 tasks

caesarxuchao force-pushed the crd-strategic-metadata branch from e91d23c to f9363e9 Compare November 30, 2017 19:28

lavalamp reviewed Nov 30, 2017

View reviewed changes

lavalamp reviewed Dec 4, 2017

View reviewed changes

caesarxuchao force-pushed the crd-strategic-metadata branch 2 times, most recently from b921a13 to d836a10 Compare December 6, 2017 00:01

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2017

caesarxuchao changed the title ~~[WIP] Let crd support strategic merge patch on metadata~~ Let crd support strategic merge patch on metadata Dec 6, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2017

caesarxuchao commented Dec 6, 2017

View reviewed changes

caesarxuchao assigned lavalamp and liggitt and unassigned caesarxuchao Dec 6, 2017

caesarxuchao force-pushed the crd-strategic-metadata branch from d836a10 to f71c99a Compare December 6, 2017 01:07

caesarxuchao commented Dec 6, 2017

View reviewed changes

let crd support strategic merge patch on metadata

5f8352c

caesarxuchao force-pushed the crd-strategic-metadata branch from f71c99a to 5f8352c Compare December 6, 2017 01:10

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 6, 2017

jennybuckley mentioned this pull request Jan 16, 2018

Don't compute SMP on conflicts if it is not supported #58064

Closed

nikhita mentioned this pull request Jan 17, 2018

Support strategic merge patch for custom resources #58414

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018

roycaihw mentioned this pull request May 3, 2018

Let the garbage collector use json merge patch when SMP is not supported #63386

Merged

k8s-ci-robot closed this May 3, 2018

Let crd support strategic merge patch on metadata #56606

Let crd support strategic merge patch on metadata #56606

Conversation

caesarxuchao commented Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt commented Nov 30, 2017

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

liggitt commented Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Nov 30, 2017

lavalamp commented Nov 30, 2017

lavalamp commented Nov 30, 2017

enisoc commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Dec 1, 2017

smarterclayton commented Dec 1, 2017

smarterclayton commented Dec 1, 2017

caesarxuchao commented Dec 1, 2017

caesarxuchao commented Dec 1, 2017 • edited Loading

lavalamp commented Dec 4, 2017

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

caesarxuchao commented Dec 6, 2017

caesarxuchao commented Dec 6, 2017

k8s-ci-robot commented Dec 6, 2017

Choose a reason for hiding this comment

lavalamp commented Dec 12, 2017

liggitt commented Dec 12, 2017

sttts commented Dec 13, 2017

lavalamp commented Dec 14, 2017

k8s-github-robot commented Feb 6, 2018

liggitt commented May 3, 2018

caesarxuchao commented Nov 30, 2017 •

edited

Loading

liggitt commented Nov 30, 2017 •

edited

Loading

caesarxuchao commented Dec 1, 2017 •

edited

Loading