support replaceKeys patch strategy #44597

mengqiy · 2017-04-18T05:00:38Z

Implementing according to this proposal.
The revision is in kubernetes/community#620.

support replaceKeys patch strategy and directive for strategic merge patch

mengqiy · 2017-04-18T05:00:43Z

/assign @pwittrock

k8s-reviewable · 2017-04-18T05:00:44Z

This change is

mengqiy · 2017-04-18T17:19:17Z

@k8s-bot bazel test this

pwittrock · 2017-04-19T18:07:26Z

@janetkuo Would you be able to review this PR?

janetkuo · 2017-04-19T21:47:56Z

@pwittrock yes

mengqiy · 2017-04-24T21:00:30Z

@janetkuo ping

janetkuo · 2017-04-25T00:55:08Z

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

@@ -58,13 +60,22 @@ type DiffOptions struct {
 	IgnoreChangesAndAdditions bool
 	// IgnoreDeletions indicates if we keep the deletions in the patch.
 	IgnoreDeletions bool
+	// ReplaceKeys will keep all the keys in the map or in the slice of maps.


Would you update the description to make it clear that "only" the keys present in the map will be kept? Something like what's described in the proposal:

We introduce a new value replaceKeys for patchStrategy. It indicates that all fields needing to be preserved must be present in the patch. And the fields that are present will be merged with live object. All the missing fields will be cleared when patching.

janetkuo · 2017-04-25T00:55:44Z

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

+	SimpleMap              map[string]string
+	ReplacingItem          runtime.RawExtension `patchStrategy:"replace"`
+	ReplaceKeysMap         ReplaceKeysMergeItem `patchStrategy:"replaceKeys"`
+	ReplaceKeysMergingList []MergeItem          `patchStrategy:"merge|replaceKeys" patchMergeKey:"name"`


Note: use merge,replaceKey as we just discussed

janetkuo · 2017-04-25T00:57:03Z

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

@@ -43,6 +43,8 @@ const (
 	replaceDirective = "replace"
 	mergeDirective   = "merge"

+	replaceKeysDirective = "$replaceKeys"


We're not using $patch: replaceKeys anymore?

Changed back to $patch: replaceKeys.

mengqiy · 2017-04-28T20:58:23Z

@k8s-bot unit test this

mengqiy · 2017-05-01T06:00:33Z

After discussing with @pwittrock offline, I have updated the PR a little to make sure the new patch is always a superset of the old patch.

mengqiy · 2017-05-02T05:16:09Z

@k8s-bot unit test this

janetkuo · 2017-05-02T23:08:05Z

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

-	// MergeDeleteList indicates if we are merging the delete parallel list.
-	MergeDeleteList bool
+	// ProcessDirectives indicates if we are merging the delete parallel list.
+	ProcessDirectives bool


Why is it renamed? Should the comment be updated as well?

I can imagine that this will also control the setOrder parallel list described in kubernetes/community#537. So I change the name to make it more general.
Will update the comment.

janetkuo · 2017-05-02T23:28:26Z

staging/src/k8s.io/apimachinery/third_party/forked/golang/json/fields.go

+}
+
+// extractReplaceKeysPatchStrategy process patch strategy, which is a string may contains multiple
+// patch strategies seperated by "|". It returns a boolean var indicating if it has replaceKeys strategies


separated by ","

janetkuo · 2017-05-02T23:31:25Z

staging/src/k8s.io/apimachinery/third_party/forked/golang/json/fields.go

+// patch strategies seperated by "|". It returns a boolean var indicating if it has replaceKeys strategies
+// and a string or other strategy.
+func extractReplaceKeysPatchStrategy(patchStrategy string) (bool, string, error) {
+	replaceKeysPatchStrategy := "replaceKeys"


optional but maybe we could reuse strategicpatch.ReplaceKeysDirective

or just declare as const (so as "patchStrategy" and "patchMergeKey")

I hesitated to use strategicpatch.ReplaceKeysDirective, because I'm worrying about introducing dependency of k8s.io/apimachinery/third_party/forked/golang/json/fields.go to k8s.io/apimachinery/util/strategicpatch. But it seems to be OK.

janetkuo · 2017-05-02T23:40:05Z

staging/src/k8s.io/apimachinery/third_party/forked/golang/json/fields.go

@@ -20,12 +20,12 @@ import (
 // Finds the patchStrategy and patchMergeKey struct tag fields on a given
 // struct field given the struct type and the JSON name of the field.
 // TODO: fix the returned errors to be introspectable.
-func LookupPatchMetadata(t reflect.Type, jsonField string) (reflect.Type, string, string, error) {
+func LookupPatchMetadata(t reflect.Type, jsonField string) (reflect.Type, bool, string, string, error) {


update comment for returned value

janetkuo · 2017-05-02T23:50:38Z

staging/src/k8s.io/apimachinery/third_party/forked/golang/json/fields.go

+	case strategies[1] == replaceKeysPatchStrategy:
+		return true, strategies[0], nil
+	default:
+		return false, "", fmt.Errorf("Unexpected patch strategy: %q\n", patchStrategy)


So we allow at most 2 patch strategies, and if there are 2 strategies, one of them needs to be "replaceKeys". Correct?

What about replaceKeys,replaceKeys?

So we allow at most 2 patch strategies, and if there are 2 strategies, one of them needs to be "replaceKeys". Correct?

In current scope, yes. You can find "replaceKeys,merge" in the unit test.
WDYT if I make it more general by returning a string list for strategy?

func LookupPatchMetadata(t reflect.Type, jsonField string) (reflect.Type, []string, string, error)

janetkuo · 2017-05-03T00:04:14Z

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

@@ -110,8 +123,10 @@ func CreateTwoWayMergeMapPatch(original, modified JSONMap, dataStruct interface{
 	diffOptions := DiffOptions{
 		IgnoreChangesAndAdditions: false,
 		IgnoreDeletions:           false,
+		ReplaceKeys:               false,
+		IgnoreReplaceKeys:         false,


These are all zero values. diffOptions := DiffOptions{} is enough

janetkuo · 2017-05-03T00:49:43Z

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

 	}
 	// Create a patch to replace the old value with the new one
 	patch[key] = modified
+	return true
 }

 // updatePatchIfMissing iterates over `original` when ignoreDeletions is false.


mention ReplaceKey too

janetkuo · 2017-05-03T00:58:34Z

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

-	}
-	// Add nils for deleted values
+// It returns a boolean var indicating if there are any changes.
+func updatePatchIfMissing(original, modified, patch map[string]interface{}, diffOptions DiffOptions) bool {


IIUC, basically when IgnoreDeletions==true we do nothing, and when IgnoreDeletions==false we add nils in the for loop, and ReplaceKeys has no nothing to do with it?

IIUC, basically when IgnoreDeletions==true we do nothing, and when IgnoreDeletions==false we add nils in the for loop, and ReplaceKeys has no nothing to do with it?

Yes.

But IgnoreDeletions(ID) and ReplaceKeys(RK) determine returning true(T) or false(F) jointly.
The truth table is below, where

nil means add nils in the map

IC means If there are Changes, it can be true or false.

ID=T ID=F

RK=T IC nil+IC

RK=F F nil+IC

mengqiy · 2017-05-10T16:06:33Z

@janetkuo Will you have bandwidth to review this PR this week?

janetkuo · 2017-05-10T20:56:10Z

Yes I do

pwittrock · 2017-05-26T19:05:15Z

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

+		return mergepatch.ErrBadPatchFormatForReplaceKeys
+	}
+
+	// validate patch to make sure all field in patch is covered in replaceKeysList


// validate patch to make sure all fields in the patch are present in the retainKeysList. The map is used only as a set, the value is never referenced

pwittrock · 2017-05-26T19:06:14Z

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

+		m[v] = sets.Empty{}
+	}
+	for k, v := range patch {
+		if v == nil || strings.HasPrefix(k, deleteFromPrimitiveListDirectivePrefix) {


pull this into a function call "isDirective"

As we discussed offline, I will cleanup this in a followup PR.

pwittrock · 2017-05-26T19:07:28Z

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

+	}
+
+	// validate patch to make sure all field in patch is covered in replaceKeysList
+	m := map[interface{}]sets.Empty{}


map[interface{}]struct{}{}

pwittrock · 2017-05-26T19:08:25Z

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

@@ -641,9 +724,14 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeOptio
 		original = map[string]interface{}{}
 	}

+	err := processReplaceKeys(original, patch, mergeOptions)


applyRetainKeysDirectives

pwittrock · 2017-05-26T19:11:47Z

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

 			typedV, ok := v.([]interface{})
 			if !ok {
 				return nil, mergepatch.ErrBadPatchFormatForPrimitiveList
 			}
 			v = uniqifyAndSortScalars(typedV)
-		} else if k != directiveMarker {
-			fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
+		} else if k != directiveMarker && k != replaceKeysDirective &&


don't need
k != replaceKeysDirective
!strings.HasPrefix(k, deleteFromPrimitiveListDirectivePrefix)

mengqiy · 2017-05-26T21:05:52Z

Created #46543 to track the unsolved code style comments.

mengqiy · 2017-05-26T21:10:47Z

Addressed comments.

pwittrock · 2017-05-26T22:05:10Z

/approve

pwittrock · 2017-05-26T22:05:14Z

/lgtm

mengqiy · 2017-05-27T00:53:38Z

@pwittrock please reapply label

pwittrock · 2017-05-27T00:57:19Z

/lgtm

k8s-github-robot · 2017-05-27T00:57:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mengqiy, pwittrock

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

~~staging/src/k8s.io/apimachinery/pkg/util/mergepatch/OWNERS~~ [pwittrock]
~~staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/OWNERS~~ [pwittrock]
~~staging/src/k8s.io/apimachinery/third_party/forked/golang/json/OWNERS~~ [pwittrock]

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

k8s-github-robot · 2017-05-27T03:34:18Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-05-27T03:39:54Z

Automatic merge from submit-queue (batch tested with PRs 46302, 44597, 44742, 46554)

k8s-ci-robot · 2017-05-27T04:30:40Z

@mengqiy: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-kubemark-e2e-gce	`16e07c7`	link	`@k8s-bot pull-kubernetes-kubemark-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@apelisse

Automatic merge from submit-queue (batch tested with PRs 50919, 51410, 50099, 51300, 50296) Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy. With the new value in `patchStrategy`, the patch will include an optional directive that will tell the apiserver to clear defaulted fields and update. This will resolve issue like #34292 (comment) and similar issue caused by defaulting in volume. The change is [backward compatible](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md#version-skew). The proposal for this new patch strategy is in https://github.com/kubernetes/community/blob/master/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md The implementation to support the new patch strategy's logic is in #44597 and has been merged in 1.7. ```release-note Add `retainKeys` to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy. ``` /assign @apelisse /assign @janetkuo for deployment change /assign @saad-ali for volume change

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 18, 2017

k8s-ci-robot assigned pwittrock Apr 18, 2017

mengqiy force-pushed the replacekeys branch from 341ebdc to 0539ec2 Compare April 18, 2017 17:12

pwittrock assigned monopole and janetkuo and unassigned pwittrock Apr 18, 2017

janetkuo reviewed Apr 25, 2017

View reviewed changes

mengqiy force-pushed the replacekeys branch 2 times, most recently from 7d57439 to 43a1f2f Compare April 28, 2017 02:53

mengqiy force-pushed the replacekeys branch from 43a1f2f to d9ef931 Compare May 2, 2017 04:25

janetkuo reviewed May 3, 2017

View reviewed changes

mengqiy force-pushed the replacekeys branch from d9ef931 to 34607cf Compare May 3, 2017 23:28

pwittrock self-assigned this May 9, 2017

pwittrock reviewed May 26, 2017

View reviewed changes

mengqiy mentioned this pull request May 26, 2017

Cleanup the replaceKeys and setElementOrder code in strategic merge patch #46543

Closed

mengqiy force-pushed the replacekeys branch from 4e5dc4e to cb66659 Compare May 26, 2017 21:09

k8s-ci-robot assigned pwittrock May 26, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2017

Support replaceKeys patch strategy and directive

16e07c7

mengqiy force-pushed the replacekeys branch from cb66659 to 16e07c7 Compare May 26, 2017 22:06

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2017

k8s-github-robot merged commit 6927e70 into kubernetes:master May 27, 2017

liggitt mentioned this pull request Jun 4, 2017

Millions of conflicts on Nodes in kubemark-scale tests #46851

Closed

mengqiy mentioned this pull request Aug 8, 2017

Add retainKeys to patchStrategy for v1 Volumes and extentions/v1beta1 DeploymentStrategy #50296

Merged

mengqiy deleted the replacekeys branch September 26, 2017 03:39

KnVerey mentioned this pull request Oct 4, 2021

kyaml is not respecting $patch replace|retainKeys directives kubernetes-sigs/kustomize#2037

Open

support replaceKeys patch strategy #44597

support replaceKeys patch strategy #44597

Conversation

mengqiy commented Apr 18, 2017 • edited

mengqiy commented Apr 18, 2017

k8s-reviewable commented Apr 18, 2017

mengqiy commented Apr 18, 2017

pwittrock commented Apr 19, 2017

janetkuo commented Apr 19, 2017

mengqiy commented Apr 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Apr 28, 2017

mengqiy commented May 1, 2017 • edited

mengqiy commented May 2, 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

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

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

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

mengqiy commented May 10, 2017

janetkuo commented May 10, 2017

pwittrock May 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock May 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented May 26, 2017

mengqiy commented May 26, 2017

pwittrock commented May 26, 2017

pwittrock commented May 26, 2017

mengqiy commented May 27, 2017

pwittrock commented May 27, 2017

k8s-github-robot commented May 27, 2017

k8s-github-robot commented May 27, 2017

k8s-github-robot commented May 27, 2017

k8s-ci-robot commented May 27, 2017

mengqiy commented Apr 18, 2017 •

edited

mengqiy commented May 1, 2017 •

edited

pwittrock May 26, 2017 •

edited

pwittrock May 26, 2017 •

edited