docker daemon: create file containing PID #378

Merged
merged 1 commit into from Apr 12, 2013

Projects

None yet

5 participants

@flavio

Ensure the docker daemon creates a file containing its PID under /var/run/docker.pid.

The daemon takes care of removing the pid file when it receives either SIGTERM, SIGINT or SIGKILL.

The daemon also refuses to start when the pidfile is found. An explanation message is shown to the user when this happens.

This change is required to make docker easier to manage by tools like checkproc which rely on this information.

A final note: I'm still a go newbie, please take that into consideration while reviewing the pull request :)

@dominikh dominikh commented on an outdated diff Apr 10, 2013
docker/docker.go
@@ -47,6 +67,30 @@ func main() {
}
}
+func createPidFile(pidfile string) error {
+ if _, err := os.Stat(pidfile); err != nil {
+ file, err := os.Create(pidfile)
+ if err != nil {
+ return err
+ }
+
+ defer file.Close()
+
+ if _, err = file.WriteString(fmt.Sprintf("%d", os.Getpid())); err != nil {
@dominikh
dominikh Apr 10, 2013
if _, err = fmt.Fprintf(file, "%d", os.Getpid()); err != nil {

simplifies the writing a bit.

@dominikh dominikh commented on an outdated diff Apr 10, 2013
docker/docker.go
+func createPidFile(pidfile string) error {
+ if _, err := os.Stat(pidfile); err != nil {
+ file, err := os.Create(pidfile)
+ if err != nil {
+ return err
+ }
+
+ defer file.Close()
+
+ if _, err = file.WriteString(fmt.Sprintf("%d", os.Getpid())); err != nil {
+ return err
+ }
+ } else {
+ return fmt.Errorf("pid file found, ensure docker is not running or delete %s", pidfile)
+ }
+ return nil
@dominikh
dominikh Apr 10, 2013

1) You could leave out the } else { and just return the error, and get rid of return nil
2) Maybe I'm missing something, but if os.Stat does not return an error, doesn't that mean that the file exists? This here looks backwards

@dominikh dominikh commented on an outdated diff Apr 10, 2013
docker/docker.go
@@ -37,6 +40,23 @@ func main() {
flag.Usage()
return
}
+ pidfile := "/var/run/docker.pid"
+ if err := createPidFile(pidfile); err != nil {
+ log.Fatal(err)
+ return
+ }
+ defer removePidFile(pidfile)
+
+ c := make(chan os.Signal, 1)
+ signal.Notify(c, os.Interrupt, os.Kill, os.Signal(syscall.SIGTERM))
+ go func() {
+ for sig := range c {
@dominikh
dominikh Apr 10, 2013

a plain <- c to wait for a signal suffices, instead of ranging over the channel.

@titanous titanous commented on an outdated diff Apr 10, 2013
docker/docker.go
@@ -37,6 +40,23 @@ func main() {
flag.Usage()
return
}
+ pidfile := "/var/run/docker.pid"
+ if err := createPidFile(pidfile); err != nil {
+ log.Fatal(err)
+ return
@titanous
titanous Apr 10, 2013

No need to return. log.Fatal calls os.Exit(1).

@titanous titanous commented on an outdated diff Apr 10, 2013
docker/docker.go
@@ -37,6 +40,23 @@ func main() {
flag.Usage()
return
}
+ pidfile := "/var/run/docker.pid"
@titanous
titanous Apr 10, 2013

This should be moved to a constant or possibly a command-line flag.

@titanous titanous commented on an outdated diff Apr 10, 2013
docker/docker.go
+ }
+
+ defer file.Close()
+
+ if _, err = file.WriteString(fmt.Sprintf("%d", os.Getpid())); err != nil {
+ return err
+ }
+ } else {
+ return fmt.Errorf("pid file found, ensure docker is not running or delete %s", pidfile)
+ }
+ return nil
+}
+
+func removePidFile(pidfile string) {
+ if err := os.Remove(pidfile); err != nil {
+ log.Println("Error:", err.Error())
@titanous
titanous Apr 10, 2013
log.Printf("Error removing %s: %s", pidfile, err)
@flavio

Thanks for the feedback, I hope the new version looks better.

@dominikh dominikh commented on an outdated diff Apr 10, 2013
docker/docker.go
@@ -37,6 +41,21 @@ func main() {
flag.Usage()
return
}
+ if err := createPidFile(*pidfile); err != nil {
+ log.Fatal(err)
+ }
+ defer removePidFile(*pidfile)
+
+ c := make(chan os.Signal, 1)
+ signal.Notify(c, os.Interrupt, os.Kill, os.Signal(syscall.SIGTERM))
+ go func() {
+ var sig os.Signal
+ sig = <-c
@dominikh
dominikh Apr 10, 2013

sig := <-c and drop the var sig ...

@titanous titanous and 1 other commented on an outdated diff Apr 10, 2013
docker/docker.go
@@ -47,6 +66,31 @@ func main() {
}
}
+func createPidFile(pidfile string) error {
+ if _, err := os.Stat(pidfile); err == nil {
+ return fmt.Errorf("pid file found, ensure docker is not running or delete %s", pidfile)
+ }
+
+ file, err := os.Create(pidfile)
+ if err != nil {
+ return err
+ }
+
+ defer file.Close()
+
+ if _, err = fmt.Fprintf(file, "%d", os.Getpid()); err != nil {
+ return err
+ }
@titanous
titanous Apr 10, 2013

Drop the if:

_, err = fmt.Fprintf(file, "%d", os.Getpid())
return err
@dominikh
dominikh Apr 10, 2013

_, err =, but yeah.

@dominikh

looks good to me.

@titanous

This raises a thought: at some point should we have a "graceful shutdown" that doesn't just os.Exit right away?

@flavio

This raises a thought: at some point should we have a "graceful shutdown" that doesn't just os.Exit right away?

You are right. I think that's something which needs to be addressed with a dedicated issue.

@titanous

Created #379.

@shykes

Hey @flavio, I look forward to merging this!

I think the pidfile should only created when running docker in daemon mode ('docker -d'). When running it in client mode we probably don't want it.

Look for daemon() in docker/docker.go

@flavio flavio docker daemon: create file containing PID
Ensure the docker daemon creates a file containing its PID under
/var/run/docker.pid.

The daemon takes care of removing the pid file when it receives either
SIGTERM, SIGINT or SIGKILL.

The daemon also refuses to start when the pidfile is found. An
explanation message is shown to the user when this happens.

This change is required to make docker easier to manage by tools like
checkproc which rely on this information.
fb0b375
@flavio

I think the pidfile should only created when running docker in daemon mode ('docker -d'). When running it in client mode we probably don't want it.

The pidfile was already being created only when running in daemon mode, but you are right: I think it's better to move this code inside of the daemon() function.

@creack creack merged commit fb0b375 into docker:master Apr 12, 2013
@creack

thanks @flavio! nice job :)

@flavio flavio deleted the flavio:create_pidfile branch Apr 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment