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.

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

Fix port allocation race condition during tests. #215

Merged
merged 5 commits into from
Apr 17, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -571,8 +571,8 @@ build:
test:
$(GO) test ./... -race

test-10:
$(GO) test ./... -race -test.count 10 -cover
test-in-ci:
$(GO) test ./... -race -test.count 25 -cover

fmt:
$(GO) fmt ./...
Expand Down Expand Up @@ -737,4 +737,4 @@ sync-deps:
sleep-10:
sleep 10

.PHONY: deploy-redirect-site sync-deps sleep-10 proxy-dashboard proxy-prometheus proxy-grafana clean clean-toolchain clean-binaries clean-protos presubmit test test-10 vet
.PHONY: deploy-redirect-site sync-deps sleep-10 proxy-dashboard proxy-prometheus proxy-grafana clean clean-toolchain clean-binaries clean-protos presubmit test test-in-ci vet
2 changes: 1 addition & 1 deletion cloudbuild.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ steps:
waitFor: ['Build: Protocol Buffers']
- id: 'Build: Test'
name: open-match-build
args: ['make', 'test-10']
args: ['make', 'test-in-ci']
volumes:
- name: 'go-vol'
path: '/go'
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ require (
github.com/modern-go/reflect2 v1.0.1 // indirect
github.com/opencensus-integrations/redigo v2.0.1+incompatible
github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
github.com/pkg/errors v0.8.0
github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90 // indirect
github.com/prometheus/procfs v0.0.0-20190403104016-ea9eea638872 // indirect
github.com/rafaeljusto/redigomock v0.0.0-20190202135759-257e089e14a1
Expand Down
1 change: 1 addition & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/9
github.com/peterbourgon/diskv v2.0.1+incompatible h1:UBdAOUP5p4RWqPBg048CAvpKN+vxiaj6gdUUzhl4XmI=
github.com/peterbourgon/diskv v2.0.1+incompatible/go.mod h1:uqqh8zWWbv1HBMNONnaR/tNboyR3/BZd58JJSHlUSCU=
github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY=
github.com/pkg/errors v0.8.0 h1:WdK/asTD0HN+q6hsWO3/vpuAkAr+tw6aNJNDFFf0+qw=
github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
Expand Down
12 changes: 11 additions & 1 deletion internal/app/mmforc/mmforc.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (
"strings"
"time"

"github.com/GoogleCloudPlatform/open-match/internal/util/netlistener"

"github.com/GoogleCloudPlatform/open-match/config"
"github.com/GoogleCloudPlatform/open-match/internal/logging"
"github.com/GoogleCloudPlatform/open-match/internal/metrics"
Expand Down Expand Up @@ -106,7 +108,15 @@ func initializeApplication() (config.View, error) {
ocMmforcViews := DefaultMmforcViews // mmforc OpenCensus views.
ocMmforcViews = append(ocMmforcViews, redigometrics.ObservabilityMetricViews...) // redis OpenCensus views.
mmforcLog.WithFields(log.Fields{"viewscount": len(ocMmforcViews)}).Info("Loaded OpenCensus views")
metrics.ConfigureOpenCensusPrometheusExporter(cfg, ocMmforcViews)

lh, err := netlistener.NewFromPortNumber(cfg.GetInt("metrics.port"))
if err != nil {
mmforcLog.WithFields(log.Fields{
"error": err.Error(),
}).Error("Unable to create metrics TCP listener")
return nil, err
}
metrics.ConfigureOpenCensusPrometheusExporter(lh, cfg, ocMmforcViews)
return cfg, nil
}

Expand Down
17 changes: 12 additions & 5 deletions internal/metrics/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ import (
"context"
"fmt"
"net/http"
"strconv"
"time"

"github.com/GoogleCloudPlatform/open-match/config"
"github.com/GoogleCloudPlatform/open-match/internal/util/netlistener"
log "github.com/sirupsen/logrus"

"go.opencensus.io/exporter/prometheus"
Expand All @@ -46,10 +46,9 @@ var (
// by Promethus for metrics gathering. The calling code can select any views
// it wants to register, from any number of libraries, and pass them in as an
// array.
func ConfigureOpenCensusPrometheusExporter(cfg config.View, views []*view.View) {

func ConfigureOpenCensusPrometheusExporter(lh *netlistener.ListenerHolder, cfg config.View, views []*view.View) {
//var infoCtx, err = tag.New(context.Background(), tag.Insert(KeySeverity, "info"))
metricsPort := cfg.GetInt("metrics.port")
metricsPort := lh.Number()
metricsEP := cfg.GetString("metrics.endpoint")
metricsRP := cfg.GetInt("metrics.reportingPeriod")

Expand Down Expand Up @@ -85,7 +84,15 @@ func ConfigureOpenCensusPrometheusExporter(cfg config.View, views []*view.View)
"port": metricsPort,
"endpoint": metricsEP,
}).Info("Attempting to start http server for OpenCensus metrics on localhost")
err := http.ListenAndServe(":"+strconv.Itoa(metricsPort), mux)
listener, err := lh.Obtain()
if err != nil {
mhLog.WithFields(log.Fields{
"error": err,
"port": metricsPort,
"endpoint": metricsEP,
}).Fatal("Failed to run Prometheus endpoint")
}
err = http.Serve(listener, mux)
if err != nil {
mhLog.WithFields(log.Fields{
"error": err,
Expand Down
18 changes: 16 additions & 2 deletions internal/serving/construction.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"github.com/GoogleCloudPlatform/open-match/internal/logging"
"github.com/GoogleCloudPlatform/open-match/internal/metrics"
redishelpers "github.com/GoogleCloudPlatform/open-match/internal/statestorage/redis"
"github.com/GoogleCloudPlatform/open-match/internal/util/netlistener"
"github.com/opencensus-integrations/redigo/redis"
log "github.com/sirupsen/logrus"
"go.opencensus.io/plugin/ocgrpc"
Expand Down Expand Up @@ -67,7 +68,14 @@ func NewMulti(paramsList []*ServerParams) (*OpenMatchServer, error) {
ocServerViews = append(ocServerViews, config.CfgVarCountView) // config loader view.
ocServerViews = append(ocServerViews, redis.ObservabilityMetricViews...) // redis OpenCensus views.
logger.WithFields(log.Fields{"viewscount": len(ocServerViews)}).Info("Loaded OpenCensus views")
metrics.ConfigureOpenCensusPrometheusExporter(cfg, ocServerViews)
promLh, err := netlistener.NewFromPortNumber(cfg.GetInt("metrics.port"))
if err != nil {
logger.WithFields(log.Fields{
"error": err.Error(),
}).Error("Unable to create metrics TCP listener")
return nil, err
}
metrics.ConfigureOpenCensusPrometheusExporter(promLh, cfg, ocServerViews)

// Connect to redis
pool, err := redishelpers.ConnectionPool(cfg)
Expand All @@ -77,7 +85,13 @@ func NewMulti(paramsList []*ServerParams) (*OpenMatchServer, error) {
}

// Instantiate the gRPC server with the bindings we've made.
grpcServer := NewGrpcServer(cfg.GetInt(paramsList[0].PortConfigName), logger)
grpcLh, err := netlistener.NewFromPortNumber(cfg.GetInt(paramsList[0].PortConfigName))
if err != nil {
logger.Fatal(err)
return nil, err
}

grpcServer := NewGrpcServer(grpcLh, logger)

omServer := &OpenMatchServer{
GrpcServer: grpcServer,
Expand Down
16 changes: 8 additions & 8 deletions internal/serving/grpcserver.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package serving

import (
"fmt"
"net"

"github.com/GoogleCloudPlatform/open-match/internal/util/netlistener"
log "github.com/sirupsen/logrus"
"go.opencensus.io/plugin/ocgrpc"

Expand All @@ -12,7 +12,7 @@ import (

// GrpcWrapper is a decoration around the standard GRPC server that sets up a bunch of things common to Open Match servers.
type GrpcWrapper struct {
port int
lh *netlistener.ListenerHolder
handlerFuncs []func(*grpc.Server)
server *grpc.Server
ln net.Listener
Expand All @@ -21,9 +21,9 @@ type GrpcWrapper struct {
}

// NewGrpcServer creates a new GrpcWrapper.
func NewGrpcServer(port int, logger *log.Entry) *GrpcWrapper {
func NewGrpcServer(lh *netlistener.ListenerHolder, logger *log.Entry) *GrpcWrapper {
return &GrpcWrapper{
port: port,
lh: lh,
logger: logger,
handlerFuncs: []func(*grpc.Server){},
}
Expand All @@ -39,17 +39,17 @@ func (gw *GrpcWrapper) Start() error {
if gw.ln != nil {
return nil
}
ln, err := net.Listen("tcp", fmt.Sprintf(":%d", gw.port))
ln, err := gw.lh.Obtain()
if err != nil {
gw.logger.WithFields(log.Fields{
"error": err.Error(),
"port": gw.port,
"port": gw.lh.Number(),
}).Error("net.Listen() error")
return err
}
gw.ln = ln

gw.logger.WithFields(log.Fields{"port": gw.port}).Info("TCP net listener initialized")
gw.logger.WithFields(log.Fields{"port": gw.lh.Number()}).Info("TCP net listener initialized")

server := grpc.NewServer(grpc.StatsHandler(&ocgrpc.ServerHandler{}))
for _, handlerFunc := range gw.handlerFuncs {
Expand All @@ -59,7 +59,7 @@ func (gw *GrpcWrapper) Start() error {
gw.grpcAwaiter = make(chan error)

go func() {
gw.logger.Infof("Serving gRPC on :%d", gw.port)
gw.logger.Infof("Serving gRPC on :%d", gw.lh.Number())
err := gw.server.Serve(ln)
gw.grpcAwaiter <- err
if err != nil {
Expand Down
20 changes: 7 additions & 13 deletions internal/serving/testing/minimatch.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/GoogleCloudPlatform/open-match/internal/pb"
"github.com/GoogleCloudPlatform/open-match/internal/serving"
redishelpers "github.com/GoogleCloudPlatform/open-match/internal/statestorage/redis"
netlistenerTesting "github.com/GoogleCloudPlatform/open-match/internal/util/netlistener/testing"
"github.com/alicebob/miniredis"
"github.com/opencensus-integrations/redigo/redis"
log "github.com/sirupsen/logrus"
Expand Down Expand Up @@ -90,12 +91,9 @@ func createOpenMatchServer(paramsList []*serving.ServerParams) (*MiniMatchServer
cfg.Set("logging.format", "text")
cfg.Set("logging.source", true)

promPort, err := NextPort()
if err != nil {
return nil, fmt.Errorf("cannot create port for prometheus, %s", err)
}
promListener := netlistenerTesting.MustListen()

cfg.Set("metrics.port", promPort)
cfg.Set("metrics.port", promListener.Number())
cfg.Set("metrics.endpoint", "/metrics")
cfg.Set("metrics.reportingPeriod", "5s")

Expand All @@ -116,7 +114,7 @@ func createOpenMatchServer(paramsList []*serving.ServerParams) (*MiniMatchServer
ocServerViews = append(ocServerViews, config.CfgVarCountView) // config loader view.
ocServerViews = append(ocServerViews, redis.ObservabilityMetricViews...) // redis OpenCensus views.
logger.WithFields(log.Fields{"viewscount": len(ocServerViews)}).Info("Loaded OpenCensus views")
metrics.ConfigureOpenCensusPrometheusExporter(cfg, ocServerViews)
metrics.ConfigureOpenCensusPrometheusExporter(promListener, cfg, ocServerViews)

// Connect to redis
mredis, err := miniredis.Run()
Expand Down Expand Up @@ -152,18 +150,14 @@ func createOpenMatchServer(paramsList []*serving.ServerParams) (*MiniMatchServer
return nil, err
}

grpcPort, err := NextPort()
if err != nil {
closeOnFailure()
return nil, fmt.Errorf("cannot provision a port for gRPC, %s", err)
}
grpcLh := netlistenerTesting.MustListen()
for _, params := range paramsList {
cfg.Set(params.PortConfigName, grpcPort)
cfg.Set(params.PortConfigName, grpcLh.Number())
}

// Instantiate the gRPC server with the connections we've made
logger.Info("Attempting to start gRPC server")
grpcServer := serving.NewGrpcServer(grpcPort, logger)
grpcServer := serving.NewGrpcServer(grpcLh, logger)

mmServer := &MiniMatchServer{
OpenMatchServer: &serving.OpenMatchServer{
Expand Down
46 changes: 0 additions & 46 deletions internal/serving/testing/nextport.go

This file was deleted.

27 changes: 0 additions & 27 deletions internal/serving/testing/nextport_test.go

This file was deleted.