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

use tempdir for graceful terminator socket when env not set #12

Closed
wants to merge 3 commits into from

Conversation

kleinschmidt
Copy link
Member

This makes the socket path more robust to images where the workdir is set to
something that's read-only.

@kleinschmidt kleinschmidt requested a review from omus August 7, 2024 16:22
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.00%. Comparing base (973acac) to head (85e00d2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
- Coverage   90.12%   90.00%   -0.13%     
==========================================
  Files           4        4              
  Lines          81       80       -1     
==========================================
- Hits           73       72       -1     
  Misses          8        8              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -14,8 +14,7 @@ _deputy_ipc_dir() = get(tempdir, ENV, "DEPUTY_IPC_DIR")
# Prefer using UNIX domain sockets but if the `DEPUTY_IPC_DIR` is set assume the file
# system is read-only and use a named pipe instead.
function _graceful_terminator_socket_path(pid::Int32)
name = "graceful-terminator.$pid"
return haskey(ENV, "DEPUTY_IPC_DIR") ? joinpath(_deputy_ipc_dir(), name) : name
return joinpath(_deputy_ipc_dir(), "graceful-terminator.$pid")
Copy link
Member

Choose a reason for hiding this comment

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

This now makes us always use named pipes. Not specifying a path should use a UNIX domain socket which don't necessary require. I'll want to run a couple of checks first.

Copy link
Member Author

Choose a reason for hiding this comment

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

what's the issue with a named pipe vs. domain socket?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/35424970/unix-socket-permissions-linux

In the Linux implementation, sockets which are visible in the filesystem honor the per‐ missions of the directory they are in. Their owner, group and their permissions can be changed. Creation of a new socket will fail if the process does not have write and search (execute) permission on the directory the socket is created in.

Copy link
Member

Choose a reason for hiding this comment

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

I thought if you specified just a name in listen that a UNIX-domain socket would be created in /run automatically. It turns out that switch between UNIX-domain sockets and named pipes is a libuv thing: https://docs.libuv.org/en/v1.x/guide/processes.html#new-stdio-pipes.

I think this change is good to be consistent on where we write these files. It also seems that we should be using /run more for both PID files and UNIX-domain sockets so I opened #13 to handle these changes.

@omus
Copy link
Member

omus commented Aug 7, 2024

Replaced by #13

@omus omus closed this Aug 7, 2024
@omus omus deleted the dfk/tmpdir-socket branch August 7, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants