-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: add --pid-file
option to write PID files
#25474
base: main-2.x
Are you sure you want to change the base?
Changes from all 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 |
---|---|---|
|
@@ -3,8 +3,14 @@ package launcher_test | |
import ( | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
"io" | ||
"io/fs" | ||
nethttp "net/http" | ||
"os" | ||
"path/filepath" | ||
"runtime" | ||
"strconv" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -14,6 +20,10 @@ import ( | |
"github.com/influxdata/influxdb/v2/http" | ||
"github.com/influxdata/influxdb/v2/tenant" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
"go.uber.org/zap" | ||
"go.uber.org/zap/zapcore" | ||
"go.uber.org/zap/zaptest/observer" | ||
) | ||
|
||
// Default context. | ||
|
@@ -164,3 +174,125 @@ func TestLauncher_PingHeaders(t *testing.T) { | |
assert.Equal(t, []string{"OSS"}, resp.Header.Values("X-Influxdb-Build")) | ||
assert.Equal(t, []string{"dev"}, resp.Header.Values("X-Influxdb-Version")) | ||
} | ||
|
||
func TestLauncher_PIDFile(t *testing.T) { | ||
pidDir := t.TempDir() | ||
pidFilename := filepath.Join(pidDir, "influxd.pid") | ||
|
||
l := launcher.RunAndSetupNewLauncherOrFail(ctx, t, func(o *launcher.InfluxdOpts) { | ||
o.PIDFile = pidFilename | ||
}) | ||
defer func() { | ||
l.ShutdownOrFail(t, ctx) | ||
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. I ran in to a similar thing as this recently: #25398 (comment) I think that this method call 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. It's inside a lambda, so it will not be evaluated until the defer calls the lambda. Also note that if it got called immediately the 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. @devanbenz - the arguments to a deferred function are evaluated on the execution of the |
||
require.NoFileExists(t, pidFilename) | ||
}() | ||
|
||
require.FileExists(t, pidFilename) | ||
pidBytes, err := os.ReadFile(pidFilename) | ||
require.NoError(t, err) | ||
require.Equal(t, strconv.Itoa(os.Getpid()), string(pidBytes)) | ||
} | ||
|
||
func TestLauncher_PIDFile_Locked(t *testing.T) { | ||
pidDir := t.TempDir() | ||
pidFilename := filepath.Join(pidDir, "influxd.pid") | ||
lockContents := []byte("foobar") // something wouldn't appear in normal lock file | ||
|
||
// Write PID file to lock out the launcher. | ||
require.NoError(t, os.WriteFile(pidFilename, lockContents, 0666)) | ||
require.FileExists(t, pidFilename) | ||
origSt, err := os.Stat(pidFilename) | ||
require.NoError(t, err) | ||
|
||
// Make sure we get an error about the PID file from the launcher | ||
l := launcher.NewTestLauncher() | ||
err = l.Run(t, ctx, func(o *launcher.InfluxdOpts) { | ||
o.PIDFile = pidFilename | ||
}) | ||
defer func() { | ||
l.ShutdownOrFail(t, ctx) | ||
|
||
require.FileExists(t, pidFilename) | ||
contents, err := os.ReadFile(pidFilename) | ||
require.NoError(t, err) | ||
require.Equal(t, lockContents, contents) | ||
curSt, err := os.Stat(pidFilename) | ||
require.NoError(t, err) | ||
|
||
// CircleCI test runners for darwin don't have `noatime` / `relatime`, so | ||
// the atime will differ, which is inside the system specific data. | ||
if runtime.GOOS != "darwin" { | ||
require.Equal(t, origSt, curSt) | ||
} | ||
}() | ||
|
||
require.ErrorIs(t, err, launcher.ErrPIDFileExists) | ||
require.ErrorContains(t, err, fmt.Sprintf("error writing PIDFile %q: PID file exists (possible unclean shutdown or another instance already running)", pidFilename)) | ||
} | ||
|
||
func TestLauncher_PIDFile_Overwrite(t *testing.T) { | ||
pidDir := t.TempDir() | ||
pidFilename := filepath.Join(pidDir, "influxd.pid") | ||
lockContents := []byte("foobar") // something wouldn't appear in normal lock file | ||
|
||
// Write PID file to lock out the launcher (or not in this case). | ||
require.NoError(t, os.WriteFile(pidFilename, lockContents, 0666)) | ||
require.FileExists(t, pidFilename) | ||
|
||
// Make sure we get an error about the PID file from the launcher. | ||
l := launcher.NewTestLauncher() | ||
loggerCore, ol := observer.New(zap.WarnLevel) | ||
l.Logger = zap.New(loggerCore) | ||
err := l.Run(t, ctx, func(o *launcher.InfluxdOpts) { | ||
o.PIDFile = pidFilename | ||
o.OverwritePIDFile = true | ||
}) | ||
defer func() { | ||
l.ShutdownOrFail(t, ctx) | ||
|
||
require.NoFileExists(t, pidFilename) | ||
}() | ||
require.NoError(t, err) | ||
|
||
expLogs := []observer.LoggedEntry{ | ||
{ | ||
Entry: zapcore.Entry{Level: zap.WarnLevel, Message: "PID file already exists, attempting to overwrite"}, | ||
Context: []zapcore.Field{zap.String("pidFile", pidFilename)}, | ||
}, | ||
} | ||
require.Equal(t, expLogs, ol.AllUntimed()) | ||
require.FileExists(t, pidFilename) | ||
pidBytes, err := os.ReadFile(pidFilename) | ||
require.NoError(t, err) | ||
require.Equal(t, strconv.Itoa(os.Getpid()), string(pidBytes)) | ||
} | ||
|
||
func TestLauncher_PIDFile_OverwriteFail(t *testing.T) { | ||
pidDir := t.TempDir() | ||
pidFilename := filepath.Join(pidDir, "influxd.pid") | ||
lockContents := []byte("foobar") // something wouldn't appear in normal lock file | ||
|
||
// Write PID file to lock out the launcher. | ||
require.NoError(t, os.WriteFile(pidFilename, lockContents, 0666)) | ||
require.FileExists(t, pidFilename) | ||
require.NoError(t, os.Chmod(pidFilename, 0000)) | ||
|
||
// Make sure we get an error about the PID file from the launcher | ||
l := launcher.NewTestLauncher() | ||
err := l.Run(t, ctx, func(o *launcher.InfluxdOpts) { | ||
o.PIDFile = pidFilename | ||
o.OverwritePIDFile = true | ||
}) | ||
defer func() { | ||
l.ShutdownOrFail(t, ctx) | ||
|
||
require.FileExists(t, pidFilename) | ||
require.NoError(t, os.Chmod(pidFilename, 0644)) | ||
pidBytes, err := os.ReadFile(pidFilename) | ||
require.NoError(t, err) | ||
require.Equal(t, lockContents, pidBytes) | ||
}() | ||
|
||
require.ErrorContains(t, err, fmt.Sprintf("error writing PIDFile %[1]q: overwrite file: open %[1]s:", pidFilename)) | ||
require.ErrorIs(t, err, fs.ErrPermission) | ||
} |
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.
Perhaps add the path in the
fmt.Errorf
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.
The context of the PID filename is added by the caller:
influxdb/cmd/influxd/launcher/launcher.go
Lines 252 to 254 in a91ba1c
I figured this was acceptable elimination of code inside
writePIDFile
since it is a private, single-use method. For a public method I definitely would have added the filename into each message.