Skip to content

Fix socket leak, clean up single use postgres databases #2413

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

Merged
merged 3 commits into from
Jun 16, 2022
Merged

Fix socket leak, clean up single use postgres databases #2413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6ea6c6
Fix socket leak, clean up single use postgres databases
spikecurtis Jun 16, 2022
474186a
Merge branch 'main' of github.com:coder/coder into spike/2347_postgre…
spikecurtis Jun 16, 2022
0d5f35f
Move migrate close defer until after we know it is not nil
spikecurtis Jun 16, 2022
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
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=10m \
-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
Expand Down
10 changes: 6 additions & 4 deletions cli/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
})
}
25 changes: 23 additions & 2 deletions coderd/database/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package database

import (
"context"
"database/sql"
"embed"
"errors"
Expand All @@ -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)
}
Expand All @@ -36,11 +46,22 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

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

Tip (non-blocking): It would be OK to call this err, at the time defer func runs, the value of err would be that of the return statement.

_, m, err := migrateSetup(db)
if err != nil {
return xerrors.Errorf("migrate setup: %w", err)
}
defer func() {
srcErr, dbErr := m.Close()
if retErr != nil {
return
}
if dbErr != nil {
retErr = dbErr
return
}
retErr = srcErr
}()

err = m.Up()
if err != nil {
Expand Down
8 changes: 7 additions & 1 deletion coderd/database/postgres/postgres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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("")
Expand Down