Expose old objects along with watchevents from a reflectors' stream

[thomastaylor312]

In the Go library, the ResourceEventHandler Update method receives the old and the new object. This is very useful for cases when you want to compare what has changed and take the appropriate actions (and example of this can be found in Krustlet).

After looking at the code, I am not quite sure where to do this. The Reflector has a cache that we could use for this, but the desired behavior is in the Informer. I don't think we'd want to also have a cache in the Informer, so any suggestions here would be welcome if this is a feature that people would want

[clux]

That's a useful suggestion. At least the spirit of it. We can't actually add it to WatchEvent because that's a kubernetes api thing (forcing something to track the old state) however we may be able to extend the Reflector/reflector abstractions so they can emit events along with extra data.

If you feel this is urgent I'd suggest trying out the branch in #258 where we rewrite a large part of the runtime structures (which is likely to land pretty soon). The reflector API therein allows using your own Store and intercepting WatchEvents at a point where you have access to the current cache. Though there might be a better interface lurking beyond what we currently got.

[clux]

Have updated the title to be more feasible imo, hope that makes sense.

[thomastaylor312]

Let me give it a whirl against that branch and see what I can do. I'll try and come up with something sane for the API on top of the reflector

[nightkr]

In general this seems like a bad idea, since:

1. It introduces a difference between "modified while the controller was running" and "modified while the controller was stopped", that is easy to forget about
2. It's easy to introduce differences between "A->B->C"-state transitions, and going straight from A to C.

I'm not very familiar with krustlet in particular, but don't your "real" containers remember which image they were instantiated for? I'm pretty sure docker inspect contains this, for example. That seems like a more reliable thing to compare against than the old pod manifest, since the container runtime might have a lifecycle that differs from your kubelet's.

[thomastaylor312]

I don't necessarily think this is a bad idea. Like I said, the Go library does this exact thing. Although that doesn't make it Right™, I have always understood it to be in context of the informer. The informer is telling me "this is the last one I had in storage and the new one I just got." We will eventually get all events (hence k8s being "eventually consistent") and modify it. Also, if a controller was stopped, the first event you get is an add, which handled differently anyway.

The image example in Krustlet most definitely can be solved other ways, but often times, you get a modified event and you want to know what changed from the last one to this one (this happens a lot when I have written custom controllers). Which is why I suggested this feature. Does that clarify my reasoning a bit?

[clux]

I can see it being helpful in certain situations, however, you are straying from the conventional "default good practice" of writing idempotent reconciliation loops and are instead trying to be smart by trying to cross examine old state vs new state.

A normal solution would be:

• handle each watchevent equally
• check that the state of the world reflects what you can see (perhaps making api calls against related resources)
• if you need to keep track of actions yet to be performed on an object it should be noted on its .status object

The benefit of the normal solution is a system that is resilient to losing events and eventually consistent.

[thomastaylor312]

Yeah, I think you're completely right here. Definitely written too much using the Go library and got stuck in that paradigm. So I didn't think it all the way through. I'll go ahead and close this

[clux]

Ok, that's good to hear. Simplifies it on our end.

Expanding on this a bit for future reference: This would be a hard thing to do do correctly in the runtime anyway. A reflector might drop its connection, and/or need to resynchronize (see #219), so you will have cases where we are not able to present a diff even if we were keeping state. Not a great thing to rely on.