Do we really require the difference between created and updated (and requeue in)? #281
Replies: 5 comments 13 replies
-
|
What are your opinions? Remove the difference between created/updated? Remove the requeue in feature? Or improve it to handle the watcher timeout? /cc @tillig @tomkerkhove @erin-allison @anekdoti among others :-) |
Beta Was this translation helpful? Give feedback.
-
|
It can be useful to have logic to understand if it's created or updated. For example when emitting events but it's not a must for me - don't have a preference |
Beta Was this translation helpful? Give feedback.
-
|
I think to distinguish between "created" and "updated" entities brings a edge-based flavour to an operator while it should work purely level-based. Therefore, I would consider to remove it. In regards to events, I would not emit an event that just tells me that the entity just got created - but I would more emit events informing that e.g. some dependent entities got created - and this has a better place when comparing the cluster state with the controlled entity. As for the requeue feature, I would be thankful for some more detailed explanation for the interaction between the watcher and requeuein g - e.g., why events can only pile up when the delay is bigger than the watcher timeout? In general, I would appreciate more documentation about these things. |
Beta Was this translation helpful? Give feedback.
-
|
I do not really agree on the event part but get your point. As said, no preference here. |
Beta Was this translation helpful? Give feedback.
-
|
I personally wire up "created", "updated" and "notmodified" all to the same handler and treat it as if they were just triggering a reconciliation loop; I don't care what calls it, when or why, just know that at that time, the state of the relevant resources should be verified/adjusted. As far as the requeue function, I don't have any issues with how it works right now, though I would agree with adding in some sort of deduplication (albeit I personally have zero clue how one would do so given that Reactive still looks like magic to me) |
Beta Was this translation helpful? Give feedback.
-
|
Ah, got it. Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
@buehler some further thoughts.... tl;dr the methods should be combined, and I believe it should not be made optional (i.e. don't let people implement one interface to allow a single method like "Reconcile" and another interface for "Created/Updated/NotModified") When the Kubernetes documentation discusses controllers and the operator pattern, an analogy is made to a basic control loop in automation and/or industrial systems design. Such a control loop behaves in a stateless manner operating on one or more process variables (our current state) and a set-point (our desired state) for each variable it then operates repeatedly to bring the process variable(s) to their set-point(s). At that point in time, however, the control loop does not simply exit or cease validating state; it continually works to ensure the state maintains its desired value, not that it just reached it at one point. In this manner, a Kubernetes controller will manage some set of dependent objects (the process variables) and periodically operate upon them until a desired state (the set-points) defined by something, such as a CRD, is reached. The controller generally doesn't, however, know of or see the past history of states; only what is present and currently desired, just as in a control loop. It is my opinion that having distinct operations for "Created", "Updated", and "NotModified" is detrimental because it opinionates the SDK by telling developers that they can (and possibly should) treat those operations differently. Not only does this go against the pattern of a stateless control loop, but it also implies that the events/resources can be treated in a stateful manner, which the library does not handle in its current state. To do so, the library would need to be able to reliably distinguish the events through persisting resource versions, actual objects or similar, which it is presently incapable of doing. https://kubernetes.io/docs/concepts/architecture/controller/#design |
Beta Was this translation helpful? Give feedback.
-
|
My thoughts exactly. One thing the SDK should handle tough: Finalizer Updated and Status Updated. What do you think? In my opinion, the difference between created/updated/notmodified should be removed, but the status updated and deleted methods should remain. |
Beta Was this translation helpful? Give feedback.
-
|
Agreed w.r.t. FinalizerUpdated + StatusUpdated to avoid infinite loops I'm at odds with myself about Deleted because technically it should just be treated as another state in the state machine, but for the sake of developer experience, I pretty much entirely agree with leaving it as-is. |
Beta Was this translation helpful? Give feedback.
-
|
I just thought about it, and the case of "deleted" is okay-ish. It is a defined watcher event and for the usecase of a "controlled deletion", one can use finalizer. |
Beta Was this translation helpful? Give feedback.
-
|
Another related observation: it seems that the operator triggers a created event the first time it sees a resource - which is not the actual creation of the resource, for example, when you restart an operator. This also might be misleading for someone, and a reason to drop it. |
Beta Was this translation helpful? Give feedback.
-
|
I'll admit I'm somewhat new to controller territory, trying to write an updated SMI adapter for Istio, so most of what I have to go on is what's already there and how it works in Go. From what I can tell, the Go controller-runtime basically does just boil it all down to a reconciliation handler to apply desired state. I haven't found any examples of controllers that somehow differentiate between, say, new and updated. I'm not sure if there is such a use case. That said, from a general API standpoint, I've always found it nice when a framework or SDK has the ability to be very granular in the various events that are available but also provides a nice glossy facade for me to use when I don't need them. I find that when the more granular events aren't present, that's when I suddenly somehow find a need for them. For example, maybe my security department wants to audit "new" vs. "update" events differently - having the ability to do that if I need to is huge. Definitely a contrived example, but... well, I've seen crazier requirements. |
Beta Was this translation helpful? Give feedback.
-
|
Totally agree with you. That was the reason why I started this project. I used operator SDK and kube-builder and had this exact problem. Now that I understand more of the internals of operators and desired/actual state, I agree with @erin-allison that you shouldn't care about created/updated and such. The problem still stands, that if the operator is restarted (because the pod is moved to another node, or for whatever reason), all found entities will trigger a created again. It is not truly "created". |
Beta Was this translation helpful? Give feedback.
-
|
Oh, yeah, I'm seeing that now. Just got my controller up and running and I'm seeing that "created" for every resource when I start a local debug session. Hmmm. |
Beta Was this translation helpful? Give feedback.
-
|
What I understand from the above thread is that now CreatedAsync, UpdateAsync,NotModifiedAsync are not required in controller and only one ReconcileAsync method is enough, right? If my understanding is correct, then Is there is a reason that templates mentioned here are still generating 3 different methods? Since, I am new to this, I am just trying to make sure, I start in the right direction :) Please advise on the right or suggested approach :) |
Beta Was this translation helpful? Give feedback.
-
|
The templates, same as the tests, reference the three methods differently because nobody's had the time to update all that code. |
Beta Was this translation helpful? Give feedback.
-
A question that rose from #279.
Is it required, that one can differentiate between "created" and "updated" during reconciliation?
When I first started the project, I thought it is required to tell those events apart. But then considering the intended way and operator works, then the difference does not matter. In "all" cases (created, updated, and not-modified), the operator should check the current state and compare it to the desired state in the entity. If something changes, the operator should apply changes to ensure the desired state again.
Furthermore, I'd like to question the usefulness of the "requeue in" feature when returning a result in a reconciliation function. When the requeue time-delay is bigger than the watcher timeout (currently 60s), then it could lead to piled up events (like described in #280). Maybe, there should be a check if the returned timespan is bigger than the configured watcher timeout. Because when the watcher terminates and recreates, all entities trigger an event again (most likely, a not modified one).
Beta Was this translation helpful? Give feedback.
All reactions