Skip to content
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.

Sign up for GitHub

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

testserver: expose NoFileCleanup option to allow retention of files after Stop #172

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

testserver: expose NoFileCleanup option to allow retention of files after Stop #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0276345
testserver: expose `NoFileCleanup` option to allow retention of files…
janmejay Oct 11, 2023
ae92af3
testserver: expose `BasedirOverride` to allow deeper testing of `NoFi…
janmejay Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions testserver/tenant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (ts *testServerImpl) NewTenantServer(proxy bool) (TestServer, error) {
}

secureFlag := "--insecure"
certsDir := filepath.Join(ts.baseDir, "certs")
certsDir := filepath.Join(ts.BaseDir(), "certs")
if ts.serverArgs.secure {
secureFlag = "--certs-dir=" + certsDir
// Create tenant client certificate.
Expand Down Expand Up @@ -235,16 +235,15 @@ func (ts *testServerImpl) NewTenantServer(proxy bool) (TestServer, error) {
// TODO(asubiotto): Specify listeningURLFile once we support dynamic
// ports.
listeningURLFile: "",
stdout: filepath.Join(ts.baseDir, logsDirName, fmt.Sprintf("cockroach.tenant.%d.stdout", tenantID)),
stderr: filepath.Join(ts.baseDir, logsDirName, fmt.Sprintf("cockroach.tenant.%d.stderr", tenantID)),
stdout: filepath.Join(ts.BaseDir(), logsDirName, fmt.Sprintf("cockroach.tenant.%d.stdout", tenantID)),
stderr: filepath.Join(ts.BaseDir(), logsDirName, fmt.Sprintf("cockroach.tenant.%d.stderr", tenantID)),
},
}

tenant := &testServerImpl{
serverArgs: ts.serverArgs,
version: ts.version,
serverState: stateNew,
baseDir: ts.baseDir,
nodes: nodes,
}

Expand Down
57 changes: 43 additions & 14 deletions testserver/testserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ type testServerImpl struct {
version *version.Version
serverArgs testServerArgs
serverState int
baseDir string
pgURL []pgURLChan
initCmd *exec.Cmd
initCmdArgs []string
Expand Down Expand Up @@ -240,6 +239,8 @@ type testServerArgs struct {
envVars []string // to be passed to cmd.Env
localityFlags []string
cockroachLogsDir string
noFileCleanup bool // do not clean files at `Stop`
baseDir string
}

// CockroachBinaryPathOpt is a TestServer option that can be passed to
Expand Down Expand Up @@ -274,6 +275,23 @@ func StoreOnDiskOpt() TestServerOpt {
}
}

// NoFileCleanup is a TestServer option that can be passed to NewTestServer
// to skip cleanup of files when Testserver is stopped
func NoFileCleanup() TestServerOpt {
return func(args *testServerArgs) {
args.noFileCleanup = true
}
}

// BasedirOverride is a TestServer option that can be passed to NewTestServer
// to use a given path as testserver base-dir (as opposed to creating a
// temporary one on the fly, which is the default behavior).
func BasedirOverride(baseDirPath string) TestServerOpt {
return func(args *testServerArgs) {
args.baseDir = baseDirPath
}
}

// SetStoreMemSizeOpt is a TestServer option that can be passed to NewTestServer
// to set the proportion of available memory that is allocated
// to the test server.
Expand Down Expand Up @@ -428,20 +446,26 @@ var errStoppedInMiddle = errors.New("download stopped in middle")
// If the download fails, we attempt just call "cockroach", hoping it is
// found in your path.
func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
baseDir, err := os.MkdirTemp("", "cockroach-testserver")
if err != nil {
return nil, fmt.Errorf("%s: could not create temp directory: %w", testserverMessagePrefix, err)
}

serverArgs := &testServerArgs{numNodes: 1}
serverArgs.storeMemSize = defaultStoreMemSize
serverArgs.initTimeoutSeconds = defaultInitTimeout
serverArgs.pollListenURLTimeoutSeconds = defaultPollListenURLTimeout
serverArgs.listenAddrHost = defaultListenAddrHost
serverArgs.cockroachLogsDir = baseDir
for _, applyOptToArgs := range opts {
applyOptToArgs(serverArgs)
}
var err error
if serverArgs.baseDir == "" {
baseDir, err := os.MkdirTemp("", "cockroach-testserver")
if err != nil {
return nil, fmt.Errorf("%s: could not create temp directory: %w", testserverMessagePrefix, err)
}
serverArgs.baseDir = baseDir
if serverArgs.cockroachLogsDir == "" {
serverArgs.cockroachLogsDir = baseDir
}
}

log.Printf("cockroach logs directory: %s", serverArgs.cockroachLogsDir)

if serverArgs.cockroachBinary != "" {
Expand Down Expand Up @@ -487,7 +511,7 @@ func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
}

mkDir := func(name string) (string, error) {
path := filepath.Join(baseDir, name)
path := filepath.Join(serverArgs.baseDir, name)
if err := os.MkdirAll(path, 0755); err != nil {
return "", fmt.Errorf("%s: could not create %s directory: %s: %w",
testserverMessagePrefix, name, path, err)
Expand Down Expand Up @@ -629,7 +653,6 @@ func NewTestServer(opts ...TestServerOpt) (TestServer, error) {
serverArgs: *serverArgs,
version: v,
serverState: stateNew,
baseDir: baseDir,
initCmdArgs: initArgs,
curTenantID: firstTenantID,
nodes: nodes,
Expand Down Expand Up @@ -677,7 +700,7 @@ func (ts *testServerImpl) StderrForNode(i int) string {

// BaseDir returns directory StoreOnDiskOpt writes to if used.
func (ts *testServerImpl) BaseDir() string {
return ts.baseDir
return ts.serverArgs.baseDir
}

// PGURL returns the postgres connection URL to reach the started
Expand Down Expand Up @@ -878,8 +901,10 @@ func (ts *testServerImpl) Stop() {
}
ts.mu.RLock()

nodeDir := filepath.Join(ts.baseDir, strconv.Itoa(i))
if err := os.RemoveAll(nodeDir); err != nil {
nodeDir := filepath.Join(ts.BaseDir(), strconv.Itoa(i))
if ts.serverArgs.noFileCleanup {
log.Printf("%s: skipping file cleanup of node-dir %s", testserverMessagePrefix, nodeDir)
} else if err := os.RemoveAll(nodeDir); err != nil {
log.Printf("error deleting tmp directory %s for node: %s", nodeDir, err)
}
if closeErr := ts.nodes[i].stdoutBuf.Close(); closeErr != nil {
Expand All @@ -891,7 +916,11 @@ func (ts *testServerImpl) Stop() {
}

// Only cleanup on intentional stops.
_ = os.RemoveAll(ts.baseDir)
if ts.serverArgs.noFileCleanup {
log.Printf("%s: skipping file cleanup of base-dir %s", testserverMessagePrefix, ts.BaseDir())
} else {
_ = os.RemoveAll(ts.BaseDir())
}
}

func (ts *testServerImpl) CockroachInit() error {
Expand All @@ -907,7 +936,7 @@ func (ts *testServerImpl) CockroachInit() error {
// Set the working directory of the cockroach process to our temp folder.
// This stops cockroach from polluting the project directory with _dump
// folders.
ts.initCmd.Dir = ts.baseDir
ts.initCmd.Dir = ts.serverArgs.baseDir

err := ts.initCmd.Start()
if ts.initCmd.Process != nil {
Expand Down
35 changes: 35 additions & 0 deletions testserver/testserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,12 @@ func TestRunServer(t *testing.T) {
)
},
},
{
name: "No File Cleanup",
instantiation: func(t *testing.T) (*sql.DB, func()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any way for this test to assert that the directory still exists at the end?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a dedicated test TestBaseDirIsPreservedWhenNoFileCleanupRequested (but also had to add some additional machinery to power it).

return testserver.NewDBForTest(t, testserver.NoFileCleanup())
},
},
} {
t.Run(tc.name, func(t *testing.T) {
db, stop := tc.instantiation(t)
Expand All @@ -248,6 +254,35 @@ func TestRunServer(t *testing.T) {
}
}

func TestBaseDirIsPreservedWhenNoFileCleanupRequested(t *testing.T) {
ts, err := testserver.NewTestServer(
testserver.NoFileCleanup(),
testserver.StoreOnDiskOpt())
require.NoError(t, err)
baseDir := ts.BaseDir()

db, err := sql.Open("postgres", ts.PGURL().String())
require.NoError(t, err)

_, err = db.Exec("create table store_preservation_test(id int primary key)")
require.NoError(t, err)
_, err = db.Exec("insert into store_preservation_test(id) values (123456789)")
require.NoError(t, err)
ts.Stop()

newTs, err := testserver.NewTestServer(
testserver.BasedirOverride(baseDir),
testserver.StoreOnDiskOpt())
require.NoError(t, err)
defer newTs.Stop()
db, err = sql.Open("postgres", newTs.PGURL().String())
require.NoError(t, err)
row := db.QueryRow("select id from store_preservation_test")
retrivedIntVal := 0
require.NoError(t, row.Scan(&retrivedIntVal))
require.Equal(t, 123456789, retrivedIntVal)
}

func TestCockroachBinaryPathOpt(t *testing.T) {
crdbBinary := "doesnotexist"
_, err := testserver.NewTestServer(testserver.CockroachBinaryPathOpt(crdbBinary))
Expand Down
2 changes: 1 addition & 1 deletion testserver/testservernode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (ts *testServerImpl) StartNode(i int) error {
// Set the working directory of the cockroach process to our temp folder.
// This stops cockroach from polluting the project directory with _dump
// folders.
currCmd.Dir = ts.baseDir
currCmd.Dir = ts.BaseDir()

if len(ts.nodes[i].stdout) > 0 {
wr, err := newFileLogWriter(ts.nodes[i].stdout)
Expand Down