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
CreateOrPatch / CreateOrUpdate do not set status properly when the object is created #2627
Comments
In the case of |
/kind support I’m wondering if this is by design. When creating an object with the client.Client, the status doesn’t get persisted because it doesn’t use a Creating an object shouldn’t have the status be carried with the object. The fake client tries to mimic this behavior for unit tests. |
Agreed for the But the It currently just does not run that code path when creating the object And since the mutate function is not supposed to know whether we’re creating or patching, I would lean toward patching the status in the same way the existing object path does (this is what the patch I proposed does, it uses the status patch code path if needed for both existing and new object. You can use the operation result to know if an object was created and requeue (this is the “workaround” I used to handle this behavior), but the fact that we patch the status in one case and not in the other seems very counter intuitive. |
And just to make sure there is no confusion, I’m talking about the |
I believe that’s intentional from a design perspective. Patching would patch both the object spec and the status if and only if the object already exists . Create would only create the object and not the status. The same logic is said to be used for create or update, from how it handles what exists and what doesn’t. I wouldn’t expect createorpatch to act differently than what we agreed create or update to do.
We would be patching against an object that doesn’t exist to deal with the create as you suggest. Making a patch against an object that doesn’t exist doesn’t settle the creation. |
Quote from the “documentation” from CreateOrPatch:
It is essentially saying it will use whatever calls are needed depending on the state of the resource. Since it does support patching the status in the case the object already exists (and it goes out of its way to do it), I really think that for consistency sake, it makes sense to also support setting the status in the create path. My patch just re-use the existing code with an empty object as the original state. From there we can derive a status patch based on the change made in mutateFn (again existing code), and ultimately patch the status if needed. If you still don’t think this makes sense, please feel free to close the issue |
The
In trying to keep the behaviors consistent with what the verbs do, I would assume what you propose would have to be done on create or update as well and change that behavior. We seem to have agreed in this issue that updating on an object that doesn’t exist wouldn’t settle the status on an object on the create case. So why would we settle on the status being carried on the create case on createOrPatch?
If we want to carry the status on creation of an object, I would contend that this behavior should be consistent in both functions like they are today or both changed to their respective functions. However, I don’t believe we want to conflate the creation of objects as being something that is also setting the status on the object from creation. |
|
The
The current
As you can see those are quite different (I’ve slightly simplified the patch case for brevity) |
I definitely agree |
We agree. I’m still not clear on this:
In your above example, both functions even after they mutate do not do anything to the status of the object being created in the create case. Only after patch (still a case for update to do this) if the object exists would we patch the status because the existing object is now different from what was fetched. My case is to express that we should probably not be setting the status on the create case for several reasons beyond the subresource not being invoked to do so on either case during creation. If we need to modify the update case in createOrUpdate to mimic the behavior of createOrPatch to satisfy the consistency, and update the status during update, that seems reasonable and not out of bounds for this discussion. |
That’s interesting. You’re considering parity between the two methods being the more important while I am actually considering that parity inside a method is more important and was not utterly worried having a difference of behavior between the two (though the behavior of each should be documented). What are your concerns regarding patching the status “during” the create? (technically it will be after the create through another patch call). |
Both functions create or update/patch the object. I would assume that the create case is the same for both functions (just create the object and not do anything to the status). The distinction isn’t in the logic but how the behavior exists today where the gap isn’t in the create case to settle the status on creation. Create makes an event which then gets reconciled to have the Reconciler update its status dependent on its conditions gathered by the state of the world the controller “reconciles” the object. Providing that status on creation seems counter to that logic. Especially if it’s for one function and not both.
Settling status with create can do things unintended for state of the object. If you were to create a pod and provide the status where the pod is running, that doesn’t reflect the lifecycle of the pod until that event occurs and it’s settled by its own controller. Making another call to patch the status after you create the object doesn’t sound like the intention of the function. If you need to patch the status of the created object, you could call it again after it’s been created and go down the existed path. |
This would only happen if the user actually changed anything in the status (you’ll see that the existing code checks if changes were made before issuing any of the patch / update calls). Assuming the user does not change anything, it won’t make any call against the status. So it only happens if there is a clear intent from the user to make a change. I ran into this issue with a kind of special case controller. It creates CRD instances that mirror the state of an external system for consumption by other controllers. So since the code only issues calls if there is actually a change made (change made inside MutateFn), I felt it would make sense to support it, even more-so since the MutateFn is not supposed to know if it’s in the existing / new object path (other than looking into object to see if there is a uuid or anything that will be set after creating it - but those kind of indirect guesses don’t feel right) If anything, I think the documentation should be improved to highlight what can / can’t be updated and in what circumstances. I can suggest some changes Regarding the case of |
When creating an object, status doesn't get carried to the object because two different writers are responsible for dealing with the request. One request (the creation of the object, only deals with the spec and the status is dealt with a status writer which writes to a different endpoint). We agree that a call to You specified in this issue that the behavior for creating an object ignores the changes to the status stanza.
I would also highlight this:
This sounds like a design decision outside of the scope of what is intended with the function. If you are utilizing external objects to replicate the objects so that the controller can do things with it, I would ask why not use a structure that will not recreate external objects but utilize the external object information for consumption? If you need to recreate the objects and have to carry the status, I suggest that you create the object, then patch the object's status in two separate calls, rather than conflating the functions intention around a use case that's not built with what the api or the documentation provide.
From what you are experiencing, it seems what you are asking for is to have this function provide a way to copy the status of an object during creation when neither function does that for create. Doing so will not only break the behavior of what is described by the api and documentation but also create a case where we assume that every object being created has a subresource needed to copy the status on creation.
If we need to update the |
I still think there is a disconnect and I’m not explaining myself correctly but I don’t want to waste more of your time. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues. This bot triages un-triaged issues according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
When using the
CreateOrPatch
orCreateOrUpdate
methods, the status of the object won't be persisted if the object does not exist.In both functions:
The method returns after the Create call.
However, this does not set the status of the object (see https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#status-subresource for instance
PUT/POST/PATCH requests to the custom resource ignore changes to the status stanza.
)The code needs to be updated to actually do the status update part even on creation. I'll have a look at it and make a proposal for the change
The text was updated successfully, but these errors were encountered: