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

socket: try removing if socket-location is a directory on Linux #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 28, 2020

Alternative for #72. Relates to moby/moby#41392

Due to race-conditions between containers starting and the Docker remote API being up, containers bind-mounting the docker-socket may cause the socket-path to be created as a directory.

This patch will attempt to remove the directory in such situations. Removing will fail if the directory is not empty.

@thaJeztah
Copy link
Member Author

Oops, didn't stage some files and pushed the wrong version; fixing now

@thaJeztah
Copy link
Member Author

done 😅

return nil, err
// Using syscall.Unlink(), not os.Remove() to prevent deleting the socket if it's in use
if err := syscall.Unlink(path); err != nil && !os.IsNotExist(err) {
if err != syscall.EISDIR {
Copy link
Contributor

Choose a reason for hiding this comment

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

OS X and Linux differ on whether unlink(dir)
returns EISDIR,

https://golang.org/src/os/file_unix.go#L289

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, interesting. I guess it's still ok to retry on Linux only then? Added a comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Also added a test

@thaJeztah thaJeztah force-pushed the retry_remove_socket branch 2 times, most recently from 111e345 to 19cabb7 Compare September 14, 2020 10:18
@thaJeztah thaJeztah changed the title socket: try removing is socket-location is a directory socket: try removing if socket-location is a directory on Linux Sep 14, 2020
@thaJeztah thaJeztah marked this pull request as ready for review September 14, 2020 10:19
@thaJeztah thaJeztah force-pushed the retry_remove_socket branch 2 times, most recently from afda0fa to 56ccd56 Compare September 14, 2020 10:56
@thaJeztah thaJeztah force-pushed the retry_remove_socket branch 3 times, most recently from 05e56ab to 52be987 Compare March 15, 2021 12:42
Due to race-conditions between containers starting and the Docker
remote API being up, containers bind-mounting the docker-socket may
cause the socket-path to be created as a directory.

This patch will attempt to remove the directory in such situations.
Removing will fail if the directory is not empty.

MacOS does not allow us to detect that the path is a directory, and we'll
return immediately instead of retrying.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

macOS is failing due to the tempdir-location and/or permissions of it; I'll have to look at that; not a "real" failure, just have to look at how to make the test work

    --- FAIL: TestUnixSocketConflictDirectory/conflicting_file (0.00s)
        unix_socket_test.go:30: dial unix /var/folders/3s/vfzpb5r51gs6y328rmlgzm7c0000gn/T/TestUnixSocketConflictDirectory549534108/test2.sock: connect: permission 

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.

None yet

2 participants