-
Notifications
You must be signed in to change notification settings - Fork 273
implement readiness notification #767
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
Conversation
N-R-K
left a comment
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.
Just a cursory glance. Didn't test it out.
src/shared/misc.c
Outdated
| eerrorx("%s: invalid ready '%s'.", applet, optarg); | ||
|
|
||
| ready.type = READY_FD; | ||
| pipe(ready.pipe); |
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.
Return value needs checking here, it may fail.
Same with some of the dup2 calls.
Adds two helper functions, and allows scripts to wait until services are ready before proceeding with it's dependees by setting `ready=fd:<num>`, where <num> is the file descriptor the daemon should write a new line to.
|
|
||
| if (ready.type == READY_FD) { | ||
| close(ready.pipe[0]); | ||
| dup2(ready.pipe[1], ready.fd); |
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.
This should be checked.
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.
those don't really matter at this point. if they fail, the daemon can't write the \n, so the supervisor will already be waiting on ready until openrc-run times out and kill it
the best we can do is check and log, if we exit same thing happens, supervisor will start waiting until timeout and s-s-d won't be doing healthchecks
i'd argue for s-s-d it's better to let the exec go through, since then we have a chance of the daemon writing it's pid file so the timeout can kill it cleanly. for s-d that doesn't matter
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.
if they fail, the daemon can't write the \n
Not necessarily. Let's say the ready fd is supposed to be 3 and the dup2 fails. And in the daemon there's code like this:
somefd = open("somefile", ...); // this gets fd 3 since it's available
// ...
write(3, "ready\n", 6); // this now writes into "somefile"Now you end up with the deamon potentially writing nonsense elsewhere.
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.
good point, i'll add the checks then
| cloexec_fds_from(3); | ||
|
|
||
| if (ready.type == READY_FD) | ||
| dup2(ready.pipe[1], ready.fd); |
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.
Same here.
No description provided.