Fix possible race condition in debounceCounts

[JonCrowther]

Issue: rancher/rancher#43515

Some users are experiencing race conditions where apiserver causes a fatal error while marshalling an APIObject. It causes the Cluster Agent to restart.

Based on the investigation in rancher/rancher#43515, they identified the source as the Count type. Investigating the code, the debounceCounts function sends currentCount as an APIEvent every 5 seconds (via debounceDuration). However, when the count is modified (added or removed resource), it updates the map currentCount.Counts. I believe that if the JSON marshalling in APIServer takes long enough, debounceCounter will start modifying that map while it's being written, resulting in fatal error: concurrent map iteration and map write. To remedy this, I will be passing in a copied version of currentCount instead of passing it directly.

This change is completely theoretical. I can't reproduce the race condition, and I believe it requires a large list of resources that causes the JSON marshalling to take a long enough time for the count to change while it's marshalling.

[puffitos]

We'll test this tomorrow in our fork of steve and get back to you in a day or so after that (the crash typically happens once per day).

[moio]

I do not have a problem approving this once empirical evidence shows it helps. I'm keeping notifications for this PR and the issues and will change to approve as soon as that is confirmed.

[JonCrowther]

So after some trial and error, I found a way to consistently reproduce the panic locally. To do so, I did the following:

• Reduce the debounceDuration to 5 milliseconds so it occurs more frequently
• Open a page that has the necessary websocket to get count events. I chose the cluster page where it displays object count
• I created a bash script that created configmaps on a loop and ran 4 instances of the script

It takes different amounts of time, but after around 6-10k configmaps I get the concurrent map iteration and map write consistently.

---

I tested the original changes I made for this PR where I attempted to do DeepCopy the count when it is sent to apiEvent

steve/pkg/resources/counts/buffer.go

Line 52 in 7913f27

result <- toAPIEvent(*currentCount)

This was still causing a panic, but now it was causing a panic during the DeepCopy, specifically during the DeepCopy of ItemCount.Namespaces. After some more investigating, I realized that the issue was that we were passing ItemCount directly into the Count channel we pass to debounceCounts.

steve/pkg/resources/counts/counts.go

Line 184 in 7913f27

schema.ID: itemCount,

I believe what happens is that we register multiple Watch for the Counts schema (I'm assuming 1 per websocket?) and when the OnChange function is called, it passes a pointer to the same ItemCount to 2 different channels. Then one edits the ItemCount.Namespaces map while the other is iterating through it.

My solution is to create a copy of ItemCount and pass that to the channel, as opposed to passing a pointer to the original object. That way each channel receives a completely separate object that it can modify without negatively impacting other channels. I've run this 5-10 times and created 40k+ configmaps without getting any error, whereas before I consistently got a panic after ~7k configmaps.

[moio]

Approved.

#162 (comment)

sounds convincing enough to me