Add method to apply strategic merge patch #13820
Conversation
|
cc @kubernetes/goog-ux |
k8s-github-robot
added
the
size/XXL
|
Labelling this PR as size/XXL |
|
Please ping or reassign when this is ready for review. cc @ghodss |
|
The code looks good to me but I don't a use case for this method. As per #13007 (comment) and then #13007 (comment) I think all we need now is the delete-decorator method, and then we can reuse the StrategicMergePatch method with the resulting patch and be done with prerequisites for kubectl apply (i.e. #1702). The more urgent patchset currently needed is that delete-decorator method, but we can merge this as well if we think there's another use case that warrants the additional complexity. |
|
Reimplemented the method using the semantics described by @ghodss above. @mikedanese ready for review. |
jackgr
changed the title
|
Unit, integration and GCE e2e build/test failed for commit 6b34700d47f7ce369c3b5d2eb797cb57ac7a76af. |
|
cc @kubernetes/kubectl |
| // of the documents is invalid, or if there are any preconditions that fail against the modified | ||
| // configuration, or, if force is false and there are conflicts between the modified and current | ||
| // configurations. | ||
| func CreateThreeWayMergePatch(original, modified, current []byte, dataStruct interface{}, force bool, fns ...PreconditionFunc) ([]byte, error) { |
There was a problem hiding this comment.
My understanding is that the "delete-decorator" method I was describing would take "patch" (the local config file given to kubectl apply) and "previous" (i.e. the content of the annotation) to produce a new strategic merge patch with delete directives for missing fields. That patch then gets sent directly to the server which applies it with the pre-existing StrategicMergePatch method. Why do we need a method that is creating its own patch from scratch using original and modified?
There was a problem hiding this comment.
This approach reuses existing proven code, instead of adding a whole new set of recursive methods, and has exactly the same semantics, except that the patch does not update fields that are already set correctly, making it smaller. Bottom line... less code and more efficient.
jackgr
force-pushed
the
apply_patch
branch
3 times, most recently
from
81545ea to
17bf288
Compare
|
Unit, integration and GCE e2e build/test failed for commit 17bf288438e359eab77789a58d60152e61599f5b. |
|
Unit, integration and GCE e2e test build/test passed for commit 46666b5225acf28f66fad6323e96532ea3ab4c4c. |
|
Unit, integration and GCE e2e test build/test passed for commit aa65108d6633ed3cafd4880d4463d6a5374395e7. |
|
The precondition and other higher level code looks fine to me, I'll defer to Sam on the deeper flows because I haven't followed the SMR and 3way discussions for context. |
| // IsPreconditionFailed returns true if the provided error indicates | ||
| // a Delta precondition did not succeed. | ||
| func IsPreconditionFailed(err error) bool { | ||
| return err == ErrPreconditionFailed |
There was a problem hiding this comment.
This is fragile, as it's effectively string comparison on a fixed error string. Better to make ErrPreconditionFailed and ErrConflict structs that implement error and do a type comparison. Added bonus, you can print a more useful error message, i.e. the actual conflicting diff or failed precondition.
There was a problem hiding this comment.
Fixed this issue a couple of commits ago.
|
Unit, integration and GCE e2e test build/test passed for commit e75ee184b82a96468d1a4a62e54484f884ec938e. |
|
@jlowdermilk Done. PTAL. |
| } | ||
|
|
||
| func newErrConflict(patch, current []byte) errConflict { | ||
| s := fmt.Sprintf("patch:\n%v\nconflicts with current:\n%v\n", patch, current) |
There was a problem hiding this comment.
Either use %s in the format string, or convert byte arrays to string (i.e. string(patch)). Otherwise you get illegible output that looks like patch: [123 34 102 111 111 34 58 34 ...
|
LGTM. Thanks for your patience @jackgr. Let's get it in! |
j3ffml
added
the
lgtm
|
Unit, integration and GCE e2e test build/test passed for commit 05fde753e85a343e575c2ceee2ff97bd93d53cea. |
|
Needs a rebase before it can be merged |
a-robinson
added
the
needs-rebase
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 05fde753e85a343e575c2ceee2ff97bd93d53cea. |
|
Automatic merge from submit-queue |
k8s-github-robot
removed
the
needs-rebase
|
GCE e2e test build/test passed for commit 9ff2e43e9a61b7d1bab762f03cfb4ecc72c88fee. |
|
LGTM was before last commit, removing LGTM |
k8s-github-robot
removed
the
lgtm
j3ffml
added
the
lgtm
|
@jlowdermilk This PR has been stuck on a Shippable build break for over a day. The offending logs are as follows: Testing api version: Unless I'm mistaken, this test failure is not related to this PR. Does anyone have any insight as to how to get past it? |
|
@jackgr shippable was broken at head for awhile, you probably picked up the bad change. Rebasing (yet again, sorry) should fix. |
|
GCE e2e test build/test passed for commit ddda379. |
|
Removing LGTM since there was a change after LGTM was granted. @jlowdermilk could you please take a look? |
piosz
removed
the
lgtm
j3ffml
added
the
lgtm
Add method to apply strategic merge patch
This PR builds on #13007 by adding a method to apply a strategic merge patch created by taking the difference between the original and modified user configuration and applying it to the current live configuration. This method detects and by default will automatically resolve conflicts in favor of the user. However, per discussion on #13007, we may want to add another method that applies the modified user configuration, plus delete directives relative to the original user configuration as the patch. Also, per discussion on #13576, we may want to revisit the question of conflict detection and resolution. Pending the development of any additional methods based on the outcome of those discussions, the method added by this PR can be used to do the equivalent of a git rebase -s theirs.