-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add pid-file flag to agent #128
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,6 +107,8 @@ func Create(config *Config, logOutput io.Writer) (*Agent, error) { | |
return nil, err | ||
} | ||
|
||
agent.storePid() | ||
|
||
return agent, nil | ||
} | ||
|
||
|
@@ -250,6 +252,8 @@ func (a *Agent) Shutdown() error { | |
err = a.client.Shutdown() | ||
} | ||
|
||
a.deletePid() | ||
|
||
a.logger.Println("[INFO] agent: shutdown complete") | ||
a.shutdown = true | ||
close(a.shutdownCh) | ||
|
@@ -496,3 +500,36 @@ func (a *Agent) Stats() map[string]map[string]string { | |
} | ||
return stats | ||
} | ||
|
||
func (a *Agent) storePid() { | ||
pidPath := a.config.PidFile | ||
|
||
if pidPath != "" { | ||
pid := os.Getpid() | ||
pidFile, err := os.OpenFile(pidPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0666) | ||
|
||
if err != nil { | ||
fmt.Errorf("Could not open pid file: %v", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These errors are never sent to stdout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, good point 😃 It seems like the rest of the functions in I'll change it to return an error like everything else in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I would prefer to fail hard. |
||
} | ||
|
||
defer pidFile.Close() | ||
|
||
_, err = pidFile.WriteString(fmt.Sprintf("%d", pid)) | ||
|
||
if err != nil { | ||
fmt.Errorf("Could not write to pid file: %s", err) | ||
} | ||
} | ||
} | ||
|
||
func (a *Agent) deletePid() { | ||
pidPath := a.config.PidFile | ||
|
||
if pidPath != "" { | ||
err := os.Remove(pidPath) | ||
|
||
if err != nil { | ||
fmt.Errorf("Could not remove pid file: %s", err) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,7 @@ func (c *Command) readConfig() *Config { | |
cmdFlags.StringVar(&cmdConfig.Datacenter, "dc", "", "node datacenter") | ||
cmdFlags.StringVar(&cmdConfig.DataDir, "data-dir", "", "path to the data directory") | ||
cmdFlags.StringVar(&cmdConfig.UiDir, "ui-dir", "", "path to the web UI directory") | ||
cmdFlags.StringVar(&cmdConfig.PidFile, "pid-file", "", "path to file to store PID") | ||
|
||
cmdFlags.BoolVar(&cmdConfig.Server, "server", false, "run agent as server") | ||
cmdFlags.BoolVar(&cmdConfig.Bootstrap, "bootstrap", false, "enable server bootstrap mode") | ||
|
@@ -485,6 +486,7 @@ Options: | |
-protocol=N Sets the protocol version. Defaults to latest. | ||
-server Switches agent to server mode. | ||
-ui-dir=path Path to directory containing the Web UI resources | ||
-pid-file=path Path to file to store agent PID | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usage info seem over-indented. |
||
|
||
` | ||
return strings.TrimSpace(helpText) | ||
|
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.
Even if this may fail to open today, it may be nice to sanity check to make sure
pidPath
is not a directory sinceos.Remove
will remove directories. I'm thinking in scenarios like this: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.
I put the check in
deletePid()
beforeos.Remove
happens, since that's what we're actually worried about. Does that seem ok?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.
I'm not sure I agree, seeing this error on start would help fixing it right away. I think this would be a totally fine fatal:
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.
Gotchya, added check to
storePid