Add a method to generate a strategic merge patch #13007
Conversation
| // If two map[string]interface{} are given, all elements must match. | ||
| func matchesValue(av, bv interface{}) bool { | ||
| switch at := av.(type) { | ||
| case string: |
There was a problem hiding this comment.
I don't think you need explicit primitive casting - see http://play.golang.org/p/1xOcMJJ5fO.
There was a problem hiding this comment.
You should be able to collapse this to
case string, float64, bool:
return bt == at
|
GCE e2e build/test passed for commit d9568b7bd5c5c3c2ab0d6616fff8767961ec8308. |
| var errBadJSONDoc = fmt.Errorf("Invalid JSON document") | ||
| var errNoListOfLists = fmt.Errorf("Lists of lists are not supported") | ||
|
|
||
| func CreateStrategicMergePatch(a, b []byte, dataStruct interface{}) ([]byte, error) { |
There was a problem hiding this comment.
I would prefer the variable names original, modified vs a, b to keep consistent with StrategicMergePatch's original, patch, but that's just a personal preference and up to you if you want to change it.
There was a problem hiding this comment.
+1 for original, modified as var names
|
This is awesome. Reconcile functionality and deployment objects together are going to enable us to switch from updating our clusters with kubectl to using git push + autoreconcile. Very very excited. |
|
@kubernetes/kubectl |
|
@jlowdermilk Do you have time to take a look? |
|
cc @sparkprime |
| @@ -31,7 +31,352 @@ import ( | |||
| // lists should be merged or replaced. | |||
| // | |||
| // For more information, see the PATCH section of docs/api-conventions.md. | |||
|
|
|||
| // CreateStrategicMergePatch creates a patch that can be passed to StrategicMergePatch. | |||
There was a problem hiding this comment.
The idea here is that we'd diff the original config (stored in an annotation) and updated config, and then apply the patch to the live state?
That's a reasonable starting point.
We'll also want to be able to detect where the live state has diverged from the original (and perhaps new) configuration.
There was a problem hiding this comment.
We'll need 2 diff modes:
One that detects all changes: changes, deletions, additions. Used to generate the patch, and maybe to determine whether anything has changed.
One that detects just changes and deletions. Additions would be ignored. This would be used to detect divergence. Initially we could barf if divergence were detected. Eventually we'll want --force to override the changes in the live state.
Export (#13038) could be the means by which a new config could be generated from the live state, if that's what the user wanted.
There was a problem hiding this comment.
Agreed that we need a mode that detects changes and deletions, while ignoring additions. I've added a boolean parameter to CreateStrategicMergePatch called ignoreAdditions. The check for divergence can be made by calling it with this parameter set to true, and then looking at the result. If and only if it's empty, then there were no changes or deletions between original and modified.
There was a problem hiding this comment.
Rather than bail if we detect any divergence between the original configuration to the live state, I think we want to bail only if we detect conflicts between such a divergence and the patch that the user now wants to apply. That's really the point of generating the strategic merge patch, right? We can apply the changes that the user is requesting on top of changes made by automated processes, if and only if there are no conflicts between the two change sets.
Ignoring for the moment the possibility of conflicts between different but related properties, we should bail if cases where the same properties are changed in conflicting ways. In other words, we should bail if there are any properties in both change sets that have different values. We're fine if one change set changed a property that the other change set didn't touch, and we're fine if both change sets changed the same property to the same value.
The OpenShift json merge implementation detects these simple conflicts by generating the patch required to go from original to live, and then comparing it with the patch being applied. However, it doesn't attempt to ignore additions, since they don't generate conflicts anyway. In other words, we didn't really the flag to ignore additions. If we don't have a compelling use case for it, then I think it would be cleaner to back it out and always generate a complete patch that includes additions, as well as deletions and modifications.
The OpenShift json merge implementation also provides a mechanism for detecting more complex conflicts involving incompatible changes to related properties: it allows the caller to register a set of preconditions and then plays them against any given patch before applying it. If the caller knows about constraints that span two or more properties, then they can write preconditions to look for violations of those constraints, and register them with the library. I'm inclined to include this feature, since it's small and simple, and provides significant power.
There was a problem hiding this comment.
Let's walk through an example.
Original object stored as annotation:
containers:
- name: a
image: x
- name: b
image: y
New local object to reconcile:
containers:
- name: b
image: y
settings:
flag1: true
Current server object:
containers:
- name: a
image: x
- name: b
image: y
settings:
flag2: true
- name: c
image: z
Patch we should generate:
containers:
- name: a
$patch: delete
- name: b
image: y
settings:
flag1: true
I am going to assume that, not only is the above correct, but that the above situation would not cause a conflict. flag1 and flag2 would simply merge together and both be set to "true". There are two situations I can think of where the data would be such that we could throw an error, if we want it to:
- If "Current server object" had some information added to container "a", then we could decide whether that throws a conflict. I think it would be safe to say that it doesn't. It also would be hard to throw an error because it can't be deduced just by looking at the patch and the server object.
- If "Current server object" didn't have flag1 set to true, but instead flag2 set to false. That would represent a true irreconcilable conflict. So we could either force-clobber, or we could try to detect and warn. If we do want to detect, as @jackgr says, I think we could do so without needing a non-additions API and instead detect by generating the patch and comparing it to the live server object.
In other words I think I'm agreeing with @jackgr that we don't need the additions API and a comparison of the live server object with the patch should be sufficient. @bgrant0607 can you outline a scenario where that is not sufficient?
@jackgr re: incompatible change constraints, should that kind of thing not just be implemented in the server-side validation of the final state of the objects post-patch, instead of being re-implemented and re-checked in this library?
There was a problem hiding this comment.
Just had a long chat with @jackgr. It looks like we may be on the wrong track with this approach. Based on what is currently implemented in this PR, here is how kubectl apply would work:
- Get the live object from the server along with the original annotation.
- Compare the local object to the original annotation and create a patch of the differences between the two. Then send that patch to the server as a strategic merge patch.
The issue with this approach is the use case for when I run kubectl apply on a resource that I have not changed since the last configuration run, but the live server state has modified fields that are explicitly defined in my local object. This should not be a conflict - any and all fields specified in our local object should always simply override all the server object fields. The above approach does not accomplish this because it has no way to detect such changes to the server object. Therefore it does not give us true consistent reconciliation of the local config object to the server every time we run kubectl apply.
The alternative approach (originally outlined in #1702 (comment)) is as follows:
- Get the live object from the server along with the original annotation.
- Compare the local object with the original annotation to generate a new JSON object which is equivalent to the current local object but decorated with null's (or
$patch: delete's) needed to remove any fields that were removed in the current local object but existed in the original annotation. - Submit that new JSON as a Strategic Merge patch.
In other words, the local config object IS the patch itself sans deletions of removed fields. So the only thing we need to add is a method like AddDeletionsToPatch, which would take the local object and the original annotation as parameters and return the local object decorated with the right deletions (e.g. mapKey: null and $patch: delete).
@bgrant0607 can you confirm that the second approach is what we actually want and we should go in that direction instead?
There was a problem hiding this comment.
Actually, that's almost correct. The current approach can detect changes to the server object, since it computes the divergence in the live state from the original. However, it chooses to fail the update with a conflict if the live state contains changes that are not consistent with the local object.
Without this conflict detection, there isn't much point in an automated process ever making changes to the live state that deviate from the annotation, since they'll simply be discarded the next time the user runs kubectl apply.
Of course, that approach begs a question of timing. How often should the user run kubectl apply to ensure that the live state still conforms to their local configuration? Once an hour? Once a minute? Once a second?
Whatever the answer, it defines a window during which the user willingly lets the configuration deviate from their intent. Perhaps they should watch for changes, and quickly update the object if they detect any divergences? Of course, I'm being facetious.
If what we really is for the configuration to never diverge from what the user specified, then we should simply prevent callers from making conflicting changes in the first place. The annotation contains the configuration requested by the user, and the state is never allowed to deviate from it, other than through additions. This policy can be easily enforced on the server side.
To implement kubectl apply, we compare the live state with the annotation to compute the additions made by automated processes, add them to the local object, and then PUT it to the server.
There was a problem hiding this comment.
If you haven't already, please read:
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#spec-and-status (the spec part)
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#defaulting
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#late-initialization
"The specification is a complete description of the desired state, including configuration settings provided by the user, default values expanded by the system, and properties initialized or otherwise changed after creation by other ecosystem components (e.g., schedulers, auto-scalers), and is persisted in stable storage with the API object."
Examples of fields that are automatically set:
- Default values: pod.spec.restartPolicy, pod.spec.containers[].imagePullPolicy
- Admission control: pod.spec.containers[].resources.limits.cpu
- Late initialization: pod.spec.nodeName, service.spec.clusterIP
- Horizontal auto-scaling and rolling update: replicationcontroller.spec.replicas
- Vertical auto-sizing: pod.spec.containers[].resources.requests.cpu
Some of these fields are immutable once set, and there are readonly fields in the metadata that can't be set by the user, either.
Due to all of these processes, it is not possible for a user to edit their original config file and feed it to kubectl replace, which expects a fully specified resource, at least not without using --force, which is very disruptive, since it deletes and recreates the resource.
THAT is the workflow that I want to enable: edit your configs and do kubectl apply -f myconfigs/. Everything else, such as conflict detection, is gravy.
Everything not explicitly specified by the user in their configs should be reserved for operational tweaking, both manual (kubectl scale, kubectl label, kubectl patch, etc.) and automated (auto-scaling, auto-sizing, automated deployments, etc.).
As I mentioned elsewhere, the user should not be re-running kubectl apply just to ensure conformance of the live state to their desired state. If the live state diverges, it's either user error, such as misconfiguring an auto-scaler, or intentional, which is where conflict detection upon the next update they try to do would be nice.
There was a problem hiding this comment.
Also read http://kubernetes.io/v1.0/docs/user-guide/managing-deployments.html
I don't want users to have to figure out whether to use patch or replace, how to construct a patch, whether to use --force or not, etc.
And we can't expect users to explicitly (esp. manually) specify every field they don't care about. That would break forward compatibility -- we wouldn't be able to add new fields to the API without forking a new API version.
There was a problem hiding this comment.
Please write all this up in a design doc, btw.
k8s-github-robot
added
the
size/XL
|
Labelling this PR as size/XL |
|
GCE e2e build/test failed for commit b38bdecd56828c750117e74f2db92b3d523feb34. |
|
GCE e2e build/test passed for commit 9b8447bb8ce80a14fd38d0969655e5b62aea1a12. |
|
The changes that I just pushed in commit 9b8447b address all of the issues raised in the comments. However, per this reply, I think we should back out the flag to ignore additions, and add the logic from the OpenShift json merge implementation that checks for conflicts. It should be the last change to this PR, modulo bug fixes. |
k8s-github-robot
added
size/XXL
|
Labelling this PR as size/XXL |
|
I have a question - I may have misunderstood parts of the ongoing discussion, so apologies in advance if this is clearly covered. We're racing on a lot of client commands with controllers - so much so that repeatable scripting is becoming difficult. For example, a command that creates an RC and then a subsequent command that adds a label fails frequently because they are conflicting over update. Will SMP be useable in the client commands to record enough of a stable diff that we can safely merge on the server? I assume so, but I've neglected this thread and others. So kubectl label should be reading, generating a patch (with before values included?) and sending that to PATCH, so that as long as another process doesn't modify the label field, the update will succeed? |
|
PATCH does not need the before values included (for maps by default and for lists because of SMP), and by default will overwrite. So as long as the specific label key you're setting isn't defined in the config that's getting reconciled, you should not have any issues. |
jackgr
force-pushed
the
diff_and_patch
branch
from
377fbf2
to
1cf66d4
Compare
|
Changed, rebased, squashed and ready to merge if no further comments. This PR now just adds a method to create a strategic merge patch from the original and modified files. cc @bgrant0607, @ghodss, @janetkuo |
|
GCE e2e build/test passed for commit 377fbf2c26dc1f1d0a05e2b92035a4048e06c3f4. |
|
GCE e2e build/test failed for commit 1cf66d4a574b8259cdaef3d692e61ef64da618d1. |
|
@jackgr Looks like you need |
jackgr
force-pushed
the
diff_and_patch
branch
from
1cf66d4
to
3ecb1ba
Compare
|
GCE e2e build/test failed for commit 3ecb1ba. |
|
@k8s-bot test this please |
|
GCE e2e build/test failed for commit 3ecb1ba. |
|
@k8s-bot test this please |
|
GCE e2e build/test passed for commit 3ecb1ba. |
|
@jlowdermilk @ghodss since the reconcile part is moved to the other PR, this one should be ready to merge now. I have another PR depending on this one so if you all agree we can merge it. |
|
lgtm |
janetkuo
added
the
lgtm
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 3ecb1ba. |
|
Automatic merge from SubmitQueue |
Auto commit by PR queue bot
This is the first step in implementing #1702. It adds a method named CreateStrategicMergePatch to pkg/util/strategicmergepatch/patch.go that generates a strategic merge patch by taking the difference between before and after YAML or JSON strings. It will be used by apply (aka reconcile) to generate patches that can be used to update live configuration based on the difference between the current and previous configurations of a resource.
cc @bgrant0607, @ghodss