From 79ae62801f62e13b23de543659f6f764594e2b19 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 30 Apr 2019 14:55:38 +0200 Subject: [PATCH 1/9] ListenRangeConfig: default network ( protocol ) to tcp --- common/net/configure_port.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/common/net/configure_port.go b/common/net/configure_port.go index dbce7ee8f..1d6050081 100644 --- a/common/net/configure_port.go +++ b/common/net/configure_port.go @@ -40,7 +40,7 @@ func (l *Listener) Close() error { // ListenRangeConfig contains options for listening to a free address [Min,Max) // range. ListenRangeConfig wraps a net.ListenConfig. type ListenRangeConfig struct { - // tcp", "udp" + // like "tcp" or "udp". defaults to "tcp". Network string Addr string Min, Max int @@ -51,6 +51,9 @@ type ListenRangeConfig struct { // until ctx is cancelled. // Listen uses net.ListenConfig.Listen internally. func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { + if lc.Network == "" { + lc.Network = "tcp" + } portRange := lc.Max - lc.Min for { if err := ctx.Err(); err != nil { From fd63ec9a6c22239514a07146dd8500ac4fcf0f41 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 30 Apr 2019 15:35:22 +0200 Subject: [PATCH 2/9] ListenRangeConfig.Listen: move trying port log after fslock trylock for less logs --- common/net/configure_port.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/net/configure_port.go b/common/net/configure_port.go index 1d6050081..51765fda6 100644 --- a/common/net/configure_port.go +++ b/common/net/configure_port.go @@ -65,8 +65,6 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { port += rand.Intn(portRange) } - log.Printf("Trying port: %d", port) - lockFilePath, err := packer.CachePath("port", strconv.Itoa(port)) if err != nil { return nil, err @@ -81,6 +79,8 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { continue // this port seems to be locked by another packer goroutine } + log.Printf("Trying port: %d", port) + l, err := lc.ListenConfig.Listen(ctx, lc.Network, fmt.Sprintf("%s:%d", lc.Addr, port)) if err != nil { if err := lock.Unlock(); err != nil { From 5d9d43c01da56c41eb3058fcee91247194848308 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 30 Apr 2019 15:42:59 +0200 Subject: [PATCH 3/9] add tests for ListenRangeConfig.Listen --- common/net/configure_port_test.go | 155 ++++++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 common/net/configure_port_test.go diff --git a/common/net/configure_port_test.go b/common/net/configure_port_test.go new file mode 100644 index 000000000..5117a70d0 --- /dev/null +++ b/common/net/configure_port_test.go @@ -0,0 +1,155 @@ +package net + +import ( + "context" + "net" + "testing" + "time" +) + +func TestListenRangeConfig_Listen(t *testing.T) { + + topCtx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var err error + var lockedListener *Listener + { // open a random port in range + ctx, cancel := context.WithTimeout(topCtx, time.Second*5) + + lockedListener, err = ListenRangeConfig{ + Min: 800, + Max: 10000, + Addr: "localhost", + }.Listen(ctx) + if err != nil { + t.Fatalf("could not open first port") + } + cancel() + defer lockedListener.Close() // in case + } + + { // open a second random port in range + ctx, cancel := context.WithTimeout(topCtx, time.Second*5) + + listener, err := ListenRangeConfig{ + Min: 800, + Max: 10000, + Addr: "localhost", + }.Listen(ctx) + if err != nil { + t.Fatalf("could not open first port") + } + cancel() + if err := listener.Close(); err != nil { // in case + t.Fatal("failed to close second random port") + } + } + + { // test that opened port cannot be openned using min/max + ctx, cancel := context.WithTimeout(topCtx, 250*time.Millisecond) + + l, err := ListenRangeConfig{ + Min: lockedListener.Port, + Max: lockedListener.Port, + }.Listen(ctx) + if err != context.DeadlineExceeded { + l.Close() + t.Fatalf("port should be taken, this should timeout: %v", err) + } + cancel() + } + + { // test that opened port cannot be openned using min only + ctx, cancel := context.WithTimeout(topCtx, 250*time.Millisecond) + + l, err := ListenRangeConfig{ + Min: lockedListener.Port, + }.Listen(ctx) + if err != context.DeadlineExceeded { + l.Close() + t.Fatalf("port should be taken, this should timeout: %v", err) + } + cancel() + } + + err = lockedListener.Close() // close port and release lock file + if err != nil { + t.Fatalf("could not release lockfile or port: %v", err) + } + + { // test that closed port can be reopenned. + ctx, cancel := context.WithTimeout(topCtx, 250*time.Millisecond) + + lockedListener, err = ListenRangeConfig{ + Min: lockedListener.Port, + }.Listen(ctx) + if err != nil { + t.Fatalf("port should have been freed: %v", err) + } + cancel() + defer lockedListener.Close() // in case + } + + err = lockedListener.Listener.Close() // close listener, keep lockfile only + if err != nil { + t.Fatalf("could not release lockfile or port: %v", err) + } + + { // test that file locked port cannot be opened + ctx, cancel := context.WithTimeout(topCtx, 250*time.Millisecond) + + l, err := ListenRangeConfig{ + Min: lockedListener.Port, + }.Listen(ctx) + if err != context.DeadlineExceeded { + l.Close() + t.Fatalf("port should be file locked, this should timeout: %v", err) + } + cancel() + } + + var netListener net.Listener + { // test that network port was closed. using net.Listen + netListener, err = net.Listen("tcp", lockedListener.Addr().String()) + if err != nil { + t.Fatalf("listen on freed port failed: %v", err) + } + + } + + if err := lockedListener.lock.Unlock(); err != nil { + t.Fatalf("error closing port: %v", err) + } + + { // test that busy port cannot be opened + ctx, cancel := context.WithTimeout(topCtx, 250*time.Millisecond) + + l, err := ListenRangeConfig{ + Min: lockedListener.Port, + }.Listen(ctx) + if err != context.DeadlineExceeded { + l.Close() + t.Fatalf("port should be file locked, this should timeout: %v", err) + } + cancel() + } + + if err := netListener.Close(); err != nil { // free port + t.Fatalf("close failed: %v", err) + } + + { // test that freed port can be opened + ctx, cancel := context.WithTimeout(topCtx, 250*time.Minute) + + lockedListener, err = ListenRangeConfig{ + Min: lockedListener.Port, + }.Listen(ctx) + if err != nil { + t.Fatalf("port should have been freed: %v", err) + } + cancel() + defer lockedListener.Close() // in case + } + +} From d9e5145de9aa8d38574961e9f7cd23e72bdb7c1f Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 30 Apr 2019 16:01:23 +0200 Subject: [PATCH 4/9] Shadow the flock pkg & add a noop filelock so that at least the solaris binary can be built Without this the following error occurs: $ GOOS=solaris go build . # github.com/hashicorp/packer/vendor/github.com/gofrs/flock vendor/github.com/gofrs/flock/flock_unix.go:28:22: undefined: syscall.LOCK_EX vendor/github.com/gofrs/flock/flock_unix.go:39:22: undefined: syscall.LOCK_SH vendor/github.com/gofrs/flock/flock_unix.go:56:12: undefined: syscall.Flock vendor/github.com/gofrs/flock/flock_unix.go:66:12: undefined: syscall.Flock vendor/github.com/gofrs/flock/flock_unix.go:96:12: undefined: syscall.Flock vendor/github.com/gofrs/flock/flock_unix.go:96:42: undefined: syscall.LOCK_UN vendor/github.com/gofrs/flock/flock_unix.go:118:21: undefined: syscall.LOCK_EX vendor/github.com/gofrs/flock/flock_unix.go:130:21: undefined: syscall.LOCK_SH vendor/github.com/gofrs/flock/flock_unix.go:149:9: undefined: syscall.Flock vendor/github.com/gofrs/flock/flock_unix.go:149:44: undefined: syscall.LOCK_NB vendor/github.com/gofrs/flock/flock_unix.go:149:44: too many errors --- common/filelock/filelock.go | 11 +++++++++++ common/filelock/filelock_solaris.go | 11 +++++++++++ common/filelock/noop.go | 8 ++++++++ common/net/configure_port.go | 7 +++---- common/step_download.go | 4 ++-- 5 files changed, 35 insertions(+), 6 deletions(-) create mode 100644 common/filelock/filelock.go create mode 100644 common/filelock/filelock_solaris.go create mode 100644 common/filelock/noop.go diff --git a/common/filelock/filelock.go b/common/filelock/filelock.go new file mode 100644 index 000000000..c83816ee6 --- /dev/null +++ b/common/filelock/filelock.go @@ -0,0 +1,11 @@ +// +build !solaris + +package filelock + +import "github.com/gofrs/flock" + +type Flock = flock.Flock + +func New(path string) *Flock { + return flock.New(path) +} diff --git a/common/filelock/filelock_solaris.go b/common/filelock/filelock_solaris.go new file mode 100644 index 000000000..06685254c --- /dev/null +++ b/common/filelock/filelock_solaris.go @@ -0,0 +1,11 @@ +// build solaris + +package filelock + +// Flock is a noop on solaris for now. +// TODO(azr): PR github.com/gofrs/flock for this. +type Flock = Noop + +func New(string) *Flock { + return &Flock{} +} diff --git a/common/filelock/noop.go b/common/filelock/noop.go new file mode 100644 index 000000000..ebf8f1967 --- /dev/null +++ b/common/filelock/noop.go @@ -0,0 +1,8 @@ +package filelock + +// this lock does nothing +type Noop struct{} + +func (_ *Noop) Lock() (bool, error) { return true, nil } +func (_ *Noop) TryLock() (bool, error) { return true, nil } +func (_ *Noop) Unlock() error { return nil } diff --git a/common/net/configure_port.go b/common/net/configure_port.go index 51765fda6..cdc0a9168 100644 --- a/common/net/configure_port.go +++ b/common/net/configure_port.go @@ -8,8 +8,7 @@ import ( "net" "strconv" - "github.com/gofrs/flock" - + "github.com/hashicorp/packer/common/filelock" "github.com/hashicorp/packer/packer" ) @@ -26,7 +25,7 @@ type Listener struct { net.Listener Port int Address string - lock *flock.Flock + lock *filelock.Flock } func (l *Listener) Close() error { @@ -70,7 +69,7 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { return nil, err } - lock := flock.New(lockFilePath) + lock := filelock.New(lockFilePath) locked, err := lock.TryLock() if err != nil { return nil, err diff --git a/common/step_download.go b/common/step_download.go index e562b619c..af7708e6e 100644 --- a/common/step_download.go +++ b/common/step_download.go @@ -10,9 +10,9 @@ import ( "runtime" "strings" - "github.com/gofrs/flock" getter "github.com/hashicorp/go-getter" urlhelper "github.com/hashicorp/go-getter/helper/url" + "github.com/hashicorp/packer/common/filelock" "github.com/hashicorp/packer/helper/multistep" "github.com/hashicorp/packer/packer" ) @@ -146,7 +146,7 @@ func (s *StepDownload) download(ctx context.Context, ui packer.Ui, source string lockFile := targetPath + ".lock" log.Printf("Acquiring lock for: %s (%s)", u.String(), lockFile) - lock := flock.New(lockFile) + lock := filelock.New(lockFile) lock.Lock() defer lock.Unlock() From 2fd7adffc4120ef5a39731265d8880744933a9a7 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 1 May 2019 11:46:48 +0200 Subject: [PATCH 5/9] add tests for ListenRangeConfig.Listen --- common/net/configure_port_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/net/configure_port_test.go b/common/net/configure_port_test.go index 5117a70d0..8f01ade46 100644 --- a/common/net/configure_port_test.go +++ b/common/net/configure_port_test.go @@ -53,8 +53,10 @@ func TestListenRangeConfig_Listen(t *testing.T) { Min: lockedListener.Port, Max: lockedListener.Port, }.Listen(ctx) - if err != context.DeadlineExceeded { + if l != nil { l.Close() + } + if err != context.DeadlineExceeded { t.Fatalf("port should be taken, this should timeout: %v", err) } cancel() From e14d1b8b0bdf2da2e40e46175249d260c36cec1d Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 1 May 2019 12:01:34 +0200 Subject: [PATCH 6/9] add a timeout trying to open a random port --- common/net/configure_port.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/net/configure_port.go b/common/net/configure_port.go index cdc0a9168..fddd4b5d6 100644 --- a/common/net/configure_port.go +++ b/common/net/configure_port.go @@ -7,6 +7,7 @@ import ( "math/rand" "net" "strconv" + "time" "github.com/hashicorp/packer/common/filelock" "github.com/hashicorp/packer/packer" @@ -97,5 +98,6 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { lock: lock, }, err + time.Sleep(20 * time.Millisecond) } } From 14f2d1c132c4c4361e540f1fdc9d0b640169f279 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 1 May 2019 12:27:32 +0200 Subject: [PATCH 7/9] ListenRangeConfig:Listen: use the retry pkg to wait a bit in between opens --- common/net/configure_port.go | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/common/net/configure_port.go b/common/net/configure_port.go index fddd4b5d6..7982cf80d 100644 --- a/common/net/configure_port.go +++ b/common/net/configure_port.go @@ -10,6 +10,7 @@ import ( "time" "github.com/hashicorp/packer/common/filelock" + "github.com/hashicorp/packer/common/retry" "github.com/hashicorp/packer/packer" ) @@ -55,11 +56,12 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { lc.Network = "tcp" } portRange := lc.Max - lc.Min - for { - if err := ctx.Err(); err != nil { - return nil, err - } + var listener *Listener + + err := retry.Config{ + RetryDelay: func() time.Duration { return 20 * time.Millisecond }, + }.Run(ctx, func(context.Context) error { port := lc.Min if portRange > 0 { port += rand.Intn(portRange) @@ -67,16 +69,16 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { lockFilePath, err := packer.CachePath("port", strconv.Itoa(port)) if err != nil { - return nil, err + return err } lock := filelock.New(lockFilePath) locked, err := lock.TryLock() if err != nil { - return nil, err + return err } if !locked { - continue // this port seems to be locked by another packer goroutine + return fmt.Errorf("Port %d is file locked", port) } log.Printf("Trying port: %d", port) @@ -84,20 +86,19 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { l, err := lc.ListenConfig.Listen(ctx, lc.Network, fmt.Sprintf("%s:%d", lc.Addr, port)) if err != nil { if err := lock.Unlock(); err != nil { - log.Printf("Could not unlock file lock for port %d: %v", port, err) + log.Fatalf("Could not unlock file lock for port %d: %v", port, err) } - - continue // this port is most likely already open + return fmt.Errorf("Port %d cannot be opened: %v", port, err) } log.Printf("Found available port: %d on IP: %s", port, lc.Addr) - return &Listener{ + listener = &Listener{ Address: lc.Addr, Port: port, Listener: l, lock: lock, - }, err - - time.Sleep(20 * time.Millisecond) - } + } + return nil + }) + return listener, err } From 38f789eedc2d1b35be805df716791ebbaf107772 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 1 May 2019 13:01:25 +0200 Subject: [PATCH 8/9] add error types to test for --- common/net/configure_port.go | 27 ++++++++++++++++++++++++--- common/net/configure_port_test.go | 30 ++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/common/net/configure_port.go b/common/net/configure_port.go index 7982cf80d..f5bc60e62 100644 --- a/common/net/configure_port.go +++ b/common/net/configure_port.go @@ -60,7 +60,7 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { var listener *Listener err := retry.Config{ - RetryDelay: func() time.Duration { return 20 * time.Millisecond }, + RetryDelay: func() time.Duration { return 1 * time.Millisecond }, }.Run(ctx, func(context.Context) error { port := lc.Min if portRange > 0 { @@ -78,7 +78,7 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { return err } if !locked { - return fmt.Errorf("Port %d is file locked", port) + return ErrPortFileLocked(port) } log.Printf("Trying port: %d", port) @@ -88,7 +88,10 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { if err := lock.Unlock(); err != nil { log.Fatalf("Could not unlock file lock for port %d: %v", port, err) } - return fmt.Errorf("Port %d cannot be opened: %v", port, err) + return &ErrPortBusy{ + Port: port, + Err: err, + } } log.Printf("Found available port: %d on IP: %s", port, lc.Addr) @@ -102,3 +105,21 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { }) return listener, err } + +type ErrPortFileLocked int + +func (port ErrPortFileLocked) Error() string { + return fmt.Sprintf("Port %d is file locked", port) +} + +type ErrPortBusy struct { + Port int + Err error +} + +func (err *ErrPortBusy) Error() string { + if err == nil { + return "" + } + return fmt.Sprintf("port %d cannot be opened: %v", err.Port, err.Err) +} diff --git a/common/net/configure_port_test.go b/common/net/configure_port_test.go index 8f01ade46..e39173e7c 100644 --- a/common/net/configure_port_test.go +++ b/common/net/configure_port_test.go @@ -53,11 +53,12 @@ func TestListenRangeConfig_Listen(t *testing.T) { Min: lockedListener.Port, Max: lockedListener.Port, }.Listen(ctx) - if l != nil { + if err == nil { l.Close() + t.Fatal("port should be taken, this should fail") } - if err != context.DeadlineExceeded { - t.Fatalf("port should be taken, this should timeout: %v", err) + if p := int(err.(ErrPortFileLocked)); p != lockedListener.Port { + t.Fatalf("wrong fileport: %d", p) } cancel() } @@ -68,9 +69,12 @@ func TestListenRangeConfig_Listen(t *testing.T) { l, err := ListenRangeConfig{ Min: lockedListener.Port, }.Listen(ctx) - if err != context.DeadlineExceeded { + if err == nil { l.Close() - t.Fatalf("port should be taken, this should timeout: %v", err) + t.Fatalf("port should be taken, this should timeout.") + } + if p := int(err.(ErrPortFileLocked)); p != lockedListener.Port { + t.Fatalf("wrong fileport: %d", p) } cancel() } @@ -104,15 +108,15 @@ func TestListenRangeConfig_Listen(t *testing.T) { l, err := ListenRangeConfig{ Min: lockedListener.Port, }.Listen(ctx) - if err != context.DeadlineExceeded { + if err == nil { l.Close() - t.Fatalf("port should be file locked, this should timeout: %v", err) + t.Fatalf("port should be file locked, this should timeout") } cancel() } var netListener net.Listener - { // test that network port was closed. using net.Listen + { // test that the closed network port can be reopened using net.Listen netListener, err = net.Listen("tcp", lockedListener.Addr().String()) if err != nil { t.Fatalf("listen on freed port failed: %v", err) @@ -130,10 +134,16 @@ func TestListenRangeConfig_Listen(t *testing.T) { l, err := ListenRangeConfig{ Min: lockedListener.Port, }.Listen(ctx) - if err != context.DeadlineExceeded { + if err == nil { l.Close() - t.Fatalf("port should be file locked, this should timeout: %v", err) + t.Fatalf("port should be file locked, this should timeout") + } + busyErr := err.(*ErrPortBusy) + if busyErr.Port != lockedListener.Port { + t.Fatal("wrong port") } + // error types vary depending on OS and it might get quickly + // complicated to test for the error we want. cancel() } From b329073e54b2e1e905806a777123bd639805fc84 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 1 May 2019 13:04:51 +0200 Subject: [PATCH 9/9] remove trying port log as retry code will show errors less logs ! --- common/net/configure_port.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/common/net/configure_port.go b/common/net/configure_port.go index f5bc60e62..3de38be9c 100644 --- a/common/net/configure_port.go +++ b/common/net/configure_port.go @@ -81,8 +81,6 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { return ErrPortFileLocked(port) } - log.Printf("Trying port: %d", port) - l, err := lc.ListenConfig.Listen(ctx, lc.Network, fmt.Sprintf("%s:%d", lc.Addr, port)) if err != nil { if err := lock.Unlock(); err != nil {