CreateOrUpdate performs Updates even if the object is unchanged #3191
Comments
ian-howell
changed the title
|
My gut reaction was that this would be related to changing
|
|
Oh this is even weirder. The Gomega
|
|
Why does that function return |
|
This addition to
So the issue must be occurring thanks to the |
|
Please provide |
|
If you CreateOrUpdate the same object twice in rapid succession then it will always trigger an update yes. This is because the cache from which it reads is eventually consistent, so it will not see the result of the first CreateOrUpdate in the second and issue an update (or create). |
|
I'm probably missing something, but it doesn't look like the test client uses a cache? Here's it's initialization:
and here's the docstring for client.New:
Anyway, I did some more testing and I think I've figured it out. The following is pulled from my branch. I recommend pulling and testing the branch, but I'll leave the contents here as well for convenience: It("updates only changed objects", func() {
// Prior to the test, deploy only has ObjectMeta. This is due to the BeforeEach.
op, err := controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr)
// After the above CreateOrUpdate, deploy will have deplSpec attached by the specr
// function.
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultCreated))
Expect(err).NotTo(HaveOccurred())
// Let's save a copy of deploy and then perform a no-op on it. We expect that existing
// will be equal to deploy after the no-op.
existing := deploy.DeepCopyObject()
// For sanity, make sure that DeepCopyObject actually worked.
Expect(existing).To(Equal(deploy))
// Call specr. This this is the same function called from CreateOrUpdate, we expect
// that it will be a no-op.
err = specr()
Expect(err).NotTo(HaveOccurred())
// However...
Expect(existing).To(Equal(deploy))
// The above function fails, but fortunately for us, it also prints the differences.
// It looks something like this (the common lines have been removed):
//
// <*v1.Deployment | 0xc000543908>: { | <*v1.Deployment | 0xc00014ef08>: {
// Replicas: 1, | Replicas: nil,
// TerminationMessagePath: "/dev/t | TerminationMessagePath: "",
// TerminationMessagePolicy: "File | TerminationMessagePolicy: "",
// ImagePullPolicy: "Always", | ImagePullPolicy: "",
//
// I took a look at the API for deployment, and it turns out that Replicas will
// default to 1 if not set. The remaining fields are part of the Container API type,
// and also have default values that align with the above.
//
// We know that the second specr should be a no-op, so what else could be going on?
// Let's revisit the *first* call to CreateOrUpdate. We made the assumption that after
// that call, the only contents of deploy would be the ObjectMeta and deplSpec that we
// defined. However, we can see from the above output that this isn't true. The
// contents of deploy (at the time) included the default values. I believe this
// happened because the Defaulter for the deployment would've been run against it.
// This also means that existing would have the default values. When we call specr, we
// wipe out those default values, and our test fails.
// Finally, suppose the above call to specr (and the assertions) is commented out.
// When we fetch the resource for the second time in the below call to CreateOrUpdate,
// we'll get the complete object, with defaults and all. But then when we call specr,
// we'll replace the entire Spec (which has the defaults), with deplSpec (which
// *doesn't* have the defaults), making the call to equality.Semantic.DeepEqual fail
// on every call, and thus CreateOrUpdate will *ALWAYS* perform an update.
op, err = controllerutil.CreateOrUpdate(context.TODO(), c, deploy, specr)
By("returning no error")
Expect(err).NotTo(HaveOccurred())
By("returning OperationResultNone")
Expect(op).To(BeEquivalentTo(controllerutil.OperationResultNone))
}) In conclusion, if the mutate function sets a field with a non-zero default to the zero value for that field, then it can never be the case that the resource fetched from the API server is equal to the resource passed to CreateOrUpdate, and it will always perform an update. |
|
I am still trying to understand what the issue is. If you use the method twice, wouldn't the Therefore the eventual consistency from what @alvaroaleman is talking about means that the resource is being "updated"? I would caution with the edge case only because conflating this behavior as inconsistent to change would likely force an update that will break other users. The tooling itself requires the mutateFn to always work on the object and that seems to make it not equal. Curious to understand more of why the expected behavior of even using this twice would result in a OperationResultNone. |
|
I think the issue is that their mutate function resets fields that get defaulted due to golang zero values, which is why there is always a diff. But short of duplicating all possible validations, I don't see how we can do anything about that in c-r |
|
Yes, the defaulting is the problem. @troy0820's answer made me notice something else though - if there are any values set on I agree with @alvaroaleman that c-r shouldn't be responsible for the defaulting problem, but perhaps we could update the documentation? I've created a PR with some potential additions: #3193 |
The controllerutil.CreateOrUpdate does not correctly handle updates.
Given the same resource multiple times,
CreateOrUpdatewill always perform an UPDATE api call, and report that it did so, even if there were no updates to the resource. This can be easily verified by making the following modifcation to the unit testThis diff results in the following error:
This is problematic:
Root Cause in this comment:
#3191 (comment)
The text was updated successfully, but these errors were encountered: