Fix postgres ephemeral ports; don't use for TestPubsub_Disconnect (coder#7798)

spikecurtis · web-flow · commit e016c307aea1 · 2023-06-05T09:24:44.000+04:00
Signed-off-by: Spike Curtis &lt;spike@coder.com&gt;
diff --git a/coderd/database/postgres/postgres.go b/coderd/database/postgres/postgres.go
@@ -3,10 +3,8 @@ package postgres
 import (
 	"database/sql"
 	"fmt"
-	"net"
 	"os"
 	"strconv"
-	"sync"
 	"time"
 
 	"github.com/cenkalti/backoff/v4"
@@ -18,10 +16,6 @@ import (
 	"github.com/coder/coder/cryptorand"
 )
 
-// Required to prevent port collision during container creation.
-// Super unlikely, but it happened. See: https://github.com/coder/coder/runs/5375197003
-var openPortMutex sync.Mutex
-
 // Open creates a new PostgreSQL database instance.  With DB_FROM environment variable set, it clones a database
 // from the provided template.  With the environment variable unset, it creates a new Docker container running postgres.
 func Open() (string, func(), error) {
@@ -68,17 +62,6 @@ func OpenContainerized(port int) (string, func(), error) {
 		return "", nil, xerrors.Errorf("create tempdir: %w", err)
 	}
 
-	openPortMutex.Lock()
-	if port == 0 {
-		// Pick an explicit port on the host to connect to 5432.
-		// This is necessary so we can configure the port to only use ipv4.
-		port, err = getFreePort()
-		if err != nil {
-			openPortMutex.Unlock()
-			return "", nil, xerrors.Errorf("get free port: %w", err)
-		}
-	}
-
 	resource, err := pool.RunWithOptions(&dockertest.RunOptions{
 		Repository: "postgres",
 		Tag:        "13",
@@ -114,10 +97,8 @@ func OpenContainerized(port int) (string, func(), error) {
 		config.RestartPolicy = docker.RestartPolicy{Name: "no"}
 	})
 	if err != nil {
-		openPortMutex.Unlock()
 		return "", nil, xerrors.Errorf("could not start resource: %w", err)
 	}
-	openPortMutex.Unlock()
 
 	hostAndPort := resource.GetHostPort("5432/tcp")
 	dbURL := fmt.Sprintf("postgres://postgres:postgres@%s/postgres?sslmode=disable", hostAndPort)
@@ -166,18 +147,3 @@ func OpenContainerized(port int) (string, func(), error) {
 		_ = os.RemoveAll(tempDir)
 	}, nil
 }
-
-// getFreePort asks the kernel for a free open port that is ready to use.
-func getFreePort() (port int, err error) {
-	// Binding to port 0 tells the OS to grab a port for us:
-	// https://stackoverflow.com/questions/1365265/on-localhost-how-do-i-pick-a-free-port-number
-	listener, err := net.Listen("tcp", "localhost:0")
-	if err != nil {
-		return 0, err
-	}
-
-	defer listener.Close()
-	// This is always a *net.TCPAddr.
-	// nolint:forcetypeassert
-	return listener.Addr().(*net.TCPAddr).Port, nil
-}
diff --git a/coderd/database/pubsub_test.go b/coderd/database/pubsub_test.go
@@ -7,8 +7,6 @@ import (
 	"database/sql"
 	"fmt"
 	"math/rand"
-	"net"
-	"net/url"
 	"strconv"
 	"testing"
 	"time"
@@ -117,11 +115,17 @@ func TestPubsub_ordering(t *testing.T) {
 	}
 }
 
+// disconnectTestPort is the hardcoded port for TestPubsub_Disconnect.  In this test we need to be able to stop Postgres
+// and restart it on the same port.  If we use an ephemeral port, there is a chance the OS will reallocate before we
+// start back up.  The downside is that if the test crashes and leaves the container up, subsequent test runs will fail
+// until we manually kill the container.
+const disconnectTestPort = 26892
+
+// nolint: paralleltest
 func TestPubsub_Disconnect(t *testing.T) {
-	t.Parallel()
 	// we always use a Docker container for this test, even in CI, since we need to be able to kill
 	// postgres and bring it back on the same port.
-	connectionURL, closePg, err := postgres.OpenContainerized(0)
+	connectionURL, closePg, err := postgres.OpenContainerized(disconnectTestPort)
 	require.NoError(t, err)
 	defer closePg()
 	db, err := sql.Open("postgres", connectionURL)
@@ -191,13 +195,8 @@ func TestPubsub_Disconnect(t *testing.T) {
 
 	// restart postgres on the same port --- since we only use LISTEN/NOTIFY it doesn't
 	// matter that the new postgres doesn't have any persisted state from before.
-	u, err := url.Parse(connectionURL)
-	require.NoError(t, err)
-	addr, err := net.ResolveTCPAddr("tcp", u.Host)
-	require.NoError(t, err)
-	newURL, closeNewPg, err := postgres.OpenContainerized(addr.Port)
+	_, closeNewPg, err := postgres.OpenContainerized(disconnectTestPort)
 	require.NoError(t, err)
-	require.Equal(t, connectionURL, newURL)
 	defer closeNewPg()
 
 	// now write messages until we DON'T hit an error -- pubsub is back up.
