I don't think this solves the problem because the snapshotting of the containers themselves and the name index are done separately, with no guarantee that they return consistent results. To do this properly, we would have to add a name index to the memdb and rely on its atomic snapshotting. This would involve changing the type of data we store in the memdb from Container to a wrapper that contains a Container and a names field, and updating the names in the memdb whenever a change is made to the Registrar.

(...or even better, remove Registrar completely and use the memdb as the authoritative source of naming information instead).

I agree that would be a better design, but also a bigger change.

You are right that the snapshotting of containers and nameIndex happens separately, but they are very close: https://github.com/moby/moby/blob/master/container/view.go#L88-L89

Maybe slightly extending the Registrar.mu.Lock() and doing the memdb snapshot while holding that lock is a simpler fix?

... in that case we should also deep copy the []string slices in (*Registrar).GetAll()

I changed the commit message to indicate that this is not a fix, only a slight improvement, since we shouldn't be calling daemon.nameIndex.GetAll() again in (*Daemon).foldFilter and just use what the snapshot has.

It seems like we're in agreement that pulling this information in the memdb is a better design.

I agree that holding a lock around the snapshotting process could potentially work, but it would require expensive deep copies, and negate part of the benefit of this new lock-free code path.

Unless there is a problem with using an out-of-date nameindex, I'd prefer to either leave this as-is or move everything into the memdb when we find time.

BTW, it seems really broken that GetAll is returning slices that can be modified later on. It looks inherently racy. Seems like an argument for replacing it with an index on the memdb.

Yep, I agreed with you at #33863 (comment)

(sorry for spreading the discussion onto multiple threads)

I'm going to try moving the replacing the registrar stuff with an index on the memdb.

I'll open a PR, either tonight or sometime next week.

does that mean we should close this PR? Thanks for thinking along btw, @fabiokung, much appreciated

I've opened #33886. It still needs testing, and possibly additional changes to make adding containers/names transactional, but I think the concept is sound.

I think we can close this one if people are happy with the direction #33886 is going.

Let's reopen as needed.