Get rid of lock during list containers

[mshaverdo]

Sometimes, when reading of cgroups tree is too slow, manager.getContainersDiff during global housekeeping could block any other actions due to acquiring mmanager.containersLock for a long time.

To fix it, we could release lock while list cgroups tree and then acquire it again to get diff.

[dashpole]

/ok-to-test

[dashpole]

The following files are not properly formatted:
manager/manager.go

[mshaverdo]

@dashpole
Fixed. Sorry.

[dashpole]

Thanks for this! Looks good with one nit.

[dashpole]

nit: rather than locking for all but the ListContainers call, can we instead lock around just the critical sections? I.E. lock and unlock around the := m.containers call above, and then Lock, and defer Unlock before the Determine which were added... section

[mshaverdo]

You're right, it's looks much better

[dashpole]

do we still need to assign cont.handler to a new variable here?

[mshaverdo]

I think, it's better to leave assignation to a new var, but move it under the lock, to avoid possible data race.

[dashpole]

ok

[mshaverdo]

With assignation placed under the lock, code looks ugly without defer, but with defer we returning to the first variant. Maybe, it's better to use initial variant?

	m.containersLock.RLock()
	cont, ok := m.containers[namespacedContainerName{
		Name: containerName,
	}]
	if !ok {
		m.containersLock.RUnlock()
		return nil, nil, fmt.Errorf("failed to find container %q while checking for new containers", containerName)
	}
	contHandler := cont.handler
	m.containersLock.RUnlock()

[dashpole]

I am not convinced there is a data race in reading the handler attribute of cont. It is not modified once it is initialized. The containersLock is meant to prevent concurrent access to the containers map, which the current design does. I don't think you need to add a new contHandler variable here.

[mshaverdo]

Yes, there are no data races in the current codebase, but i have one concern:
When m.containersLock locked, we expect exclusive access to the m.containers and it's members' fields. It's unexpected, that cont.handler could be read by anyone else during lock, so it could lead to tricky data race in future.
What do you think about it?

[dashpole]

I think we will need to reassess that when the time comes. Other functions, including getContainer and getSubcontainers, getAllDockerContainers, etc. return references to containerData structs, and could allow concurrent modification of containerData objects, if used in the way you are suggesting it could be. We would need much more than a change here to fix it.

[mshaverdo]

Got it. Fixed.

[dashpole]

lgtm