From b6ea6c6b722d528fc27ab552d18033e945d97474 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Thu, 16 Jun 2022 08:17:17 -0700 Subject: [PATCH 1/2] Fix socket leak, clean up single use postgres databases Signed-off-by: Spike Curtis --- Makefile | 4 ++-- cli/list_test.go | 10 ++++++---- coderd/database/migrate.go | 25 +++++++++++++++++++++++-- coderd/database/postgres/postgres.go | 8 +++++++- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/Makefile b/Makefile index a75a9cb2884c9..2fd940cb6f13b 100644 --- a/Makefile +++ b/Makefile @@ -115,9 +115,9 @@ test: test-clean .PHONY: test-postgres test-postgres: test-clean DB=ci gotestsum --junitfile="gotests.xml" --packages="./..." -- \ - -covermode=atomic -coverprofile="gotests.coverage" -timeout=5m \ + -covermode=atomic -coverprofile="gotests.coverage" -timeout=30m \ -coverpkg=./...,github.com/coder/coder/codersdk \ - -count=1 -parallel=1 -race -failfast + -count=1 -race -failfast .PHONY: test-postgres-docker diff --git a/cli/list_test.go b/cli/list_test.go index 994b364dd7489..98a3ae7fdf060 100644 --- a/cli/list_test.go +++ b/cli/list_test.go @@ -5,7 +5,7 @@ import ( "testing" "time" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" "github.com/coder/coder/cli/clitest" "github.com/coder/coder/coderd/coderdtest" @@ -30,13 +30,15 @@ func TestList(t *testing.T) { pty := ptytest.New(t) cmd.SetIn(pty.Input()) cmd.SetOut(pty.Output()) - errC := make(chan error) + done := make(chan any) go func() { - errC <- cmd.ExecuteContext(ctx) + errC := cmd.ExecuteContext(ctx) + assert.NoError(t, errC) + close(done) }() pty.ExpectMatch(workspace.Name) pty.ExpectMatch("Running") cancelFunc() - require.NoError(t, <-errC) + <-done }) } diff --git a/coderd/database/migrate.go b/coderd/database/migrate.go index 25ba7a44d6143..795157ee88da9 100644 --- a/coderd/database/migrate.go +++ b/coderd/database/migrate.go @@ -1,6 +1,7 @@ package database import ( + "context" "database/sql" "embed" "errors" @@ -17,12 +18,21 @@ import ( var migrations embed.FS func migrateSetup(db *sql.DB) (source.Driver, *migrate.Migrate, error) { + ctx := context.Background() sourceDriver, err := iofs.New(migrations, "migrations") if err != nil { return nil, nil, xerrors.Errorf("create iofs: %w", err) } - dbDriver, err := postgres.WithInstance(db, &postgres.Config{}) + // there is a postgres.WithInstance() method that takes the DB instance, + // but, when you close the resulting Migrate, it closes the DB, which + // we don't want. Instead, create just a connection that will get closed + // when migration is done. + conn, err := db.Conn(ctx) + if err != nil { + return nil, nil, xerrors.Errorf("postgres connection: %w", err) + } + dbDriver, err := postgres.WithConnection(ctx, conn, &postgres.Config{}) if err != nil { return nil, nil, xerrors.Errorf("wrap postgres connection: %w", err) } @@ -36,8 +46,19 @@ func migrateSetup(db *sql.DB) (source.Driver, *migrate.Migrate, error) { } // MigrateUp runs SQL migrations to ensure the database schema is up-to-date. -func MigrateUp(db *sql.DB) error { +func MigrateUp(db *sql.DB) (retErr error) { _, m, err := migrateSetup(db) + defer func() { + srcErr, dbErr := m.Close() + if retErr != nil { + return + } + if dbErr != nil { + retErr = dbErr + return + } + retErr = srcErr + }() if err != nil { return xerrors.Errorf("migrate setup: %w", err) } diff --git a/coderd/database/postgres/postgres.go b/coderd/database/postgres/postgres.go index 73306eb88b4c0..39cd015321dfb 100644 --- a/coderd/database/postgres/postgres.go +++ b/coderd/database/postgres/postgres.go @@ -44,7 +44,13 @@ func Open() (string, func(), error) { return "", nil, xerrors.Errorf("create db: %w", err) } - return "postgres://postgres:postgres@127.0.0.1:5432/" + dbName + "?sslmode=disable", func() {}, nil + deleteDB := func() { + ddb, _ := sql.Open("postgres", dbURL) + defer ddb.Close() + _, _ = ddb.Exec("DROP DATABASE " + dbName) + } + + return "postgres://postgres:postgres@127.0.0.1:5432/" + dbName + "?sslmode=disable", deleteDB, nil } pool, err := dockertest.NewPool("") From 0d5f35f7e9f295ef016d25929386f350f8efb512 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Thu, 16 Jun 2022 08:44:43 -0700 Subject: [PATCH 2/2] Move migrate close defer until after we know it is not nil Signed-off-by: Spike Curtis --- coderd/database/migrate.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/database/migrate.go b/coderd/database/migrate.go index 795157ee88da9..88b4bf5e3b725 100644 --- a/coderd/database/migrate.go +++ b/coderd/database/migrate.go @@ -48,6 +48,9 @@ func migrateSetup(db *sql.DB) (source.Driver, *migrate.Migrate, error) { // MigrateUp runs SQL migrations to ensure the database schema is up-to-date. func MigrateUp(db *sql.DB) (retErr error) { _, m, err := migrateSetup(db) + if err != nil { + return xerrors.Errorf("migrate setup: %w", err) + } defer func() { srcErr, dbErr := m.Close() if retErr != nil { @@ -59,9 +62,6 @@ func MigrateUp(db *sql.DB) (retErr error) { } retErr = srcErr }() - if err != nil { - return xerrors.Errorf("migrate setup: %w", err) - } err = m.Up() if err != nil {