Ping @cpuguy83 @tonistiigi 🎉

also related to #28754

Thanks for working on this @fabiokung !

I like the idea. This looks way simpler than the original iteration that involved deep copies.

It looks like daemon.containersReplica.Save(c.Snapshot()) is a common pattern, and it needs to be protected by the container lock to avoid transactions overwriting each other. Maybe it would be good idea to define a method on Container that does this, and add a prominent comment that it should only be called with the lock held (or it could acquire the lock itself, if this pattern works for the call sites).

Good point on moving the replication to Container. I'll take a stab at it.

I'm bit torn about this. I stand by my earlier comment that we shouldn't be afraid of locks, just the wrong implementation of them. This problem doesn't only show in performance, most places we call IsRunning/IsPaused/IsRestarting are clear races. If we had proper synchronization between lifecycle states, a plain RWMutex should probably even perform better than this without the extra maintenance cost. But I don't see anyone taking on this refactor, at least before everything about containerd integration is cleared. This version does look simpler than the earlier one. Not going to block if other maintainers want to go forward with it.

@tonistiigi fair point. This doesn't block going back to locking for reads when/if you are comfortable enough that all concurrency and locking is being properly handled across the codebase. It would be a bit of throwaway work, but at least it fixes the problem we are having today in the short term, until more long term solutions (what you propose with proper synchronization between lifecycle states, or "lock-free" transactional mutations everywhere, ...).

I'm not proposing we lock down on a decision on how concurrency will be handled long term, but this is a big pain right now and I don't feel we can wait much for a big re-architecture to happen.

@aaronlehmann I moved the checkpointing/replication operation to *Container. How does it look now?

I liked it more, since now it is clearer that checkpointing should probably be done when saving state to disk. It also allowed container.snapshot() to not be public.

Thanks, I think that's an improvement.

This needs a rebase, but it looks like some of the requested changes were made, so could use another review as well

Rebased.

I wonder if instead of having this intermediate object, we can store a duplicate of the container object like so:

(warning, pseudo-code follows)

func(c *Container) ToDisk() {
   // normal stuff

 snapshot := &Container{ID: c.ID}
 snapshot.FromDisk()
  c.snapshotsStore.Add(snapshot)
}

@cpuguy83 isn't it what we discussed here: #31273 (comment) ?

Or is it something else I'm missing?

@fabiokung Not the same, this would be the actual *container.Container type which gets stored in the snapshot which is fully-copied by definition since it'll be unmarshalled from the on-disk json.

The dump shows in the daemon logs, not on the command-iine. I'm not sure docker 1.6.2 had this already; possibly it needs debug to be enabled on the daemon

That debugging functionality was added in docker 1.7 #10786

Anyway, this is not really the best location for this discussion - feel free to message me on Slack if you need assistance

I googled docker slack channel before, but no help info, what is the link of docker slack channel? @thaJeztah

Here's more info about the slack channel; https://blog.docker.com/2016/11/introducing-docker-community-directory-docker-community-slack/, register here for the docker community, and to get an invitation for the Slack channel: http://dockr.ly/community

Thanks @thaJeztah , as I cannot be approved to the community, I want to ask you the last question here, hope it is OK. ;-)

Regarding the debug mode, it is really helpful for troubleshooting. The question is: is it good to enable the debug mode in a production env? For me, I do want to enable it, but not sure if there are any overhead for this especially for production.

Got the answer myself, we can turn on the debug mode automatically, cool!

@fabiokung

Container queries (docker ps) currently try to grab a lock on every single container being inspected

One question for the issue, before your fix, why does docker ps need to grab a lock for each single container? Why read-only operations require a lock?

Another issue I want to mention is that when such issue happens, not only docker ps hang, but also docker images hang, does your fix include this? Also, do you know why docker images also hang? Other commands such as docker inspect <container id>, docker logs <container id> works fine.

@gyliu513 it uses locks to prevent partial reads and data corruption, since container data is being concurrently modified. Even in read-only operations, locking is how is ensures the state being read will be consistent.

This fix only applies to docker ps, and was a first step in fixing others you mentioned. I did mention them in the description. docker inspect will also hang, if you happen to hit one of the containers that has its lock held somewhere else.

@fabiokung this fix is available in what docker versions ?

@krnayankk this will be included in docker 17.07 and up

@thaJeztah How about docker-ee? Will we have it in 17.06-ee3 or ee4? Or should we wait until 17.09?

@thiagoalves It will not be in EE until the next major release. I do not see this one getting backported.

@thiagoalves btw, if you have a specific case where you are seeing a deadlock, please report so it can be fixed. This patch is just making the deadlock less apparent, not actually fixing deadlocks.

@mlespiau

17.07 sounds great, when can we expect to see it? I'm only seeing 17.05 with the last commit from May of this year. and oldschool docker 1.13.1 from Feb of this year.

How long do critical fixes affecting an entire ecosystem of projects typically bake before a release is cut?

@robertglen 17.07 was released in July....

@robertglen You probably need to update your apt/yum repositories from dockerproject.org to download.docker.com. https://docs.docker.com/engine/installation/

heh, OK I just went to the code portion of this repo and looked at branches and tags and it stopped at 17.05 and had completely dismissed visiting docker's website :\ my bad.

@robertglen

The Moby project doesn't make releases of this repo, rather Docker Inc makes releases of the Docker Community Edition software (which includes code from this repo). Those releases are tagged in the docker-ce repository.