Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent panic errors in tombstone GC #2582

Closed
wants to merge 2 commits into from
Closed

Prevent panic errors in tombstone GC #2582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28e47cf
Prevent panic errors in tombstone GC
Dec 8, 2016
0562871
Create a global port reservation mapping
Dec 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions consul/state/tombstone_gc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,14 @@ func (t *TombstoneGC) expireTime(expires time.Time) {
// Get the maximum index and clear the entry
t.lock.Lock()
exp := t.expires[expires]
enabled := t.enabled
delete(t.expires, expires)
t.lock.Unlock()

if !enabled {
return
}

// Notify the expires channel
t.expireCh <- exp.maxIndex
}
55 changes: 50 additions & 5 deletions testutil/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"os"
"os/exec"
"strings"
"sync"

"github.com/hashicorp/consul/consul/structs"
"github.com/hashicorp/go-cleanhttp"
Expand Down Expand Up @@ -100,14 +101,57 @@ func defaultServerConfig() *TestServerConfig {
}
}

// Ports allocation state.
//
// We let the kernel choose random port number, so there's a good
// chance the port is unused. But kernel could allocate the same
// port twice, if agent did not have time to start listening on it.
//
// There is no obvious way to defend from the other processes in
// the system, which will use OS-assigned ports, but at least it
// will reduce duplicated ports shared by the agents.
var nmap = make(map[int]struct{})
var nmapmu sync.Mutex

// randomPort asks the kernel for a random port to use.
func randomPort() int {
l, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
panic(err)
makeport := func() (int, bool) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sethvargo, could you, please, say if the changes acceptable to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ybubnov

This is the testutil server and should never be used for production uses. Pushing the ports into a map does not solve the issue though (and I've talked with DC about this). Global varies are not shared across multiple packages: https://gist.github.com/sethvargo/ebc696c9ed8f541a9887307115918181.

If more than one package imports this package, they both start with empty, independent maps. It's really a terrible problem that does not have a good solution 😦

Copy link
Author

@ybubnov ybubnov Dec 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I understand this play with ports allocation is necessary to speed up the tests invocation only, definitely it is not targeted for production usage.

But I did not precisely get the point with global variables that are not shared across multiple packages (I assume you are talking about the testing packages).

If so, then go test command will sequentially (one package by one) compile a standalone binary for each testing package and launch the tests in parallel only within the single package:

go test --help
...
        -parallel n
            Allow parallel execution of test functions that call t.Parallel.
            The value of this flag is the maximum number of tests to run
            simultaneously; by default, it is set to the value of GOMAXPROCS.
            Note that -parallel only applies within a single test binary.
            The 'go test' command may run tests for different packages
            in parallel as well, according to the setting of the -p flag
            (see 'go help build').
...

So this map will be shared within a single binary. I also agree that proposed changes does not solve the problem (because there is clearly no solution). These changes to my mind reduce the probability of the case, when we allocate two same ports while testing the single package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, as long as we accept the tradeoffs, I'm okay with this.

l, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
panic(err)
}

defer l.Close()
port := l.Addr().(*net.TCPAddr).Port

nmapmu.Lock()
defer nmapmu.Unlock()

_, dup := nmap[port]
nmap[port] = struct{}{}
return port, dup
}

for i := 0; i < 100; i++ {
if port, dup := makeport(); !dup {
return port
}
}
defer l.Close()
return l.Addr().(*net.TCPAddr).Port

panic(fmt.Errorf("no usable ports"))
}

// releasePorts removes the ports reserved by test server.
func releasePorts(c *TestPortConfig) {
nmapmu.Lock()
defer nmapmu.Unlock()

delete(nmap, c.DNS)
delete(nmap, c.HTTP)
delete(nmap, c.RPC)
delete(nmap, c.SerfLan)
delete(nmap, c.SerfWan)
delete(nmap, c.Server)
}

// TestService is used to serialize a service definition.
Expand Down Expand Up @@ -257,6 +301,7 @@ func NewTestServerConfig(t TestingT, cb ServerConfigCallback) *TestServer {
// directory once we are done.
func (s *TestServer) Stop() {
defer os.RemoveAll(s.Config.DataDir)
defer releasePorts(s.Config.Ports)

if err := s.cmd.Process.Kill(); err != nil {
s.t.Errorf("err: %s", err)
Expand Down