Skip to content
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

Remove aufs debugEBusy() #31665

Merged
merged 1 commit into from
Mar 10, 2017
Merged

Remove aufs debugEBusy() #31665

merged 1 commit into from
Mar 10, 2017

Conversation

mlaventure
Copy link
Contributor

@mlaventure mlaventure commented Mar 8, 2017

Since it was introduced no reports were made and lsof seems to cause
issues on some systems.

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

--

Closes: #30782

@tonistiigi
Copy link
Member

@anusha-ragunathan Have we got any useful reports with aufs debug data since this was added? Maybe we should just remove it?

Change LGTM but even with the extra goroutine this blocks for 500ms and keeps anything else from mounting. We should only take ID based locks for the long running processes.

@anusha-ragunathan
Copy link
Contributor

@tonistiigi : No reports with the added debugEbusy so far.

@mlaventure
Copy link
Contributor Author

Should I just update the PR to remove it altogether then?

@estesp
Copy link
Contributor

estesp commented Mar 9, 2017

If not being used, seems reasonable to just PR remove it and skip the added complexity of the goroutine.

@anusha-ragunathan
Copy link
Contributor

@mlaventure : SGTM

Since it was introduced no reports were made and lsof seems to cause
issues on some systems.

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure
Copy link
Contributor Author

Updated PR to remove debugEBusy()

@mlaventure mlaventure changed the title Run aufs debugEBusy() out-of-band with a timeount Remove aufs debugEBusy() Mar 9, 2017
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member

Not sure it's related; failure on experimental;

23:48:02 FAIL: docker_cli_plugins_test.go:302: DockerTrustSuite.TestPluginTrustedInstall
23:48:02 
23:48:02 docker_cli_plugins_test.go:320:
23:48:02     c.Assert(err, checker.IsNil)
23:48:02 ... value *errors.errorString = &errors.errorString{s:"\nCommand: /go/src/github.com/docker/docker/bundles/17.04.0-dev/binary-client/docker plugin enable 127.0.0.1:5000/dockercli/trusted-plugin-install:latest\nExitCode: 1, Error: exit status 1\nStdout: \nStderr: Error response from daemon: error setting up propagated mount dir: invalid argument\n\n\nFailures:\nExitCode was 1 expected 0\nExpected no error\n"} ("\nCommand: /go/src/github.com/docker/docker/bundles/17.04.0-dev/binary-client/docker plugin enable 127.0.0.1:5000/dockercli/trusted-plugin-install:latest\nExitCode: 1, Error: exit status 1\nStdout: \nStderr: Error response from daemon: error setting up propagated mount dir: invalid argument\n\n\nFailures:\nExitCode was 1 expected 0\nExpected no error\n")

@anusha-ragunathan
Copy link
Contributor

Failure is unrelated. That test seems to be flaky on janky/experimental. #31724

@anusha-ragunathan
Copy link
Contributor

LGTM

@cpuguy83 cpuguy83 merged commit 2c2983c into moby:master Mar 10, 2017
@mlaventure mlaventure deleted the aufs-lsof-oob branch March 10, 2017 18:31
@allencloud
Copy link
Contributor

Thanks for your guys to fix this.
Actually we are facing the issue some while. In addition, I think this change could ease the pain, and this change could also be applied to old versions of docker engine.
Therefore, I suggest that we could back port this change into docker-ee 17.03. Then that action definitely brings much benefits for users.

@thaJeztah
Copy link
Member

@allencloud this was already being considered for back porting 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants