Thanks for your submission. We have indeed more evidence the original fix is incomplete, but I have reason to believe the actual problem is in the first part of this guard:

In other words, I think we are misusing m.sourcesReady.AllReady() because AllReady() actually turns true when the sources are all connected, not when they have processed all the pods. We assumed the latter, turns out is actually the former (pending final verification). This explains why we see the failure happen not at every restart: we still have a race, albeit less likely (hopefully less likely?)

My initial thought is we can probably just write

	if m.isContainerAlreadyRunning(podUID, contName) {
		klog.V(3).InfoS("container detected running, nothing to do", "deviceNumber", needed, "resourceName", resource, "podUID", podUID, "containerName", contName)
		return nil, nil
	}

but this has to be very carefully validated.

Good idea, it seems that both AllReady and isContainerAlreadyRunning may have problems.

From my logs, it appears that I entered the isContainerAlreadyRunning function twice and found different results.

I0731 16:58:54.195904  909172 manager.go:1100] "container found in the initial set, assumed running" podUID="58cf408f-5297-4209-96a0-f2c367392151" containerName="app" containerID="f393bcca6c8028c74f8987165759a472b3085f4bdf26173eb3dbb4cbe1f6cc9d"
I0731 16:58:54.195953  909172 manager.go:1095] "container not present in the initial running set" podUID="58cf408f-5297-4209-96a0-f2c367392151" containerName="app" containerID="491ae36456e71a83a6c39270e364e85306f583f959b386660262898086462374"

From the #133382 perspective, it seems that AllReady also has problems, and I will also investigate it.

I agree isContainerAlreadyRunning can have bugs. The intention to use it was and is:

1. create the initial set at startup, once
2. check for presence during the allocation flow
   in other words, the set is supposed to be created onceusing the data from the container runtime and never mutated again, which greatly reduces the chance to introduce bugs.

In your observation, which flow can make the container running set inconsistent?

Ah, I see. There could be an inconsistency between containerMap and containerRunningSet, this is also what you mentioned in the commit message. Am I right?
Still is not very clear to me how the new code is more robust: it seems an equivalent rewrite with some different pros and cons.
The original code was written trying to be as defensive as possible and reuse as much as possible the existing data we collect anyway in kubelet. The intention was to make a minimal, as safe as possible incremental change because this flow (kubelet restart) is ancient, rarely touched and hard to test. The fact the code is hard to test is unfortunate and we should eventually rectify this.
It's possible the original intent backfired and led to suboptimal, still buggy implementation.

Yes, you are right. There is an inconsistency between map and set. The map contains containers that have been stopped, which causes the same containerName to have two containerID.