From b2232445174d36c6962415c24906c3efd66144b1 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Wed, 16 Nov 2022 12:05:50 +0100 Subject: [PATCH] Fix race in determining start port for cluster The logic here is racy since multiple endtoend tests can create the file at around the same time and allocate the same base port. This then leads to flaky failing endtoend tests since not all ports can be used. Even with the listening check later on, this is still racy because the listen check might run at a later point still trying to allocate the same port for the same service in different endtoend test. This copies the Go filelock implementation, but only for unix systems since that's the only supported platform for Vitess. Go has a proposal open to make filelock available, but that is still pending in https://github.com/golang/go/issues/33974. Once that is resolved, we can remove our copy of the implementation. Signed-off-by: Dirkjan Bussink --- dev.env | 2 +- go/test/endtoend/cluster/cluster_process.go | 44 +++++++-- go/test/endtoend/filelock/filelock.go | 99 +++++++++++++++++++++ go/test/endtoend/filelock/filelock_unix.go | 42 +++++++++ 4 files changed, 177 insertions(+), 10 deletions(-) create mode 100644 go/test/endtoend/filelock/filelock.go create mode 100644 go/test/endtoend/filelock/filelock_unix.go diff --git a/dev.env b/dev.env index 781f54be7ba..7426dde45f2 100644 --- a/dev.env +++ b/dev.env @@ -22,7 +22,7 @@ source ./build.env -export VTPORTSTART=15000 +export VTPORTSTART=6700 # Add chromedriver to path for Selenium tests. diff --git a/go/test/endtoend/cluster/cluster_process.go b/go/test/endtoend/cluster/cluster_process.go index 9b871282e30..9cfdeee4814 100644 --- a/go/test/endtoend/cluster/cluster_process.go +++ b/go/test/endtoend/cluster/cluster_process.go @@ -38,6 +38,7 @@ import ( "vitess.io/vitess/go/json2" "vitess.io/vitess/go/mysql" "vitess.io/vitess/go/sqltypes" + "vitess.io/vitess/go/test/endtoend/filelock" "vitess.io/vitess/go/vt/grpcclient" "vitess.io/vitess/go/vt/log" "vitess.io/vitess/go/vt/vtgate/planbuilder/plancontext" @@ -1027,24 +1028,49 @@ func (cluster *LocalProcessCluster) GetAndReservePort() int { return cluster.nextPortForProcess } +// portFileTimeout determines when we see the content of a port file as +// stale. After this time, we assume we can start with the default base +// port again. +const portFileTimeout = 1 * time.Hour + // getPort checks if we have recent used port info in /tmp/todaytime.port // If no, then use a random port and save that port + 200 in the above file // If yes, then return that port, and save port + 200 in the same file // here, assumptions is 200 ports might be consumed for all tests in a package func getPort() int { - tmpPortFileName := path.Join(os.TempDir(), time.Now().Format("01022006.port")) + portFile, err := os.OpenFile(path.Join(os.TempDir(), "endtoend.port"), os.O_CREATE|os.O_RDWR, 0644) + if err != nil { + panic(err) + } + + filelock.Lock(portFile) + defer filelock.Unlock(portFile) + + fileInfo, err := portFile.Stat() + if err != nil { + panic(err) + } + + portBytes, err := io.ReadAll(portFile) + if err != nil { + panic(err) + } + var port int - if _, err := os.Stat(tmpPortFileName); os.IsNotExist(err) { + if len(portBytes) == 0 || time.Now().After(fileInfo.ModTime().Add(portFileTimeout)) { port = getVtStartPort() } else { - result, _ := os.ReadFile(tmpPortFileName) - cport, err := strconv.Atoi(string(result)) - if err != nil || cport > 60000 || cport == 0 { - cport = getVtStartPort() + parsedPort, err := strconv.ParseInt(string(portBytes), 10, 64) + if err != nil { + panic(err) } - port = cport + port = int(parsedPort) } - os.WriteFile(tmpPortFileName, []byte(fmt.Sprintf("%d", port+200)), 0666) + + portFile.Truncate(0) + portFile.Seek(0, 0) + portFile.WriteString(fmt.Sprintf("%v", port+200)) + portFile.Close() return port } @@ -1068,7 +1094,7 @@ func getRandomNumber(maxNumber int32, baseNumber int) int { func getVtStartPort() int { osVtPort := os.Getenv("VTPORTSTART") if osVtPort != "" { - cport, err := strconv.Atoi(string(osVtPort)) + cport, err := strconv.Atoi(osVtPort) if err == nil { return cport } diff --git a/go/test/endtoend/filelock/filelock.go b/go/test/endtoend/filelock/filelock.go new file mode 100644 index 00000000000..05f27c321a8 --- /dev/null +++ b/go/test/endtoend/filelock/filelock.go @@ -0,0 +1,99 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package filelock provides a platform-independent API for advisory file +// locking. Calls to functions in this package on platforms that do not support +// advisory locks will return errors for which IsNotSupported returns true. +package filelock + +import ( + "errors" + "io/fs" + "os" +) + +// A File provides the minimal set of methods required to lock an open file. +// File implementations must be usable as map keys. +// The usual implementation is *os.File. +type File interface { + // Name returns the name of the file. + Name() string + + // Fd returns a valid file descriptor. + // (If the File is an *os.File, it must not be closed.) + Fd() uintptr + + // Stat returns the FileInfo structure describing file. + Stat() (fs.FileInfo, error) +} + +// Lock places an advisory write lock on the file, blocking until it can be +// locked. +// +// If Lock returns nil, no other process will be able to place a read or write +// lock on the file until this process exits, closes f, or calls Unlock on it. +// +// If f's descriptor is already read- or write-locked, the behavior of Lock is +// unspecified. +// +// Closing the file may or may not release the lock promptly. Callers should +// ensure that Unlock is always called when Lock succeeds. +func Lock(f File) error { + return lock(f, writeLock) +} + +// RLock places an advisory read lock on the file, blocking until it can be locked. +// +// If RLock returns nil, no other process will be able to place a write lock on +// the file until this process exits, closes f, or calls Unlock on it. +// +// If f is already read- or write-locked, the behavior of RLock is unspecified. +// +// Closing the file may or may not release the lock promptly. Callers should +// ensure that Unlock is always called if RLock succeeds. +func RLock(f File) error { + return lock(f, readLock) +} + +// Unlock removes an advisory lock placed on f by this process. +// +// The caller must not attempt to unlock a file that is not locked. +func Unlock(f File) error { + return unlock(f) +} + +// String returns the name of the function corresponding to lt +// (Lock, RLock, or Unlock). +func (lt lockType) String() string { + switch lt { + case readLock: + return "RLock" + case writeLock: + return "Lock" + default: + return "Unlock" + } +} + +// IsNotSupported returns a boolean indicating whether the error is known to +// report that a function is not supported (possibly for a specific input). +// It is satisfied by ErrNotSupported as well as some syscall errors. +func IsNotSupported(err error) bool { + return isNotSupported(underlyingError(err)) +} + +var ErrNotSupported = errors.New("operation not supported") + +// underlyingError returns the underlying error for known os error types. +func underlyingError(err error) error { + switch err := err.(type) { + case *fs.PathError: + return err.Err + case *os.LinkError: + return err.Err + case *os.SyscallError: + return err.Err + } + return err +} diff --git a/go/test/endtoend/filelock/filelock_unix.go b/go/test/endtoend/filelock/filelock_unix.go new file mode 100644 index 00000000000..23064dae0be --- /dev/null +++ b/go/test/endtoend/filelock/filelock_unix.go @@ -0,0 +1,42 @@ +// Copyright 2018 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package filelock + +import ( + "io/fs" + "syscall" +) + +type lockType int16 + +const ( + readLock lockType = syscall.LOCK_SH + writeLock lockType = syscall.LOCK_EX +) + +func lock(f File, lt lockType) (err error) { + for { + err = syscall.Flock(int(f.Fd()), int(lt)) + if err != syscall.EINTR { + break + } + } + if err != nil { + return &fs.PathError{ + Op: lt.String(), + Path: f.Name(), + Err: err, + } + } + return nil +} + +func unlock(f File) error { + return lock(f, syscall.LOCK_UN) +} + +func isNotSupported(err error) bool { + return err == syscall.ENOSYS || err == syscall.ENOTSUP || err == syscall.EOPNOTSUPP || err == ErrNotSupported +}