-
Notifications
You must be signed in to change notification settings - Fork 329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vsock crash #50
Comments
Hi @tailhook & thanks for your report. Sorry for the delay replying, I've been AFK travelling. The assertion is checking whether the reply ring is full, I'm reasonably confident the check is correct, although as the comment indicates a second pair of eyes wouldn't go amis ;-). However this is slightly moot in light of seeing the assertion actually fire, even if the check were wrong it would certainly only be an off-by-one, which would indicate that either the ring had but a single slot left or had overflowed by one slot (the latter is obviously worse, but as I'll explain still a bit moot). The issue is that if the reply ring has filled up (+/-1 if the check is wrong, but one slot either way isn't going to save us or change what follows) then due to a protocol bug in the vsock protocol spec there is actually nothing we can do other than crash or deadlock. The assert turns the deadlock into a crash, which is obviously not ideal either but better than some other component silently locking up. There is a description of the deadlock in 1ea0301 which is where the reply ring was introduced (previously the deadlock was much more obvious, there are also some subsequent followup fixes but that is the main one). This all stems from the assertion in the design that vsock is reliable and therefore cannot drop anything, while also not including any back pressure mechanisms, while also including cross talk between the rx and tx rings (i.e. a tx request can result in a requirement to have a free rx slot). I raised this deadlock upstream a while back and had a discussion with the guys doing the protocol specification (as well as the Linux frontend implementation) and they were going to look at remedying it in the next version. I see in my inbox this morning that I have a new set of frontend patches (RFC v6) and a new version of the specification proposal (v7, nb the spec proposal and the frontend implementation are independently numbered, but v6 of the patches corresponds to v7 of the spec) and a changelog which specifically mentions this issue, so hopefully something has changed (or been more tightly specified) which will resolve this issue at root. That's going to take a little time to digest and to make the necessary updates on both ends. In the meantime increasing If you could give details of your repro case (ideally in the form of a |
It's hard to do. We have a huge web project in python. We run The python process is a web application with a pretty large HTML pages (~10Mb). The crash usually happens when we refresh a page in a webbrowser. In fact when we use minified version of page (which is about 200Kb, i.e. 50x smaller) the failure is more rare. So it probably depends on amount of traffic between host system and containers. Each web page visit also fetches data from a database that is outside of this physical box. Also, each web page touches session file which is put on shared volume. And it may happen (don't know how to verify it) that some inotify watches may follow that file changes for whatever reason we misconfigured the watchers. This is the story. I can't publish all of our sources. But if you'll give me some hints which exact operations influence the ringbuf usage, we can come up with some smaller script to reproduce the issue: do we need filesystem operations? are network connections or packets fill the ringbuf? what about inotify? |
This is very well said. My project is in a similar situation. While I open source all my personal projects, the one this bug affects is at my job, so source code is difficult to share. I attempted to start an nginx image that loads an HTML page returning many large images and wasn't able to reproduce the bug. The workaround I used until I discovered I could increase the reply rings size was just disabling images. I agree that knowing exactly what causes reply rings to increment would definitely help me to upload a testcase. @ijc25 Thank you for your detailed comment! Reading it was very interesting after looking at the source code myself. I appreciate a developer that explains how an issue is occurring so the rest of us could learn. |
@ijc any phrase like "fix [issue ref]" will close an issue even if preceded by "not". :-P |
I'm seeing a similar crash:
Unlike @tailhook, I was running a single container without any volumes mounted. |
Experiencing the same issue here. Using docker-compose with multiple containers. Only occurs with a large amount of i/o. |
I'm seeing the same crash as @kudos with the latest beta, running two containers (portal and a DB) with the code mounted into the container.
|
… of rx v7 of the virtio vsock spec proposal adds a requirement to continue processing the TX ring so long as "additional resources" are available in order to avoid deadlock. In this implementation those additional resources take to form of the reply_ring. In order to avoid deadlock we must ensure that we process the reply ring in preference to actual data RX frequently enough that we avoid filling the reply ring too. Since the reply ring is 2x the virtio ring size I _think_ it should be sufficient to ensure that we check for new reply entries periodically (something less than the virtio ring size) rather than every time. This is discussed in several replies to the spec posting at <1470324277-19300-1-git-send-email-stefanha@redhat.com>. A more formal consideration of how deadlock is avoided using this scheme should be forthcoming (see the above thread). Worst case is this change doesn't avoid deadlock, it can't make things any worse. This should hopefully address moby#50. Signed-off-by: Ian Campbell <ian.campbell@docker.com>
I've merged the changes in #59 and the new Linux frontend patches into Docker for Mac's master branch so they will be present in the next beta, which will be beta27 due (hopefully) next week (but possibly as long as the week after depending on other bugs etc). I think these changes will fix this issue so I am going to close it now while it is on my mind. Once beta27 is released (if you install beta26 now you should get it via autoupdate) please do retest and file a bug against https://github.com/docker/for-mac (via the "Diagnose & Feedback" entry in the whale menu) if the issue persists or recurs in a different form. Thanks for you report(s) and your patience. Ian. |
@ijc25 - nice work. I have been testing today and I'm not experiencing the implosion of Docker I was getting before with heavy i/o 👍 |
@telecoda the changes referred to here are not yet in any released version of docker for mac (they will be in beta27 when it is released). So I don't know what change has fixed things for you, but I'm glad everything is ok! |
@ijc25 sorry, I was a little premature with my congrats. That'll explain why I still have the same issue.. |
Now that beta27 landed - looks like I'm still getting some sort of error in this class, here are some |
@bryanhelmig Yes, that does look rather similar :-(. Please could you open a fresh ticket against https://github.com/docker/for-mac with the full output of diagnostics. Any sort of reproduction case would also be useful in tracking this down. I understand that many of the repro cases are when building proprietary software but if someone was able to par their case down to just the basic build system skeleton required to trigger the issue (removing all the proprietary stuff) or find a subset of their code which they were willing/able to post then we'll stand a much better chance of nailing this once and for all. |
I will try and set up a build that does the following things we're doing:
I will see if we can reproduce it. Would a github repo with a docker-compose.yml be sufficient? |
In a similar configuration (also django with django-static-precompiler and less) we encountered similar problems. However these turned out to be extremly difficult to replicate in a more minimalistic setup. We were able to get arround this by re-enabling sendfile in our uwsig config which we had previously disabled in our dev setup to fix a bug in an older version of docker for mac. |
@mpauly curious to hear a bit about your uwsgi + dev server setup. |
@ijc25 Hey Ian - I have a reproducible sample project here https://github.com/bryanhelmig/docker-compose-crash-repro. It should be as simple as cloning, building and running the container to repro. You can tweak up/down the pressure as needed. Can you take a peek and let me know if it happens for you? Happy to go further - this is a massive pain point for us now. |
Unfortunately I was a bit early on this - even after enabling sendfile the bug reoccured, only less frequently. I also created a sample project to reproduce https://github.com/mpauly/dockerbug. It looks fairly similar to what @bryanhelmig did. |
@ijc25 thanks from us as well - looking forward to giving beta28 a spin. |
First pass with beta28 and it looks 👍 . If you don't see me complain/link to a new issue, this is resolved in the wild for us! |
Same here, beta28 solves the problem. Thanks! |
Hi,
We have an annoying crash of docker for mac which boils down to invalid ringbuf check here AFAIU.
Log:
As far as I can see the error is here:
I.e. someone should "XXX correct check" :)
Alternatively, if it's hard to fix could we make
VTSOCK_REPLYRINGSZ
bigger (if it will help to make error more rare)?The text was updated successfully, but these errors were encountered: