Use signal pipe to make live reload better. #1843
Conversation
|
retest this please |
| return | ||
| } | ||
| log.Infof("Received message from pid %v: %v", p.Pid, string(data[:len])) | ||
| cancel() |
There was a problem hiding this comment.
cancel() is called twice.
| log.Debugf("Failed to write to pipe: %v", trace.DebugReport(err)) | ||
| return | ||
| } | ||
| cancel() |
There was a problem hiding this comment.
it's on purpose to ensure all code paths are covered, cancel function is re-entrable
lib/service/signals.go
Outdated
| case <-time.After(signalPipeTimeout): | ||
| return trace.BadParameter("Failed to write to parent process pipe") | ||
| case <-messageSent.Done(): | ||
| log.Infof("Signalled success to parent process") |
There was a problem hiding this comment.
Signaled. Might want to use structured logging like below as well.
lib/service/signals.go
Outdated
| if !trace.IsNotFound(err) { | ||
| return trace.Wrap(err) | ||
| } | ||
| log.Debugf("No signal pipe found, not going to signal") |
There was a problem hiding this comment.
Maybe change message to, "No signal pipe to import, must be first Teleport process."
lib/service/signals.go
Outdated
| } | ||
| defer signalPipe.Close() | ||
|
|
||
| messageSent, cancel := context.WithCancel(context.TODO()) |
There was a problem hiding this comment.
Since in this case context.TODO() will never be replaced (it's only used to signal when the write is complete), I'd use context.Background(). That way it won't highlight TODO in editors.
lib/service/service.go
Outdated
| @@ -441,6 +431,20 @@ func NewTeleport(cfg *Config) (*TeleportProcess, error) { | |||
| return nil, trace.BadParameter("all services failed to start") | |||
| } | |||
|
|
|||
| if err := process.writeToSignalPipe(fmt.Sprintf("Process %v has started.", os.Getpid())); err != nil { | |||
| log.Warningf("Failed to write to signal pipe: %v", trace.DebugReport(err)) | |||
There was a problem hiding this comment.
Seems like an error should be returned here? Is it okay to fail writing to the pipe?
There was a problem hiding this comment.
yes, it's ok, as it's not critical for process, it could mean that parent process has crashed. I will add comment.
This commit allows teleport parent process to track the status of the forked child process using os.Pipe. Child process signals success to parent process by writing to Pipe. This allows HUP and USR2 to be more intelligent as they can now detect the failure or success of the process.
|
retest this please |
This commit allows teleport parent process to track
the status of the forked child process using os.Pipe.
Child process signals success to parent process by writing
to Pipe.
This allows HUP and USR2 to be more intelligent as they
can now detect the failure or success of the process.
fixes #1844