From ad6f18322d836d8bb0b745c355dcf318cf6ca8b5 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 24 Jul 2024 05:47:31 +0000 Subject: [PATCH 1/6] Add flake.nix for consistent project dev environment --- flake.lock | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ flake.nix | 23 ++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 flake.lock create mode 100644 flake.nix diff --git a/flake.lock b/flake.lock new file mode 100644 index 0000000..ba356d5 --- /dev/null +++ b/flake.lock @@ -0,0 +1,61 @@ +{ + "nodes": { + "flake-utils": { + "inputs": { + "systems": "systems" + }, + "locked": { + "lastModified": 1710146030, + "narHash": "sha256-SZ5L6eA7HJ/nmkzGG7/ISclqe6oZdOZTNoesiInkXPQ=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "b1d9ab70662946ef0850d488da1c9019f3a9752a", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1721622093, + "narHash": "sha256-iQ+quy3A1EKeFyLyAtjhgSvZHH7r+xybXZkxMhasN4I=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "453402b94f39f968a7c27df28e060f69e4a50c3b", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixpkgs-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, + "root": { + "inputs": { + "flake-utils": "flake-utils", + "nixpkgs": "nixpkgs" + } + }, + "systems": { + "locked": { + "lastModified": 1681028828, + "narHash": "sha256-Vy1rq5AaRuLzOxct8nz4T6wlgyUR7zLU309k9mBC768=", + "owner": "nix-systems", + "repo": "default", + "rev": "da67096a3b9bf56a91d16901293e51ba5b49a27e", + "type": "github" + }, + "original": { + "owner": "nix-systems", + "repo": "default", + "type": "github" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix new file mode 100644 index 0000000..b08a91b --- /dev/null +++ b/flake.nix @@ -0,0 +1,23 @@ +{ + inputs = { + nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable"; + flake-utils.url = "github:numtide/flake-utils"; + }; + + outputs = { self, nixpkgs, flake-utils }: (flake-utils.lib.eachDefaultSystem (system: + let + pkgs = nixpkgs.legacyPackages.${system}; + in + { + devShells.default = pkgs.mkShell { + buildInputs = with pkgs; [ + go_1_21 + gotools + golint + go-tools + golangci-lint + gnumake + ]; + }; + })); +} From 1b9e11bf4477b8eb7bbb78b3cd42cb039533ae85 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 24 Jul 2024 05:47:46 +0000 Subject: [PATCH 2/6] go.mod: update, drop unused dependency The runc dependency is unused, we can drop it --- go.mod | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 33c7957..3a392e4 100644 --- a/go.mod +++ b/go.mod @@ -2,16 +2,20 @@ module github.com/ngrok-oss/tableroll/v2 require ( github.com/euank/filelock v0.0.0-20200318073246-6ea232a62104 - github.com/go-stack/stack v1.8.0 // indirect github.com/inconshreveable/log15 v0.0.0-20180818164646-67afb5ed74ec - github.com/mattn/go-colorable v0.0.9 // indirect - github.com/mattn/go-isatty v0.0.4 // indirect github.com/pkg/errors v0.8.1 github.com/stretchr/testify v1.6.1 golang.org/x/sys v0.0.0-20201101102859-da207088b7d1 k8s.io/utils v0.0.0-20190221042446-c2654d5206da ) -replace github.com/opencontainers/runc => github.com/kevinburke/runc v1.0.0-rc8.0.20190502181430-3ec7f94c7eff +require ( + github.com/davecgh/go-spew v1.1.0 // indirect + github.com/go-stack/stack v1.8.0 // indirect + github.com/mattn/go-colorable v0.0.9 // indirect + github.com/mattn/go-isatty v0.0.4 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c // indirect +) -go 1.13 +go 1.21 From 8d7e3c63ba23461cc7077b5bf951a5cf5895058a Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 24 Jul 2024 06:10:58 +0000 Subject: [PATCH 3/6] fds: fix json tag typo Thanks go, good type-system --- fds.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fds.go b/fds.go index f45e7f1..bf61ab0 100644 --- a/fds.go +++ b/fds.go @@ -93,8 +93,8 @@ type fd struct { ID string `json:"id"` // for conns/listeners, stored just for pretty-printing - Network string `json:"network,omitEmpty"` - Addr string `json:"addr,omitEmpty"` + Network string `json:"network,omitempty"` + Addr string `json:"addr,omitempty"` } func (f *fd) String() string { From b35e6af1281389de884a569dbdcf77865b76ca1d Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 24 Jul 2024 06:13:42 +0000 Subject: [PATCH 4/6] Fix non-test golangci lints --- coordination.go | 7 +++---- fds.go | 17 ++--------------- sibling.go | 5 ++--- upgrader.go | 2 -- 4 files changed, 7 insertions(+), 24 deletions(-) diff --git a/coordination.go b/coordination.go index 2182b5d..e6f617a 100644 --- a/coordination.go +++ b/coordination.go @@ -3,7 +3,6 @@ package tableroll import ( "context" "fmt" - "io/ioutil" "net" "os" "path/filepath" @@ -52,7 +51,7 @@ func (c *coordinator) Listen(ctx context.Context) (*net.UnixListener, error) { } func touchFile(path string) error { - f, err := os.OpenFile(path, os.O_CREATE|os.O_RDONLY, 0755) + f, err := os.OpenFile(path, os.O_CREATE|os.O_RDONLY, 0o755) f.Close() return err } @@ -96,7 +95,7 @@ func (c *coordinator) idFile() string { // It should only be called while the lock is held. func (c *coordinator) BecomeOwner() error { c.l.Info("writing id to become owner", "id", c.id) - return ioutil.WriteFile(c.idFile(), []byte(c.id), 0755) + return os.WriteFile(c.idFile(), []byte(c.id), 0o755) } // Unlock unlocks the coordination id file @@ -113,7 +112,7 @@ func (c *coordinator) Unlock() error { // It will return 'errNoOwner' if there isn't currently an owner. func (c *coordinator) GetOwnerID() (string, error) { c.l.Info("discovering current owner") - data, err := ioutil.ReadFile(c.idFile()) + data, err := os.ReadFile(c.idFile()) if err != nil { return "", err } diff --git a/fds.go b/fds.go index bf61ab0..0082999 100644 --- a/fds.go +++ b/fds.go @@ -48,8 +48,8 @@ type fdKind string const ( fdKindListener fdKind = "listener" - fdKindConn = "conn" - fdKindFile = "file" + fdKindConn fdKind = "conn" + fdKindFile fdKind = "file" ) // file works around the fact that it's not possible @@ -449,19 +449,6 @@ func (f *Fds) copy() map[string]*fd { return files } -func unlinkUnixSocket(path string) error { - info, err := os.Stat(path) - if err != nil { - return err - } - - if info.Mode()&os.ModeSocket == 0 { - return nil - } - - return os.Remove(path) -} - func dupConn(conn syscall.Conn, name string) (*file, error) { // Use SyscallConn instead of File to avoid making the original // fd non-blocking. diff --git a/sibling.go b/sibling.go index 0452423..920bf3e 100644 --- a/sibling.go +++ b/sibling.go @@ -11,9 +11,8 @@ import ( ) type sibling struct { - readyC chan struct{} - conn *net.UnixConn - l log15.Logger + conn *net.UnixConn + l log15.Logger } func newSibling(l log15.Logger, conn *net.UnixConn) *sibling { diff --git a/upgrader.go b/upgrader.go index 092cdb7..117e440 100644 --- a/upgrader.go +++ b/upgrader.go @@ -131,8 +131,6 @@ func (u *Upgrader) becomeOwner(ctx context.Context) (bool, error) { return sess.hasOwner(), nil } -var errClosed = errors.New("connection closed") - func (u *Upgrader) serveUpgrades() { for { conn, err := u.upgradeSock.AcceptUnix() From 0c953f2fe597eb484441efa0169d2ad783a9f633 Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 24 Jul 2024 06:13:55 +0000 Subject: [PATCH 5/6] Fix golangci lints for tests, and make various minor improvements --- coordinator_test.go | 62 +++++++--------- fds_test.go | 24 +++---- transfer_ownership_test.go | 18 ++--- upgrader_process_test.go | 61 ++++++++-------- upgrader_test.go | 141 ++++++++++++------------------------- utils_test.go | 24 +++++++ 6 files changed, 142 insertions(+), 188 deletions(-) create mode 100644 utils_test.go diff --git a/coordinator_test.go b/coordinator_test.go index 6c0ee36..5a20ef8 100644 --- a/coordinator_test.go +++ b/coordinator_test.go @@ -2,11 +2,11 @@ package tableroll import ( "context" - "io/ioutil" - "os" + "io" "testing" "github.com/inconshreveable/log15" + "github.com/stretchr/testify/require" "k8s.io/utils/clock" ) @@ -14,63 +14,51 @@ import ( func TestConnectOwner(t *testing.T) { l := log15.New() ctx := context.Background() - tmpdir, err := ioutil.TempDir("", "tableroll_coord_test") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := tmpDir(t) coord1 := newCoordinator(clock.RealClock{}, l, tmpdir, "1") coord2 := newCoordinator(clock.RealClock{}, l, tmpdir, "2") coord1l, err := coord1.Listen(ctx) - if err != nil { - t.Fatalf("Unable to listen: %v", err) - } - coord1.Lock(ctx) - coord1.BecomeOwner() - coord1.Unlock() + require.NoError(t, err) + require.NoError(t, coord1.Lock(ctx)) + require.NoError(t, coord1.BecomeOwner()) + require.NoError(t, coord1.Unlock()) connw, err := coord2.ConnectOwner(ctx) - if err != nil { - t.Fatalf("unable to connect to owner") - } + require.NoError(t, err) + connErr := make(chan error, 1) go func() { - connw.Write([]byte("hello world")) - connw.Close() + defer close(connErr) + if _, err := connw.Write([]byte("hello world")); err != nil { + connErr <- err + } + if err := connw.Close(); err != nil { + connErr <- err + } }() connr, err := coord1l.Accept() if err != nil { t.Fatalf("acccept err: %v", err) } - data, err := ioutil.ReadAll(connr) - if err != nil { - t.Fatalf("read err: %v", err) - } - if string(data) != "hello world" { - t.Fatalf("expected to read %q; got %q", "hello world", string(data)) - } + data, err := io.ReadAll(connr) + require.NoError(t, err) + require.Equal(t, "hello world", string(data)) + require.NoError(t, <-connErr) } // TestLockCoordinationDirCtxCancel tests that a call to `lockCoordinationDir` can be // canceled by canceling the passed in context. func TestLockCoordinationDirCtxCancel(t *testing.T) { l := log15.New() - ctx := context.Background() - tmpdir, err := ioutil.TempDir("", "tableroll_coord_test") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpdir) + ctx := testCtx(t) + tmpdir := tmpDir(t) coord1 := newCoordinator(clock.RealClock{}, l, tmpdir, "1") coord2 := newCoordinator(clock.RealClock{}, l, tmpdir, "2") - err = coord1.Lock(ctx) - if err != nil { - t.Fatalf("Error getting coordination dir: %v", err) - } - defer coord1.Unlock() + require.NoError(t, coord1.Lock(ctx)) + defer func() { require.NoError(t, coord1.Unlock()) }() ctx2, cancel := context.WithCancel(ctx) coordErr := make(chan error) @@ -85,7 +73,7 @@ func TestLockCoordinationDirCtxCancel(t *testing.T) { default: } cancel() - err = <-coordErr + err := <-coordErr if err != context.Canceled { t.Errorf("expected context cancel, got %v", err) } diff --git a/fds_test.go b/fds_test.go index ef62416..3a715ab 100644 --- a/fds_test.go +++ b/fds_test.go @@ -2,11 +2,12 @@ package tableroll import ( "context" - "io/ioutil" "net" "os" "path/filepath" "testing" + + "github.com/stretchr/testify/require" ) func TestFdsListen(t *testing.T) { @@ -42,11 +43,7 @@ func TestFdsListener(t *testing.T) { } defer tcp.Close() - temp, err := ioutil.TempDir("", "tableflip") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(temp) + temp := tmpDir(t) socketPath := filepath.Join(temp, "socket") socketPath2 := filepath.Join(temp, "socket2") @@ -84,20 +81,19 @@ func TestFdsListener(t *testing.T) { child := newFds(l, parent.copy()) ln, err := child.Listener("1") - if err != nil { - t.Fatal("Can't get listener:", err) - } + require.NoError(t, err) if ln == nil { t.Fatal("Missing listener") } ln.Close() - child.Remove("2") + require.NoError(t, child.Remove("2")) if _, err := os.Stat(socketPath); err != nil { t.Errorf("expected socket to still exist") } ln, err = child.Listener("3") + require.NoError(t, err) ln.(*net.UnixListener).SetUnlinkOnClose(true) ln.Close() if _, err := os.Stat(socketPath2); err == nil { @@ -117,10 +113,10 @@ func TestFdsConn(t *testing.T) { t.Fatal("Can't add conn:", err) } unixConn.Close() - defer parent.Remove("1") + defer func() { _ = parent.Remove("1") }() child := newFds(l, parent.copy()) - defer child.Remove("1") + defer func() { _ = child.Remove("1") }() conn, err := child.Conn("1") if err != nil { t.Fatal("Can't get conn:", err) @@ -145,7 +141,7 @@ func TestFdsFile(t *testing.T) { t.Fatal("Can't add file:", err) } w.Close() - defer parent.Remove("test") + defer func() { require.NoError(t, parent.Remove("test")) }() child := newFds(l, parent.copy()) file, err := child.File("test") @@ -162,7 +158,7 @@ func TestFdsLock(t *testing.T) { fds := newFds(l, nil) ln, err := fds.ListenWith("1", "tcp", "127.0.0.1:0", net.Listen) - defer ln.Close() + defer func() { require.NoError(t, ln.Close()) }() if err != nil { t.Fatalf("expected no error in unlocked fds: %v", err) } diff --git a/transfer_ownership_test.go b/transfer_ownership_test.go index e5a8e23..f5b8f22 100644 --- a/transfer_ownership_test.go +++ b/transfer_ownership_test.go @@ -2,28 +2,24 @@ package tableroll import ( "context" - "io/ioutil" - "os" "testing" "github.com/inconshreveable/log15" "github.com/pkg/errors" + "github.com/stretchr/testify/require" "k8s.io/utils/clock" ) func TestGetFilesCtxCancel(t *testing.T) { ctx := context.Background() l := log15.New() - tmpdir, err := ioutil.TempDir("", "tableroll_getfiles") - if err != nil { - panic(err) - } - defer os.RemoveAll(tmpdir) + tmpdir := tmpDir(t) parent := newCoordinator(clock.RealClock{}, l, tmpdir, "1") - parent.Listen(ctx) - parent.Lock(ctx) - parent.BecomeOwner() - parent.Unlock() + _, err := parent.Listen(ctx) + require.NoError(t, err) + require.NoError(t, parent.Lock(ctx)) + require.NoError(t, parent.BecomeOwner()) + require.NoError(t, parent.Unlock()) newParent := newCoordinator(clock.RealClock{}, l, tmpdir, "2") diff --git a/upgrader_process_test.go b/upgrader_process_test.go index 952875f..c4f7655 100644 --- a/upgrader_process_test.go +++ b/upgrader_process_test.go @@ -6,7 +6,6 @@ import ( "context" "fmt" "io" - "io/ioutil" "net" "net/http" "os" @@ -33,15 +32,13 @@ func loopbackTCPAddr(t *testing.T) string { } func TestBasicProcessUpgrade(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - tmpdir, cleanup := tmpDir() - defer cleanup() + ctx := testCtx(t) + tmpdir := tmpDir(t) testAddr := loopbackTCPAddr(t) t.Logf("running with addr %v", testAddr) - stdout, errC, exitC := runHelper(t, ctx, tmpdir, "main1", testAddr) + stdout, errC, exitC := runHelper(ctx, tmpdir, "main1", testAddr) select { case msg := <-stdout: @@ -77,7 +74,7 @@ func TestBasicProcessUpgrade(t *testing.T) { } // Now pass fds to process 2 - stdout2, errC2, exitC2 := runHelper(t, ctx, tmpdir, "main1", testAddr) + stdout2, errC2, exitC2 := runHelper(ctx, tmpdir, "main1", testAddr) // process 1 should exit select { @@ -128,13 +125,11 @@ func TestBasicProcessUpgrade(t *testing.T) { } func TestUnixMultiProcessUpgrade(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - tmpdir, cleanup := tmpDir() - defer cleanup() + ctx := testCtx(t) + tmpdir := tmpDir(t) sock := filepath.Join(tmpdir, "testsock") - stdout, errC, exitC := runHelper(t, ctx, tmpdir, "main2", "") + stdout, errC, exitC := runHelper(ctx, tmpdir, "main2", "") select { case msg := <-stdout: @@ -152,7 +147,7 @@ func TestUnixMultiProcessUpgrade(t *testing.T) { if err != nil { t.Fatalf("expected no error in get") } - data, _ := ioutil.ReadAll(conn) + data, _ := io.ReadAll(conn) if string(data) != "hello world" { t.Fatalf("expected hello world, got %s", data) } @@ -172,7 +167,7 @@ func TestUnixMultiProcessUpgrade(t *testing.T) { // now pass fds through 10 more processes for i := 0; i < 10; i++ { // Now pass fds to process n - stdoutn, errCn, exitCn := runHelper(t, ctx, tmpdir, "main2", "") + stdoutn, errCn, exitCn := runHelper(ctx, tmpdir, "main2", "") // process n-1 should exit exit := <-prevExit @@ -197,7 +192,7 @@ func TestUnixMultiProcessUpgrade(t *testing.T) { if err != nil { t.Fatalf("expected no error in get") } - data, _ = ioutil.ReadAll(conn) + data, _ = io.ReadAll(conn) if string(data) != "hello world" { t.Fatalf("expected hello world, got %s", data) } @@ -220,10 +215,9 @@ func TestUnixMultiProcessUpgrade(t *testing.T) { func TestMaxSocketUpg(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - tmpdir, cleanup := tmpDir() - defer cleanup() + tmpdir := tmpDir(t) - stdout, errC, exitC1 := runHelper(t, ctx, tmpdir, "listenOnManySockets", "") + stdout, errC, exitC1 := runHelper(ctx, tmpdir, "listenOnManySockets", "") select { case msg := <-stdout: if msg != MsgReady { @@ -235,7 +229,7 @@ func TestMaxSocketUpg(t *testing.T) { t.Fatalf("unexpected exit: %v", exit) } - stdout, errC, exitC := runHelper(t, ctx, tmpdir, "listenOnManySockets", "") + stdout, errC, exitC := runHelper(ctx, tmpdir, "listenOnManySockets", "") select { case msg := <-stdout: if msg != MsgReady { @@ -257,16 +251,16 @@ func TestMaxSocketUpg(t *testing.T) { // These are used to allow integration style testing of multiple processes. // This function takes a tableroll directory and function name to execute, and // optionally a tcp address to listen on which is used by certain tests. -func runHelper(t *testing.T, ctx context.Context, dir string, funcName string, addr string) (<-chan string, <-chan error, <-chan int) { +func runHelper(ctx context.Context, dir string, funcName string, addr string) (<-chan string, <-chan error, <-chan int) { child := exec.CommandContext(ctx, os.Args[0], "-test.run=TestSpawnHelper", "--") stderr, _ := child.StderrPipe() stdout, _ := child.StdoutPipe() var stderrBuffer bytes.Buffer - stderrEOF := make(chan struct{}) + stderrEOF := make(chan error, 1) go func() { - io.Copy(&stderrBuffer, stderr) - stderrEOF <- struct{}{} + _, err := io.Copy(&stderrBuffer, stderr) + stderrEOF <- err }() child.Env = append(child.Env, []string{ @@ -292,7 +286,9 @@ func runHelper(t *testing.T, ctx context.Context, dir string, funcName string, a }() go func() { err := child.Run() - <-stderrEOF + if err := <-stderrEOF; err != nil { + errorChan <- fmt.Errorf("error getting stderrEOF: %w", err) + } if stderrBuffer.Len() != 0 { errorChan <- fmt.Errorf("stderr: %v", stderrBuffer.String()) } @@ -346,10 +342,13 @@ func main1() int { server := &http.Server{ Handler: http.HandlerFunc(func(r http.ResponseWriter, req *http.Request) { fmt.Println(MsgServedRequest) - r.Write([]byte(fmt.Sprintf("hello from %v!\n", os.Getpid()))) + _, err := r.Write([]byte(fmt.Sprintf("hello from %v!\n", os.Getpid()))) + if err != nil { + panic(err) + } }), } - go server.Serve(ln) + go func() { _ = server.Serve(ln) }() if err := upg.Ready(); err != nil { fmt.Fprintf(os.Stderr, "%v", err) @@ -385,7 +384,10 @@ func main2() int { if err != nil { return } - conn.Write([]byte("hello world")) + _, err = conn.Write([]byte("hello world")) + if err != nil { + panic(err) + } conn.Close() fmt.Println(MsgServedRequest) } @@ -447,8 +449,9 @@ func listenOnManySockets() int { } // free up ids to free ip FDs for other tests for _, id := range ids { - upg.Fds.Remove(id) - + if err := upg.Fds.Remove(id); err != nil { + panic(err) + } } return 0 } diff --git a/upgrader_test.go b/upgrader_test.go index 537edd1..a7bed41 100644 --- a/upgrader_test.go +++ b/upgrader_test.go @@ -2,7 +2,7 @@ package tableroll import ( "context" - "io/ioutil" + "io" "math/rand" "net" "net/http" @@ -21,16 +21,6 @@ import ( var l = log15.New() -func tmpDir() (string, func()) { - dir, err := ioutil.TempDir("", "tableroll_test") - if err != nil { - panic(err) - } - return dir, func() { - os.RemoveAll(dir) - } -} - // TestGCingUpgradeHandoff tests that the upgradehandoff test works even with // gc running more frequently. // This test exists because there was a point when `fd`s were owned by two file @@ -59,8 +49,7 @@ func TestGCingUpgradeHandoff(t *testing.T) { // TestUpgradeHandoff tests the happy path flow of two servers handing off the listening socket. // It includes one client connection that spans the listener handoff and must be drained. func TestUpgradeHandoff(t *testing.T) { - coordDir, cleanup := tmpDir() - defer cleanup() + coordDir := tmpDir(t) // Server 1 starts listening server1Reqs, server1Msgs, upg1, s1 := createTestServer(t, clock.RealClock{}, 1, coordDir) @@ -105,19 +94,14 @@ func TestUpgradeHandoff(t *testing.T) { } func TestMutableUpgrading(t *testing.T) { - coordDir, cleanup := tmpDir() - defer cleanup() + coordDir := tmpDir(t) upg1, err := newUpgrader(context.Background(), clock.RealClock{}, coordDir, "1", WithLogger(l)) - if err != nil { - t.Fatalf("error creating upgrader: %v", err) - } - if err := upg1.Ready(); err != nil { - t.Fatalf("error marking ready: %v", err) - } + require.NoError(t, err) + require.NoError(t, upg1.Ready()) defer upg1.Stop() - upgradeDone := make(chan struct{}) + upgradeDone := make(chan error, 1) expectedFis := map[string]*os.File{} // Mutably add a bunch of fds to the store at random, make sure that all the ones that were added without error are inherited go func() { @@ -130,13 +114,15 @@ func TestMutableUpgrading(t *testing.T) { if err = upg1.Fds.Remove(id); err == nil { delete(expectedFis, id) } else if err != ErrUpgradeInProgress && err != ErrUpgradeCompleted { - t.Fatalf("unexpected error: %v", err) + upgradeDone <- err + return } } else { if fi, err = upg1.Fds.OpenFileWith(id, id, memoryOpenFile); err == nil { expectedFis[id] = fi } else if err != ErrUpgradeInProgress && err != ErrUpgradeCompleted { - t.Fatalf("unexpected error: %v", err) + upgradeDone <- err + return } } } @@ -144,19 +130,15 @@ func TestMutableUpgrading(t *testing.T) { }() upg2, err := newUpgrader(context.Background(), clock.RealClock{}, coordDir, "2", WithLogger(l)) - if err != nil { - t.Fatalf("error creating upgrader: %v", err) - } - if err := upg2.Ready(); err != nil { - t.Fatalf("error marking ready: %v", err) - } + require.NoError(t, err) + require.NoError(t, upg2.Ready()) // we expect that upg1 should have gotten a terminal error and we should have // got the full set of ids it thinks it stored - <-upgradeDone + require.NoError(t, <-upgradeDone) for id, expectedFi := range expectedFis { - if fi, err := upg2.Fds.File(id); fi != fi || err != nil { + if fi, err := upg2.Fds.File(id); err != nil { t.Errorf("expected upg2 to have file for %v of %#v, but had file %#v, err %v", id, expectedFi, fi, err) } } @@ -168,14 +150,7 @@ func TestMutableUpgrading(t *testing.T) { // TestPIDReuse verifies that if a new server gets a pid of a previous server, // it can still listen on the `${pid}.sock` socket correctly. func TestPIDReuse(t *testing.T) { - coordDir, err := ioutil.TempDir("", "tableroll_test") - if err != nil { - panic(err) - } - defer os.RemoveAll(coordDir) - - server1Msgs, server2Msgs := make(chan string), make(chan string) - server1Reqs, server2Reqs := make(chan struct{}), make(chan struct{}) + coordDir := tmpDir(t) // Server 1 starts listening server1Reqs, server1Msgs, upg1, s1 := createTestServer(t, clock.RealClock{}, 1, coordDir) @@ -229,8 +204,7 @@ func TestPIDReuse(t *testing.T) { func TestFdPassMultipleTimes(t *testing.T) { ctx := context.Background() clock := fakeclock.NewFakeClock(time.Now()) - coordDir, cleanup := tmpDir() - defer cleanup() + coordDir := tmpDir(t) server1Reqs, server1Msgs, upg1, s1 := createTestServer(t, clock, 1, coordDir) defer upg1.Stop() @@ -245,33 +219,32 @@ func TestFdPassMultipleTimes(t *testing.T) { assertResp(t, s1.URL, c1, "msg1") // s1 listening - syncUpgraderTimeout := make(chan struct{}) + syncUpgraderTimeout := make(chan error, 1) go func() { // Now make an s2 that fails to ready-up upg2, err := newUpgrader(ctx, clock, coordDir, "2", WithLogger(l)) if err != nil { - t.Fatalf("expected no error creating upgrader: %v", err) + syncUpgraderTimeout <- err + return } // now the upgrader is connected, but not 'Ready', so the server should be // waiting on a ready timeout soon - syncUpgraderTimeout <- struct{}{} + syncUpgraderTimeout <- nil // wait for the server to get a timeout before we stop, otherwise // `HasWaiters` could deadlock due to `Stop` causing the server to not // timeout <-syncUpgraderTimeout upg2.Stop() }() - <-syncUpgraderTimeout + require.NoError(t, <-syncUpgraderTimeout) // wait for the upgrade server to start waiting for a ready, then skip // forward 3 minutes since the timeout is 2 minutes by default for !clock.HasWaiters() { time.Sleep(1 * time.Millisecond) } clock.Step(3 * time.Minute) - syncUpgraderTimeout <- struct{}{} + close(syncUpgraderTimeout) - server3Msgs := make(chan string) - server3Reqs := make(chan struct{}) // now see that we get a working s3 server3Reqs, server3Msgs, upg3, s3 := createTestServer(t, clock, 3, coordDir) defer upg3.Stop() @@ -290,51 +263,35 @@ func TestFdPassMultipleTimes(t *testing.T) { // TestUpgradeHandoffCloseCtx closes the 'New' context as soon as possible and // verifies that the context was only for instantiation. func TestUpgradeHandoffCloseCtx(t *testing.T) { - coordDir, cleanup := tmpDir() - defer cleanup() + coordDir := tmpDir(t) ctx1, cancel1 := context.WithTimeout(context.Background(), 1*time.Second) upg1, err := newUpgrader(ctx1, clock.RealClock{}, coordDir, "1", WithLogger(l)) - if err != nil { - t.Fatalf("error creating upgrader: %v", err) - } + require.NoError(t, err) defer upg1.Stop() cancel1() - if err := upg1.Ready(); err != nil { - t.Fatalf("unable to mark self as ready: %v", err) - } + require.NoError(t, upg1.Ready()) ctx2, cancel2 := context.WithTimeout(context.Background(), 5*time.Second) upg2, err := newUpgrader(ctx2, clock.RealClock{}, coordDir, "2", WithLogger(l)) - if err != nil { - t.Fatalf("error creating upgrader: %v", err) - } + require.NoError(t, err) defer upg2.Stop() cancel2() - if err := upg2.Ready(); err != nil { - t.Fatalf("unable to mark self as ready: %v", err) - } + require.NoError(t, upg2.Ready()) } func TestUpgradeTimeout(t *testing.T) { ctx := context.Background() clock := fakeclock.NewFakeClock(time.Now()) - coordDir, cleanup := tmpDir() - defer cleanup() + coordDir := tmpDir(t) // If upg1 times out serving the upgrade, upg2 should not be able to think it's the owner upg1, err := newUpgrader(ctx, clock, coordDir, "1", WithLogger(l.New("pid", "1")), WithUpgradeTimeout(30*time.Millisecond)) - if err != nil { - t.Fatalf("error creating upgrader: %v", err) - } - if err := upg1.Ready(); err != nil { - t.Fatalf("unable to mark self as ready: %v", err) - } + require.NoError(t, err) + require.NoError(t, upg1.Ready()) upg2, err := newUpgrader(ctx, clock, coordDir, "2", WithLogger(l.New("pid", "2"))) - if err != nil { - t.Fatalf("error creating upgrader: %v", err) - } + require.NoError(t, err) // upg1 serve timeout for !clock.HasWaiters() { time.Sleep(1 * time.Millisecond) @@ -356,14 +313,13 @@ func TestUpgradeTimeout(t *testing.T) { // deadlocking. func TestFailedUpgradeListen(t *testing.T) { ctx := context.Background() - coordDir, cleanup := tmpDir() - defer cleanup() + coordDir := tmpDir(t) upg1, err := newUpgrader(ctx, clock.RealClock{}, coordDir, "1", WithLogger(l.New("pid", "1"))) require.Nil(t, err) ln, err := upg1.Fds.Listen(ctx, "id", &net.ListenConfig{}, "tcp", "127.0.0.1:0") require.Nil(t, err) - upg1.Ready() + require.NoError(t, upg1.Ready()) // fail an upgrade upg2, err := newUpgrader(ctx, clock.RealClock{}, coordDir, "2", WithLogger(l.New("pid", "2"))) @@ -384,16 +340,10 @@ func TestFailedUpgradeListen(t *testing.T) { func assertResp(t *testing.T, url string, c *http.Client, expected string) { resp, err := c.Get(url) - if err != nil { - t.Fatalf("error using test server 1: %v", err) - } - respData, err := ioutil.ReadAll(resp.Body) - if err != nil { - t.Fatalf("error reading body: %v", err) - } - if expected != string(respData) { - t.Fatalf("expected %s, got %s", expected, string(respData)) - } + require.NoError(t, err) + respData, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, expected, string(respData)) } func createTestServer(t *testing.T, clock clock.Clock, pid int, coordDir string) (chan struct{}, chan string, *Upgrader, *httptest.Server) { @@ -405,23 +355,20 @@ func createTestServer(t *testing.T, clock clock.Clock, pid int, coordDir string) requests <- struct{}{} // And now respond, as requested by the test harness resp := <-responses - w.Write([]byte(resp)) + _, err := w.Write([]byte(resp)) + if err != nil { + panic(err) + } })) upg, err := newUpgrader(context.Background(), clock, coordDir, strconv.Itoa(pid), WithLogger(l)) - if err != nil { - t.Fatalf("error creating upgrader: %v", err) - } + require.NoError(t, err) listen, err := upg.Fds.Listen(context.Background(), "testListen", nil, "tcp", "127.0.0.1:0") - if err != nil { - t.Fatalf("unable to listen: %v", err) - } + require.NoError(t, err) server.Listener = listen server.Start() - if err := upg.Ready(); err != nil { - t.Fatalf("unable to mark self as ready: %v", err) - } + require.NoError(t, upg.Ready()) return requests, responses, upg, server } diff --git a/utils_test.go b/utils_test.go new file mode 100644 index 0000000..d21f025 --- /dev/null +++ b/utils_test.go @@ -0,0 +1,24 @@ +package tableroll + +import ( + "context" + "os" + "testing" +) + +func tmpDir(t *testing.T) string { + dir, err := os.MkdirTemp("", "tableroll_test") + if err != nil { + panic(err) + } + t.Cleanup(func() { + os.RemoveAll(dir) + }) + return dir +} + +func testCtx(t *testing.T) context.Context { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + return ctx +} From 9715e16b1718db7e91a9c9ab612397e478c2d19a Mon Sep 17 00:00:00 2001 From: Euan Kemp Date: Wed, 24 Jul 2024 06:16:57 +0000 Subject: [PATCH 6/6] .github: update workflows to use flake.nix go version etc And also run lints. This is cribbed from the ngrok-go repo --- .envrc | 7 +++++++ .github/workflows/go.yml | 25 ------------------------- .github/workflows/lint.yml | 32 ++++++++++++++++++++++++++++++++ .github/workflows/test.yml | 22 ++++++++++++++++++++++ .gitignore | 1 + dup_file.go | 1 + dup_file_legacy.go | 1 + 7 files changed, 64 insertions(+), 25 deletions(-) create mode 100644 .envrc delete mode 100644 .github/workflows/go.yml create mode 100644 .github/workflows/lint.yml create mode 100644 .github/workflows/test.yml create mode 100644 .gitignore diff --git a/.envrc b/.envrc new file mode 100644 index 0000000..29601f8 --- /dev/null +++ b/.envrc @@ -0,0 +1,7 @@ +if ! has nix_direnv_version || ! nix_direnv_version 2.2.0; then + source_url "https://raw.githubusercontent.com/nix-community/nix-direnv/2.2.0/direnvrc" "sha256-5EwyKnkJNQeXrRkYbwwRBcXbibosCJqyIUuz9Xq+LRc=" +fi +dotenv_if_exists +GOPATH=`pwd`/.direnv/go +PATH=${GOPATH}/bin:${PATH} +use flake diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml deleted file mode 100644 index ce0ad4a..0000000 --- a/.github/workflows/go.yml +++ /dev/null @@ -1,25 +0,0 @@ -name: Go - -on: - push: - branches: [ master ] - pull_request: - branches: [ master ] - -jobs: - - build: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - - name: Set up Go - uses: actions/setup-go@v2 - with: - go-version: 1.16 - - - name: Build - run: go build -v ./... - - - name: Test - run: go test -race -v ./... diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000..9e3b757 --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,32 @@ +name: lint +on: + push: + branches: [ "main" ] + pull_request: + branches: [ "main" ] +permissions: + contents: read + # Optional: allow read access to pull request. Use with `only-new-issues` option. + # pull-requests: read +jobs: + lint: + name: lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 2 + - uses: cachix/install-nix-action@v18 + - uses: HatsuneMiku3939/direnv-action@v1 + - name: direnv allow + run: direnv allow . + - name: Run goimports + shell: bash + run: | + shopt -s globstar + direnv exec . goimports -format-only -w -local github.com/ngrok/tableroll **/*.go + - name: Lint + run: direnv exec . golangci-lint run . + - name: Check diff + shell: bash + run: git diff --exit-code diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..7209c7f --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,22 @@ +name: Go +on: + push: + branches: [ "main" ] + pull_request: + branches: [ "main" ] +jobs: + build-and-test: + runs-on: ubuntu-latest + env: + NGROK_TEST_ONLINE: 1 + NGROK_TEST_LONG: 1 + NGROK_TEST_FLAKEY: 1 + NGROK_AUTHTOKEN: ${{ secrets.NGROK_AUTHTOKEN }} + steps: + - uses: actions/checkout@v3 + - uses: cachix/install-nix-action@v18 + - uses: HatsuneMiku3939/direnv-action@v1 + - name: direnv allow + run: direnv allow . + - name: Test + run: direnv exec . go test -v ./... diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..7ad6275 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +/.direnv diff --git a/dup_file.go b/dup_file.go index c48e7d1..476c33a 100644 --- a/dup_file.go +++ b/dup_file.go @@ -1,3 +1,4 @@ +//go:build go1.12 // +build go1.12 package tableroll diff --git a/dup_file_legacy.go b/dup_file_legacy.go index 57ce1b8..06161e4 100644 --- a/dup_file_legacy.go +++ b/dup_file_legacy.go @@ -1,3 +1,4 @@ +//go:build !go1.12 // +build !go1.12 package tableroll