From 5e0f0ba1785f1a67ccea7d010785180a266fda13 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 3 May 2018 13:56:42 -0700 Subject: [PATCH] agent/proxy: write pid file whenever the daemon process changes --- agent/agent.go | 44 ++------------------ agent/proxy/daemon.go | 26 +++++++++++- agent/proxy/daemon_test.go | 85 ++++++++++++++++++++++++++++++++++++++ lib/file/atomic.go | 46 +++++++++++++++++++++ 4 files changed, 159 insertions(+), 42 deletions(-) create mode 100644 lib/file/atomic.go diff --git a/agent/agent.go b/agent/agent.go index 128a40a4a..2c18c08d3 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -34,6 +34,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/consul/lib" + "github.com/hashicorp/consul/lib/file" "github.com/hashicorp/consul/logger" "github.com/hashicorp/consul/types" "github.com/hashicorp/consul/watch" @@ -362,7 +363,7 @@ func (a *Agent) Start() error { a.proxyManager = proxy.NewManager() a.proxyManager.State = a.State a.proxyManager.Logger = a.logger - a.proxyManager.LogDir = filepath.Join(a.config.DataDir, "proxy", "logs") + a.proxyManager.DataDir = filepath.Join(a.config.DataDir, "proxy") go a.proxyManager.Run() // Start watching for critical services to deregister, based on their @@ -1557,7 +1558,7 @@ func (a *Agent) persistService(service *structs.NodeService) error { return err } - return writeFileAtomic(svcPath, encoded) + return file.WriteAtomic(svcPath, encoded) } // purgeService removes a persisted service definition file from the data dir @@ -1585,7 +1586,7 @@ func (a *Agent) persistCheck(check *structs.HealthCheck, chkType *structs.CheckT return err } - return writeFileAtomic(checkPath, encoded) + return file.WriteAtomic(checkPath, encoded) } // purgeCheck removes a persisted check definition file from the data dir @@ -1597,43 +1598,6 @@ func (a *Agent) purgeCheck(checkID types.CheckID) error { return nil } -// writeFileAtomic writes the given contents to a temporary file in the same -// directory, does an fsync and then renames the file to its real path -func writeFileAtomic(path string, contents []byte) error { - uuid, err := uuid.GenerateUUID() - if err != nil { - return err - } - tempPath := fmt.Sprintf("%s-%s.tmp", path, uuid) - - if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { - return err - } - fh, err := os.OpenFile(tempPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) - if err != nil { - return err - } - if _, err := fh.Write(contents); err != nil { - fh.Close() - os.Remove(tempPath) - return err - } - if err := fh.Sync(); err != nil { - fh.Close() - os.Remove(tempPath) - return err - } - if err := fh.Close(); err != nil { - os.Remove(tempPath) - return err - } - if err := os.Rename(tempPath, path); err != nil { - os.Remove(tempPath) - return err - } - return nil -} - // AddService is used to add a service entry. // This entry is persistent and the agent will make a best effort to // ensure it is registered diff --git a/agent/proxy/daemon.go b/agent/proxy/daemon.go index d6b68ad3a..e3b376c05 100644 --- a/agent/proxy/daemon.go +++ b/agent/proxy/daemon.go @@ -6,8 +6,11 @@ import ( "os" "os/exec" "reflect" + "strconv" "sync" "time" + + "github.com/hashicorp/consul/lib/file" ) // Constants related to restart timers with the daemon mode proxies. At some @@ -38,6 +41,12 @@ type Daemon struct { // a file. Logger *log.Logger + // PidPath is the path where a pid file will be created storing the + // pid of the active process. If this is empty then a pid-file won't + // be created. Under erroneous conditions, the pid file may not be + // created but the error will be logged to the Logger. + PidPath string + // For tests, they can set this to change the default duration to wait // for a graceful quit. gracefulWait time.Duration @@ -187,8 +196,21 @@ func (p *Daemon) start() (*os.Process, error) { // Start it p.Logger.Printf("[DEBUG] agent/proxy: starting proxy: %q %#v", cmd.Path, cmd.Args[1:]) - err := cmd.Start() - return cmd.Process, err + if err := cmd.Start(); err != nil { + return nil, err + } + + // Write the pid file. This might error and that's okay. + if p.PidPath != "" { + pid := strconv.FormatInt(int64(cmd.Process.Pid), 10) + if err := file.WriteAtomic(p.PidPath, []byte(pid)); err != nil { + p.Logger.Printf( + "[DEBUG] agent/proxy: error writing pid file %q: %s", + p.PidPath, err) + } + } + + return cmd.Process, nil } // Stop stops the daemon. diff --git a/agent/proxy/daemon_test.go b/agent/proxy/daemon_test.go index 32acde636..652364c5e 100644 --- a/agent/proxy/daemon_test.go +++ b/agent/proxy/daemon_test.go @@ -142,6 +142,91 @@ func TestDaemonStop_kill(t *testing.T) { require.Equal(mtime, fi.ModTime()) } +func TestDaemonStart_pidFile(t *testing.T) { + t.Parallel() + + require := require.New(t) + td, closer := testTempDir(t) + defer closer() + + path := filepath.Join(td, "file") + pidPath := filepath.Join(td, "pid") + uuid, err := uuid.GenerateUUID() + require.NoError(err) + + d := &Daemon{ + Command: helperProcess("start-once", path), + ProxyToken: uuid, + Logger: testLogger, + PidPath: pidPath, + } + require.NoError(d.Start()) + defer d.Stop() + + // Wait for the file to exist + retry.Run(t, func(r *retry.R) { + _, err := os.Stat(pidPath) + if err == nil { + return + } + + r.Fatalf("error: %s", err) + }) + + // Check the pid file + pidRaw, err := ioutil.ReadFile(pidPath) + require.NoError(err) + require.NotEmpty(pidRaw) +} + +// Verify the pid file changes on restart +func TestDaemonRestart_pidFile(t *testing.T) { + t.Parallel() + + require := require.New(t) + td, closer := testTempDir(t) + defer closer() + path := filepath.Join(td, "file") + pidPath := filepath.Join(td, "pid") + + d := &Daemon{ + Command: helperProcess("restart", path), + Logger: testLogger, + PidPath: pidPath, + } + require.NoError(d.Start()) + defer d.Stop() + + // Wait for the file to exist. We save the func so we can reuse the test. + waitFile := func() { + retry.Run(t, func(r *retry.R) { + _, err := os.Stat(path) + if err == nil { + return + } + r.Fatalf("error waiting for path: %s", err) + }) + } + waitFile() + + // Check the pid file + pidRaw, err := ioutil.ReadFile(pidPath) + require.NoError(err) + require.NotEmpty(pidRaw) + + // Delete the file + require.NoError(os.Remove(path)) + + // File should re-appear because the process is restart + waitFile() + + // Check the pid file and it should not equal + pidRaw2, err := ioutil.ReadFile(pidPath) + require.NoError(err) + require.NotEmpty(pidRaw2) + require.NotEqual(pidRaw, pidRaw2) +} + func TestDaemonEqual(t *testing.T) { cases := []struct { Name string diff --git a/lib/file/atomic.go b/lib/file/atomic.go new file mode 100644 index 000000000..e1d6e6693 --- /dev/null +++ b/lib/file/atomic.go @@ -0,0 +1,46 @@ +package file + +import ( + "fmt" + "os" + "path/filepath" + + "github.com/hashicorp/go-uuid" +) + +// WriteAtomic writes the given contents to a temporary file in the same +// directory, does an fsync and then renames the file to its real path +func WriteAtomic(path string, contents []byte) error { + uuid, err := uuid.GenerateUUID() + if err != nil { + return err + } + tempPath := fmt.Sprintf("%s-%s.tmp", path, uuid) + + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + return err + } + fh, err := os.OpenFile(tempPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) + if err != nil { + return err + } + if _, err := fh.Write(contents); err != nil { + fh.Close() + os.Remove(tempPath) + return err + } + if err := fh.Sync(); err != nil { + fh.Close() + os.Remove(tempPath) + return err + } + if err := fh.Close(); err != nil { + os.Remove(tempPath) + return err + } + if err := os.Rename(tempPath, path); err != nil { + os.Remove(tempPath) + return err + } + return nil +}