From c94751ad4390d89b20635aa0297711aab2d0b2f9 Mon Sep 17 00:00:00 2001 From: Frank Schroeder Date: Sat, 21 Oct 2017 21:15:01 +0200 Subject: [PATCH] test: replace porter tool with freeport lib This patch removes the porter tool which hands out free ports from a given range with a library which does the same thing. The challenge for acquiring free ports in concurrent go test runs is that go packages are tested concurrently and run in separate processes. There has to be some inter-process synchronization in preventing processes allocating the same ports. freeport allocates blocks of ports from a range expected to be not in heavy use and implements a system-wide mutex by binding to the first port of that block for the lifetime of the application. Ports are then provided sequentially from that block and are tested on localhost before being returned as available. --- GNUmakefile | 8 +- agent/consul/server_test.go | 7 +- agent/testagent.go | 7 +- lib/freeport/freeport.go | 107 +++++++++++++++++++++++ test/porter/client.go | 61 ------------- test/porter/cmd/porter/main.go | 154 --------------------------------- testutil/porter_test.go | 13 --- testutil/server.go | 33 +------ 8 files changed, 115 insertions(+), 275 deletions(-) create mode 100644 lib/freeport/freeport.go delete mode 100644 test/porter/client.go delete mode 100644 test/porter/cmd/porter/main.go delete mode 100644 testutil/porter_test.go diff --git a/GNUmakefile b/GNUmakefile index 72351df5a7..d5d7ed3cf9 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -62,11 +62,11 @@ cov: gocov test $(GOFILES) | gocov-html > /tmp/coverage.html open /tmp/coverage.html -test: other-consul porter dev-build vet +test: other-consul dev-build vet @echo "--> Running go test" @rm -f test.log exit-code go test -tags '$(GOTAGS)' -i ./... - porter go test $(GOTEST_FLAGS) -tags '$(GOTAGS)' -timeout 5m -v ./... &>test.log ; echo $$? > exit-code + go test $(GOTEST_FLAGS) -tags '$(GOTAGS)' -timeout 5m -v ./... &>test.log ; echo $$? > exit-code @echo "Exit code: $$(cat exit-code)" >> test.log @grep -A5 'DATA RACE' test.log || true @grep -A10 'panic: test timed out' test.log || true @@ -86,10 +86,6 @@ other-consul: exit 1 ; \ fi -porter: - @echo "--> Building port number service..." - go install github.com/hashicorp/consul/test/porter/cmd/porter - cover: go test $(GOFILES) --cover diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index be347fe56c..9090b6e847 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -13,7 +13,7 @@ import ( "github.com/hashicorp/consul/agent/metadata" "github.com/hashicorp/consul/agent/token" - "github.com/hashicorp/consul/test/porter" + "github.com/hashicorp/consul/lib/freeport" "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/testutil" "github.com/hashicorp/consul/testutil/retry" @@ -41,10 +41,7 @@ func testServerConfig(t *testing.T) (string, *Config) { dir := testutil.TempDir(t, "consul") config := DefaultConfig() - ports, err := porter.RandomPorts(3) - if err != nil { - t.Fatal("RandomPorts:", err) - } + ports := freeport.Get(3) config.NodeName = uniqueNodeName(t.Name()) config.Bootstrap = true config.Datacenter = "dc1" diff --git a/agent/testagent.go b/agent/testagent.go index 4998587fc8..9b99cd6471 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -20,8 +20,8 @@ import ( "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/lib/freeport" "github.com/hashicorp/consul/logger" - "github.com/hashicorp/consul/test/porter" "github.com/hashicorp/consul/testutil/retry" uuid "github.com/hashicorp/go-uuid" ) @@ -297,10 +297,7 @@ func UniqueID() string { // Instead of relying on one set of ports to be sufficient we retry // starting the agent with different ports on port conflict. func randomPortsSource() config.Source { - ports, err := porter.RandomPorts(5) - if err != nil { - panic(err) - } + ports := freeport.Get(5) return config.Source{ Name: "ports", Format: "hcl", diff --git a/lib/freeport/freeport.go b/lib/freeport/freeport.go new file mode 100644 index 0000000000..08fdf62348 --- /dev/null +++ b/lib/freeport/freeport.go @@ -0,0 +1,107 @@ +// Package freeport provides a helper for allocating free ports across multiple +// processes on the same machine. +package freeport + +import ( + "math/rand" + "net" + "sync" + "time" +) + +const ( + // blockSize is the size of the allocated port block. ports are given out + // consecutively from that block with roll-over for the lifetime of the + // application/test run. + blockSize = 500 + + // maxBlocks is the number of available port blocks. + // lowPort + maxBlocks * blockSize must be less than 65535. + maxBlocks = 30 + + // lowPort is the lowest port number that should be used. + lowPort = 10000 + + // attempts is how often we try to allocate a port block + // before giving up. + attempts = 10 +) + +var ( + // firstPort is the first port of the allocated block. + firstPort int + + // lockLn is the system-wide mutex for the port block. + lockLn net.Listener + + // mu guards nextPort + mu sync.Mutex + + // port is the last allocated port. + port int +) + +func init() { + if lowPort+maxBlocks*blockSize > 65535 { + panic("freeport: block size too big or too many blocks requested") + } + + rand.Seed(time.Now().UnixNano()) + firstPort, lockLn = alloc() +} + +// alloc reserves a port block for exclusive use for the lifetime of the +// application. lockLn serves as a system-wide mutex for the port block and is +// implemented as a TCP listener which is bound to the firstPort and which will +// be automatically released when the application terminates. +func alloc() (int, net.Listener) { + for i := 0; i < attempts; i++ { + block := int(rand.Int31n(int32(maxBlocks))) + firstPort := lowPort + block*blockSize + ln, err := net.ListenTCP("tcp", tcpAddr("127.0.0.1", firstPort)) + if err != nil { + continue + } + // log.Printf("[DEBUG] freeport: allocated port block %d (%d-%d)", block, firstPort, firstPort+blockSize-1) + return firstPort, ln + } + panic("freeport: cannot allocate port block") +} + +func tcpAddr(ip string, port int) *net.TCPAddr { + return &net.TCPAddr{IP: net.ParseIP(ip), Port: port} +} + +// Get returns a list of free ports from the allocated port block. It is safe +// to call this method concurrently. Ports have been tested to be available on +// 127.0.0.1 TCP but there is no guarantee that they will remain free in the +// future. +func Get(n int) (ports []int) { + mu.Lock() + defer mu.Unlock() + + if n > blockSize-1 { + panic("freeport: block size too small") + } + + for len(ports) < n { + port++ + + // roll-over the port + if port < firstPort+1 || port >= firstPort+blockSize { + port = firstPort + 1 + } + + // if the port is in use then skip it + ln, err := net.ListenTCP("tcp", tcpAddr("127.0.0.1", port)) + if err != nil { + // log.Println("[DEBUG] freeport: port already in use: ", port) + continue + } + ln.Close() + + ports = append(ports, port) + } + // log.Println("[DEBUG] freeport: free ports:", ports) + return ports +} diff --git a/test/porter/client.go b/test/porter/client.go deleted file mode 100644 index 1c746e9be0..0000000000 --- a/test/porter/client.go +++ /dev/null @@ -1,61 +0,0 @@ -package porter - -import ( - "encoding/json" - "fmt" - "io/ioutil" - "net/http" - "os" - "strings" -) - -var ( - // DefaultAddr is the the default bind address of a Porter server. This acts - // as the fallback address if the Porter server is not specified. - DefaultAddr = "127.0.0.1:7965" -) - -const ( - // porterErrPrefix is the string returned when displaying a porter error - porterErrPrefix = `Are you running porter? -Install with 'go install github.com/hashicorp/consul/test/porter/cmd/porter' -Then run 'porter go test ...'` -) - -// PorterExistErr is used to wrap an error that is likely from Porter not being -// run. -type PorterExistErr struct { - Wrapped error -} - -func (p *PorterExistErr) Error() string { - return fmt.Sprintf("%s:\n%s", porterErrPrefix, p.Wrapped) -} - -func RandomPorts(n int) ([]int, error) { - addr := os.Getenv("PORTER_ADDR") - if addr == "" { - b, err := ioutil.ReadFile("/tmp/porter.addr") - if err == nil { - addr = string(b) - } - } - if addr == "" { - addr = DefaultAddr - } - resp, err := http.Get(fmt.Sprintf("http://%s/%d", addr, n)) - if err != nil { - if strings.Contains(err.Error(), "connection refused") { - return nil, &PorterExistErr{Wrapped: err} - } - return nil, err - } - defer resp.Body.Close() - data, err := ioutil.ReadAll(resp.Body) - if err != nil { - return nil, err - } - var p []int - err = json.Unmarshal(data, &p) - return p, err -} diff --git a/test/porter/cmd/porter/main.go b/test/porter/cmd/porter/main.go deleted file mode 100644 index ea1bdb29d6..0000000000 --- a/test/porter/cmd/porter/main.go +++ /dev/null @@ -1,154 +0,0 @@ -package main - -import ( - "encoding/json" - "flag" - "fmt" - "io/ioutil" - "log" - "net" - "net/http" - "os" - "os/exec" - "os/signal" - "strconv" - "sync" - - "github.com/hashicorp/consul/test/porter" -) - -var addrFile = "/tmp/porter.addr" - -var ( - addr string - firstPort int - lastPort int - verbose bool - - mu sync.Mutex - port int -) - -func main() { - log.SetFlags(0) - - flag.StringVar(&addr, "addr", porter.DefaultAddr, "host:port") - flag.IntVar(&firstPort, "first-port", 10000, "first port to allocate") - flag.IntVar(&lastPort, "last-port", 20000, "last port to allocate") - flag.BoolVar(&verbose, "verbose", false, "log port allocations") - flag.Parse() - - // check if there is an instance running - startServer := true - b, err := ioutil.ReadFile(addrFile) - if err == nil { - addr = string(b) - conn, err := net.Dial("tcp", addr) - if err == nil { - log.Println("found running porter instance at", addr) - startServer = false - conn.Close() - } else { - log.Printf("found dead porter instance at %s, will take over", addr) - } - } - - args := flag.Args() - if startServer { - if err := ioutil.WriteFile(addrFile, []byte(addr), 0644); err != nil { - log.Fatalf("Cannot write %s: %s", addrFile, err) - } - defer os.Remove(addrFile) - go func() { - http.HandleFunc("/", servePort) - if err := http.ListenAndServe(addr, nil); err != nil { - log.Fatal(err) - } - }() - } else { - if len(args) == 0 { - log.Println("no command and existing porter instance found, exiting") - os.Exit(0) - } - } - - // no command to run: wait for CTRL-C - if len(args) == 0 { - log.Print("PORTER_ADDR=" + addr) - c := make(chan os.Signal, 1) - signal.Notify(c, os.Interrupt) - s := <-c - log.Println("Got signal:", s) - return - } - - // run command and exit with 1 in case of error - if err := run(args); err != nil { - log.Fatal(err) - } -} - -func run(args []string) error { - path, err := exec.LookPath(args[0]) - if err != nil { - return fmt.Errorf("Cannot find %q in path", args[0]) - } - - cmd := exec.Command(path, args[1:]...) - if os.Getenv("PORTER_ADDR") == "" { - cmd.Env = append(os.Environ(), "PORTER_ADDR="+addr) - } - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - return cmd.Run() -} - -func servePort(w http.ResponseWriter, r *http.Request) { - var count int - n, err := strconv.Atoi(r.RequestURI[1:]) - if err == nil { - count = n - } - if count <= 0 { - count = 1 - } - - // getPort assumes the lock is already held and tries to return a port - // that's not in use. It will panic if it has to try too many times. - getPort := func() int { - for i := 0; i < 10; i++ { - port++ - if port < firstPort { - port = firstPort - } - if port >= lastPort { - port = firstPort - } - - conn, err := net.Dial("tcp", fmt.Sprintf("127.0.0.1:%d", port)) - if err != nil { - return port - } - conn.Close() - if verbose { - log.Printf("porter: skipping port %d, already in use", port) - } - } - panic(fmt.Errorf("could not find a free port")) - } - - p := make([]int, count) - mu.Lock() - for i := 0; i < count; i++ { - p[i] = getPort() - } - mu.Unlock() - - if err := json.NewEncoder(w).Encode(p); err != nil { - // this shouldn't happen so we panic since we can't recover - panic(err) - } - if verbose { - log.Printf("porter: allocated ports %v", p) - } -} diff --git a/testutil/porter_test.go b/testutil/porter_test.go deleted file mode 100644 index 8595b74b69..0000000000 --- a/testutil/porter_test.go +++ /dev/null @@ -1,13 +0,0 @@ -package testutil - -import ( - "testing" - - "github.com/hashicorp/consul/test/porter" -) - -func Test_PorterIsRunning(t *testing.T) { - if _, err := porter.RandomPorts(1); err != nil { - t.Fatalf("Porter must be running for Consul's unit tests") - } -} diff --git a/testutil/server.go b/testutil/server.go index 93b734a671..4993f13cb9 100644 --- a/testutil/server.go +++ b/testutil/server.go @@ -27,7 +27,7 @@ import ( "testing" "time" - "github.com/hashicorp/consul/test/porter" + "github.com/hashicorp/consul/lib/freeport" "github.com/hashicorp/consul/testutil/retry" "github.com/hashicorp/go-cleanhttp" "github.com/hashicorp/go-uuid" @@ -111,17 +111,7 @@ func defaultServerConfig() *TestServerConfig { panic(err) } - ports, err := porter.RandomPorts(6) - if err != nil { - if _, ok := err.(*porter.PorterExistErr); ok { - // Fall back in the case that the testutil server is being used - // without porter. This should NEVER be used for Consul's own - // unit tests. See comments for getRandomPorts() for more details. - ports = getRandomPorts(6) - } else { - panic(err) - } - } + ports := freeport.Get(6) return &TestServerConfig{ NodeName: "node-" + nodeID, NodeID: nodeID, @@ -390,22 +380,3 @@ func (s *TestServer) waitForLeader() error { } return nil } - -// getRandomPorts returns a set of random port or panics on error. This -// is here to support external uses of testutil which may not have porter, -// but this has been shown not to work well with parallel tests (such as -// Consul's unit tests). This fallback should NEVER be used for Consul's -// own tests. -func getRandomPorts(n int) []int { - ports := make([]int, n) - for i := 0; i < n; i++ { - l, err := net.Listen("tcp", ":0") - if err != nil { - panic(err) - } - l.Close() - ports[i] = l.Addr().(*net.TCPAddr).Port - } - - return ports -}