Make sure no op updates don't affect the resource version due to serverside apply #81673
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
release-note-none
jennybuckley
commented
Aug 20, 2019
| // Update the time in the managedFieldsEntry for this operation | ||
| managed.Times[manager] = &metav1.Time{Time: time.Now().UTC()} | ||
| // Update the time in the managedFieldsEntry for this operation if the fieldset changed | ||
| if originalManagedFields == nil || !managed.Fields[manager].Set().Difference(originalManagedFields.Set()).Empty() { |
There was a problem hiding this comment.
This would mean that we only update the time on these entries when the field set changes, not when the values change of something which is already owned.
Also, we don't need to check the reverse difference because an updater's fieldset can only grow by doing an update operation.
There was a problem hiding this comment.
an updater's fieldset can only grow by doing an update operation.
Can you elaborate on that? What if you remove an optional field?
There was a problem hiding this comment.
Removing a field or list item is something I didn't think of. We can check both directions of the difference then. But I wasn't sure if this is even the direction we want to go for when to update the time
There was a problem hiding this comment.
Thinking about it more, we could probably do this instead:
if c, err := liveObjTyped.Compare(newObjTyped); err == nil && !c.IsSame() {
|
/retest |
|
That's very important, thanks for doing it |
apelisse
reviewed
Aug 20, 2019
| "metadata": { | ||
| "name": "` + podName + `" | ||
| }, | ||
| "spec": { |
There was a problem hiding this comment.
Would it make sense to have a more sophisticated object so that we know that maps, associative list and everything keep things in order?
There was a problem hiding this comment.
We can do that, but as it is, the ownership already looks like:
"fields": {
"f:spec": {
"f:containers": {
"k:{\"name\":\"test-container\"}": {
"f:image": {},
"f:imagePullPolicy": {},
"f:name": {},
"f:resources": {},
"f:terminationMessagePath": {},
"f:terminationMessagePolicy": {},
".": {}
}
},
"f:dnsPolicy": {},
"f:enableServiceLinks": {},
"f:restartPolicy": {},
"f:schedulerName": {},
"f:securityContext": {},
"f:terminationGracePeriodSeconds": {}
}
}
There was a problem hiding this comment.
Great, multiple containers does help
There was a problem hiding this comment.
multiple containers and a few labels should cover most of the interesting features
k8s-ci-robot
added
size/L
apelisse
reviewed
Aug 20, 2019
| // Update the time in the managedFieldsEntry for this operation | ||
| managed.Times[manager] = &metav1.Time{Time: time.Now().UTC()} | ||
| // Update the time in the managedFieldsEntry for this operation if the object changed | ||
| if same, err := f.isSame(liveObjTyped, newObjTyped); err == nil && !same { |
There was a problem hiding this comment.
We should run before/after benchmarks to see if that's fine
| "metadata": { | ||
| "name": "` + podName + `" | ||
| }, | ||
| "spec": { |
There was a problem hiding this comment.
Great, multiple containers does help
jennybuckley
force-pushed
the
grouping-etcd
branch
from
1ac9b12
to
bc63b7e
Compare
|
|
I'm guessing mostly because compare is still allocating a merged object which we throw away |
|
Yeah, that's what I was expecting too ... |
|
And the other benchmarks: |
jennybuckley
force-pushed
the
grouping-etcd
branch
from
a1591b1
to
5524043
Compare
|
Okay, without the comparison, the new benchcmp is much better: |
jennybuckley
force-pushed
the
grouping-etcd
branch
2 times, most recently
from
8e5b6e6
to
46bddb7
Compare
apelisse
reviewed
Aug 20, 2019
| // operations from the same manager if the object changed | ||
| if f, ok := managed.Fields["self"]; ok { | ||
| if old, ok := managed.Fields[manager]; ok { | ||
| f = fieldpath.NewVersionedSet(f.Set().Union(old.Set()), f.APIVersion(), f.Applied()) |
There was a problem hiding this comment.
Interesting how we could remove that union from smd now
There was a problem hiding this comment.
Yeah we could do that and simplify things, just need to make sure that people don't give the same manager name to smd as any existing manager for updates (maybe return an error?)
There was a problem hiding this comment.
I think it's fine. As long as we never send the same manager, the union is always going to be a no-op?
jennybuckley
force-pushed
the
grouping-etcd
branch
from
7af95df
to
572d2dc
Compare
jennybuckley
force-pushed
the
grouping-etcd
branch
from
572d2dc
to
aa1f01e
Compare
|
@apelisse the tests are passing now |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, jennybuckley The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
/priority important-soon
/sig api-machinery
/wg apply
/cc @apelisse
What this PR does / why we need it:
Adds a test to make sure no op updates don't affect the resource version due to serverside apply, and fixes a bug where this wasn't true
Does this PR introduce a user-facing change?: