wave-k8s / wave

Kubernetes configuration tracking controller
Apache License 2.0
646 stars 82 forks source link

simplify code and remove ownerReferences and finalizer #154

Closed jabdoa2 closed 1 month ago

jabdoa2 commented 1 month ago

Implementation of #153. This is fully feature complete.

Please let me know what you think.

wonderhoss commented 1 month ago

I don't have time to look into the code in detail, but I did want to ask: have you thought through the concurrency implications of this change?

The reason we originally used Finalisers and OwnerRefs was to avoid having to manage scenarios where a Wave Pod might get terminated mid-reconcile and lose internal state or a scenario where two Wave Pods might be running simultaneously in a degraded cluster. Handling these cases gracefully requires external state.

By offloading this to the kube API, we got these cases handled atomically for free.

I'm not saying this is a bad change, but I did rant to chime in with thoughts.

jabdoa2 commented 1 month ago

I don't have time to look into the code in detail, but I did want to ask: have you thought through the concurrency implications of this change?

The reason we originally used Finalisers and OwnerRefs was to avoid having to manage scenarios where a Wave Pod might get terminated mid-reconcile and lose internal state or a scenario where two Wave Pods might be running simultaneously in a degraded cluster. Handling these cases gracefully requires external state.

By offloading this to the kube API, we got these cases handled atomically for free.

I'm not saying this is a bad change, but I did rant to chime in with thoughts.

Since we reconcile everything on startup and there is only one active controller at a time (due to master election) this should be perfectly fine. Basically what happens on startup is:

There is no need to store any state as we can rebuild it on startup. Theoretically two controllers could even run at the same time as they would generate the same hashes. However, kubebuilder uses master election and will ensure that only one controller runs.

For webhooks (in the other PR) we got multiple active controllers but the watch logic only happens in the controller/reconciler part. Webhooks really only change the object on the fly and Kubernetes routes the request to one of them.