[bug]: Resource watcher does not reconnect #585
Comments
|
Tried to debug this in a locally running operator ... sadly the issue only happens when the operator runs inside the cluster :( |
|
From what I can see in the apiserver log, all watches terminate and then not all of them are reconnecting. It seems to me that this is a bug in the KubernetesClient watch logic. Any thoughts ? |
|
I've further debugged the issue by placing additional log messages into KubeOps ... _logger.LogTrace(
@"Processed watch event ""{eventType}"" for ""{kind}/{name}"".",
type,
resource.Kind,
resource.Metadata.Name);The log message shows that for all resource types that stop working this message is never printed - |
|
I'm a bit curious about the WatcherHttpTimeout default setting of one minute. Is there a reason for this being so low? |
|
Hey @jhested If I recall correctly, that was the main reason back in the day when I wrote the thing. |
|
@buehler Thank you for your fast reply. The reason I'm asking is because we would like our resource to be reconciled every 20 min. But every time the watcher is recreated, it will trigger the 'Added' event for each resource that it is watching, causing ReconcileAsync() to be called every minute, instead of the desired RequeueAfter delay. Is the a better approach to handling this in our own code? |
|
@buehler with the mentioned fix it works well even if I use watcher http timeout of 1 hour. In my opinion you dont need to explicitly specify a timeout, the API Server will close the connection anyways. |
|
Hey @jhested In theory, your code should be totally fine with being called each minute. The manifest states the desired state and the operator should take actions to reach that state. So, if another reconcile is called, there should be no actions to take. |
|
I'm just wondering why the ongoing discussion is not about the bug 🤐 |
|
@prochnowc you are right :-) Would you mind opening a PR? |
|
@buehler @prochnowc Im testing #554 suggested change (replacing Switch with Merge) on my prod-like cluster and can validate already that it fixes the timed reconcile issue when status is updated. This invalidates, or at least provides a working solution, my thoughts explained on my related issue #579 Ill let you know if I observe any side issue |
|
Thank you :-) I'm currently restructuring this repository, and I have a big refactoring for 8.0 going on. I really cannot get my head around if it should be possible to "re queue" an event since it actually is an anti pattern too. |
|
I dont get your point of anti-pattern related to the "requeue" event thing. its true as you have highlighted above that the Old Created/Updated/etc... events could be considered as anti-pattern somehow. However, in this specific case we are just talking about the ability to schedule reconcile event to execute on an specified interval, and this does not get out of the pattern. You have stated this before in the doc of the project, as example, ensuring the state of a database could require to execute the reconcile event in a period smaller than the default of 5 minutes. PD: Even though this is not the place, I want to express my excitement about what you bring us on v8 |
|
Going back to the Issue. I conclude the root error is not here. As mentioned, default reconcile execution occurs every 5 minutes. After applying the fix we are talking about, it is true that reconcile get enqueued within the intended interval, on the opposite, an additional reconcile keeps being fired every 5 minutes. Therefore, over time, you end up with multiple reconciles getting accumulated, u can see a log trace below (at time 09_22_10_21_35 and 09_22_10_26_42 a new reconcile was trigger). |
|
Hi @buehler, I kept investigating and got down to whats going on here. As it has been stated, timed requeue reconcile is broken on StatusUpdated. This is happening whenever the Status is updated right before the end of the Reconcile event, as of today probably most of us do, in such case only the last Event, WatcherEvent or LocalEvent, reaching the EventQueue will be kept. In most cases is the ResourceWatcher StatusModified Event reaching the last, and overriding the timed reconcile. Replacing Switch with Merge provides a solution to this override with the side effect of unlimited accumulated events I mentioned on my previous entry. I want to apologise due to the missleading, there is no default of 5 minutes, what I was reaching was the ResourceWatcher timeout, that I've configured to 5 minutes, whereas the default is 1 minute. Once the ResourceWatcher is restarted after the timeout it will trigger automatically an Added event. In my opinion this is pointless and undesired for any other case other than on startup, even if after a reconcile you returned null and no requeue, why would you want your resource to be reconciled again after the watcher timeout? Therefore, since the watcher events are not finally "added" to Observable list in EventQueue until they are mapped at MapToResourceEvent, where we have access to cache. My suggestion is to always drop any watcher event of type added whenever we already have a TEntity cached with the same Uid. This two changes should help us keep both events, StatusModified and timed reconcile, as well as remove the side effect. Please let me know if you agree with this, and if you want I can submit a PR. For the time being, introducing a small Sleep after updating status will a better chance to your TimedReconcile to win over StatusModified (of course this is not guarantied) |
|
@Crespalvaro Nice investigation. Just wanted to note that the correct way ist to compare the resource version of the resources been notifiied. |
|
@prochnowc thanks for your feedback. As I understand the code flow, these Added event should just not occur, they are automatically triggered on watcher recreation, but in fact there's no new added resource. There is actually no chance for a resource to be newly added with an uid already on the cache since of course k8s wont create 2 different resource entities with same uid. The only scenario in which we should be worried, is where the resource gets updated meanwhile the watcher is being recreated, i.e. by manual applied changes. In such case we would miss any event the watcher would normally catch. However, this sounds very unlikely to happen. Furthermore, if this is still a concern, we have access to ResourceCache{TEntity}.CompareCache comparing resource versions and outputs as result CacheComparisonResult.Other whenever there are no changes to the Spec nor Status of the resource. Said so we could go the safe way dropping Added events if uid exist in cache and CacheComparisonResult.Other. I would still vote for the simple check of uid. |
|
This definitely makes sense. If there is a quick-fix I'd like you to make a PR into the release branch. The new main branch is used by v8 until all the refactoring is done. In v8 I try to make everything simpler (and therefore faster as well) while still keeping the features. |
|
@Crespalvaro The "added" events are sent when the watch is started with the parameter "sendInitalEvents:true". The Kubernetes API docs states that the correct/efficient way to watch for changes is to do a list and then start the watch with the resourceVersion of the list which was returned. This has to be done everytime the watcher has been disconnected. This would make sure that no change is lost. See https://kubernetes.io/docs/reference/using-api/api-concepts/#efficient-detection-of-changes |
|
@prochnowc wow nice contribution, we could actually make use of this as well. Currently, we are just re-opening watcher without resource version, I can give it a try to send cached versions so we wont miss any event as described on the doc you posted. |
|
@buehler Im about to push changes to a fix branch created from "release" branch but Im missing rights, could you fix them out? |
|
Hey @Crespalvaro |
|
There you go @buehler, thanks for the guideline though, first time push to open source to be honest |
Watcher and Local events are now merged to produce a sequence containing all captured events instead of just the sequence that was last produced. Events will still be switch but only at group level, grouping by resource Uid and sub-grouping by event type, therefore Keeping always last event for each type of each resources, preventing events accumulation. Fixes #585 #579
|
This should work in v8 now |
Describe the bug
My operator implements 4 CRs.
Everything is working normal after startup and I can see log messages like this for each CRD:
After a few minutes uptime some instances of the ResourceWatcher do not any longer receive the connection closed event.
Therefore the connection is not re-established and my operator does not receive any further events for the CRs.
Restarting the operator fixes the issue .. but after a few minutes the behavior is the same.
To reproduce
I'm currently unable to reproduce this issue ... will try to provide small repro.
Expected behavior
No response
Screenshots
No response
Additional Context
No response
The text was updated successfully, but these errors were encountered: