Skip to content
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

support replaceKeys patch strategy #44597

Merged
merged 1 commit into from
May 27, 2017

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Apr 18, 2017

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

support replaceKeys patch strategy and directive for strategic merge patch

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

mengqiy commented Apr 18, 2017

/assign @pwittrock

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 18, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Apr 18, 2017

@k8s-bot bazel test this

@pwittrock pwittrock assigned monopole and janetkuo and unassigned pwittrock Apr 18, 2017
@pwittrock
Copy link
Member

@janetkuo Would you be able to review this PR?

@janetkuo
Copy link
Member

@pwittrock yes

@mengqiy
Copy link
Member Author

mengqiy commented Apr 24, 2017

@janetkuo ping

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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

Choose a reason for hiding this comment

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

Note: use merge,replaceKey as we just discussed

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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

replaceKeysDirective = "$replaceKeys"
Copy link
Member

Choose a reason for hiding this comment

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

We're not using $patch: replaceKeys anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed back to $patch: replaceKeys.

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

mengqiy commented Apr 28, 2017

@k8s-bot unit test this

@mengqiy
Copy link
Member Author

mengqiy commented May 1, 2017

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
Copy link
Member Author

mengqiy commented May 2, 2017

@k8s-bot unit test this

// MergeDeleteList indicates if we are merging the delete parallel list.
MergeDeleteList bool
// ProcessDirectives indicates if we are merging the delete parallel list.
ProcessDirectives bool
Copy link
Member

Choose a reason for hiding this comment

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

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

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

}

// 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
Copy link
Member

Choose a reason for hiding this comment

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

separated by ","

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

// 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"
Copy link
Member

Choose a reason for hiding this comment

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

optional but maybe we could reuse strategicpatch.ReplaceKeysDirective

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

update comment for returned value

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

mention ReplaceKey too

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}
// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

mengqiy commented May 10, 2017

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

@janetkuo
Copy link
Member

Yes I do

return mergepatch.ErrBadPatchFormatForReplaceKeys
}

// validate patch to make sure all field in patch is covered in replaceKeysList
Copy link
Member

@pwittrock pwittrock May 26, 2017

Choose a reason for hiding this comment

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

// 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

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

Choose a reason for hiding this comment

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

pull this into a function call "isDirective"

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

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

@pwittrock pwittrock May 26, 2017

Choose a reason for hiding this comment

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

map[interface{}]struct{}{}

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

err := processReplaceKeys(original, patch, mergeOptions)
Copy link
Member

Choose a reason for hiding this comment

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

applyRetainKeysDirectives

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 &&
Copy link
Member

Choose a reason for hiding this comment

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

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

@mengqiy
Copy link
Member Author

mengqiy commented May 26, 2017

Created #46543 to track the unsolved code style comments.

@mengqiy
Copy link
Member Author

mengqiy commented May 26, 2017

Addressed comments.

@pwittrock
Copy link
Member

/approve

@pwittrock
Copy link
Member

/lgtm

@k8s-ci-robot 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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2017
@mengqiy
Copy link
Member Author

mengqiy commented May 27, 2017

@pwittrock please reapply label

@pwittrock
Copy link
Member

/lgtm

@k8s-ci-robot 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
Copy link

[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:

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
Copy link

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

@k8s-github-robot
Copy link

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

@k8s-github-robot k8s-github-robot merged commit 6927e70 into kubernetes:master May 27, 2017
@k8s-ci-robot
Copy link
Contributor

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

k8s-github-robot pushed a commit that referenced this pull request Aug 29, 2017
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
@mengqiy mengqiy deleted the replacekeys branch September 26, 2017 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.