-
Notifications
You must be signed in to change notification settings - Fork 0
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 /run
as the default for PID/UNIX-domain sockets
#13
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
==========================================
- Coverage 90.12% 89.61% -0.52%
==========================================
Files 4 4
Lines 81 77 -4
==========================================
- Hits 73 69 -4
Misses 8 8 ☔ View full report in Codecov by Sentry. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.socket") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could mock this call to the IPC dir for testing purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also possibly use mocking for the external command. Will look into that quickly.
@@ -12,6 +14,7 @@ | |||
end | |||
|
|||
cmd = `$(Base.julia_cmd()) --color=no -e $code` | |||
cmd = addenv(cmd, "DEPUTY_IPC_DIR" => deputy_ipc_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah maybe not if we are using an external command...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the very least it seems that it woudl be annoying lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely annoying. I'll look into replacing DEPUTY_IPC_DIR
with Mocking as a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah let's not hold this up
While reviewing #12 I needed to review when UNIX-domain sockets or named pipes were created. The answer seems to be UNIX-domain sockets are created on Linux and macOS and only Windows creates named pipes. I also reviewed the filesystem hierarchy standard (FHS) which states that
/run
should store both PID files and transient UNIX-domain sockets and realized that the/run
directory could be made writable on read-only file systems with mounts which eliminated the need forDEPUTY_IPC_DIR
.Unfortunately, in order to allow for local testing of K8sDeputy.jl we need to keep
DEPUTY_IPC_DIR
around as normally/run
is not writable by a normal user.Supersedes #12.