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

Fix to prevent data races #422

Merged
merged 6 commits into from
Nov 25, 2021
Merged

Fix to prevent data races #422

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba50d5c
removed unnecessary fields from context to prevent data races
Mixaster995 Nov 22, 2021
0a060d7
fixed review comments
Mixaster995 Nov 22, 2021
179ccf8
added traceCtx to manager
Mixaster995 Nov 23, 2021
d1a9461
refactored after review comments
Mixaster995 Nov 23, 2021
223cdd5
review refactoring
Mixaster995 Nov 24, 2021
9260bad
added spanlogger finish
Mixaster995 Nov 25, 2021
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
32 changes: 9 additions & 23 deletions internal/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ import (
"github.com/networkservicemesh/sdk/pkg/networkservice/common/authorize"
"github.com/networkservicemesh/sdk/pkg/tools/grpcutils"
"github.com/networkservicemesh/sdk/pkg/tools/log"
"github.com/networkservicemesh/sdk/pkg/tools/log/logruslogger"
"github.com/networkservicemesh/sdk/pkg/tools/log/spanlogger"
"github.com/networkservicemesh/sdk/pkg/tools/opentracing"
"github.com/networkservicemesh/sdk/pkg/tools/spiffejwt"
"github.com/networkservicemesh/sdk/pkg/tools/token"
Expand All @@ -54,6 +52,7 @@ const (

type manager struct {
Copy link
Member

@denis-tingaikin denis-tingaikin Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I would be suggested to remove this package. And use all this code inline. This can be done in separate PR.
@edwarnicke Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

In the generic I agree... not sure whether its necessary before the release or not though. How much work would it be and how much risk would it entail?

Copy link
Member

Choose a reason for hiding this comment

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

I think you're correct. Let's consider this after release.

ctx context.Context
logger log.Logger
configuration *config.Config
cancelFunc context.CancelFunc
mgr nsmgr.Nsmgr
Expand Down Expand Up @@ -84,23 +83,19 @@ func (m *manager) initSecurity() (err error) {
}

// RunNsmgr - start nsmgr.
func RunNsmgr(ctx context.Context, configuration *config.Config) error {
func RunNsmgr(ctx context.Context, logger log.Logger, configuration *config.Config) error {
starttime := time.Now()

m := &manager{
configuration: configuration,
logger: logger,
}
log.EnableTracing(true)
traceCtx, finish := withTraceLogger(ctx, "start")
defer finish()

// Context to use for all things started in main
m.ctx, m.cancelFunc = context.WithCancel(ctx)
m.ctx = log.WithFields(m.ctx, map[string]interface{}{"cmd": "Nsmgr"})
Copy link
Member

Choose a reason for hiding this comment

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

I think this is needed for https://github.com/networkservicemesh/cmd-nsmgr/blob/main/internal/manager/manager.go#L164

Please consider using separate contexts for chain nsmgr and for logging in RunNsmgr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

m.ctx = log.WithLog(m.ctx, logruslogger.New(m.ctx))

if err := m.initSecurity(); err != nil {
log.FromContext(traceCtx).Errorf("failed to create new spiffe TLS Peer %v", err)
m.logger.Errorf("failed to create new spiffe TLS Peer %v", err)
return err
}

Expand Down Expand Up @@ -161,25 +156,16 @@ func RunNsmgr(ctx context.Context, configuration *config.Config) error {
// Create GRPC server
m.startServers(m.server)

log.FromContext(m.ctx).Infof("Startup completed in %v", time.Since(starttime))
m.logger.Infof("Startup completed in %v", time.Since(starttime))
starttime = time.Now()
<-m.ctx.Done()

log.FromContext(m.ctx).Infof("Exit requested. Uptime: %v", time.Since(starttime))
m.logger.Infof("Exit requested. Uptime: %v", time.Since(starttime))
// If we here we need to call Stop
m.Stop()
return nil
}

func withTraceLogger(ctx context.Context, operation string) (c context.Context, f func()) {
ctx, sLogger, span, sFinish := spanlogger.FromContext(ctx, operation)
ctx, lLogger, lFinish := logruslogger.FromSpan(ctx, span, operation)
return log.WithLog(ctx, sLogger, lLogger), func() {
sFinish()
lFinish()
}
}

func createListenFolders(configuration *config.Config) {
for i := 0; i < len(configuration.ListenOn); i++ {
u := &configuration.ListenOn[i]
Expand All @@ -196,7 +182,7 @@ func waitErrChan(ctx context.Context, errChan <-chan error, m *manager) {
case err := <-errChan:
// We need to cal cancel global context, since it could be multiple context of this kind
m.cancelFunc()
log.FromContext(ctx).Warnf("failed to serve: %v", err)
m.logger.Warnf("failed to serve: %v", err)
}
}

Expand All @@ -217,7 +203,7 @@ func (m *manager) getPublicURL() string {
}
addrs, err := net.InterfaceAddrs()
if err != nil {
logrus.Warn(err.Error())
m.logger.Warn(err.Error())
return u.String()
}
for _, a := range addrs {
Expand All @@ -239,7 +225,7 @@ func (m *manager) startServers(server *grpc.Server) {
go func() {
// Create a required number of servers
errChan := grpcutils.ListenAndServe(m.ctx, listenURL, server)
logrus.Infof("NSMGR Listening on: %v", listenURL.String())
m.logger.Infof("NSMGR Listening on: %v", listenURL.String())
// For public schemas we need to perform registation of nsmgr into registry.
wg.Done()

Expand Down
43 changes: 15 additions & 28 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,16 @@ import (
"os/signal"
"syscall"

nested "github.com/antonfisher/nested-logrus-formatter"
"github.com/kelseyhightower/envconfig"
"github.com/sirupsen/logrus"

"github.com/networkservicemesh/cmd-nsmgr/internal/config"
"github.com/networkservicemesh/cmd-nsmgr/internal/manager"
"github.com/networkservicemesh/sdk/pkg/tools/debug"
"github.com/networkservicemesh/sdk/pkg/tools/jaeger"
"github.com/networkservicemesh/sdk/pkg/tools/log"
"github.com/networkservicemesh/sdk/pkg/tools/log/logruslogger"
"github.com/networkservicemesh/sdk/pkg/tools/log/spanlogger"

"github.com/networkservicemesh/cmd-nsmgr/internal/config"
"github.com/networkservicemesh/cmd-nsmgr/internal/manager"
)

func main() {
Expand All @@ -51,57 +49,46 @@ func main() {
)
defer cancel()

logrus.SetFormatter(&nested.Formatter{})
ctx = log.WithFields(ctx, map[string]interface{}{"cmd": os.Args[:1]})
ctx = log.WithLog(ctx, logruslogger.New(ctx))
_, sLogger, span, sFinish := spanlogger.FromContext(ctx, "cmd-nsmgr")
defer sFinish()
_, lLogger, lFinish := logruslogger.FromSpan(ctx, span, "cmd-nsmgr")
defer lFinish()
logger := log.Combine(sLogger, lLogger)

// ********************************************************************************
// Debug self if necessary
// ********************************************************************************
if err := debug.Self(); err != nil {
log.FromContext(ctx).Infof("%s", err)
logger.Infof("%s", err)
}

// ********************************************************************************
// Configure open tracing
// ********************************************************************************
// Enable Jaeger
log.EnableTracing(true)
jaegerCloser := jaeger.InitJaeger(ctx, "nsmgr")
jaegerCloser := jaeger.InitJaeger(log.WithLog(ctx, logger), "nsmgr")
defer func() { _ = jaegerCloser.Close() }()
traceCtx, finish := withTraceLogger(ctx, "nsmgr")

// Get cfg from environment
cfg := &config.Config{}
if err := envconfig.Usage("nsm", cfg); err != nil {
logrus.Fatal(err)
logger.Fatal(err)
}
if err := envconfig.Process("nsm", cfg); err != nil {
logrus.Fatalf("error processing cfg from env: %+v", err)
logger.Fatalf("error processing cfg from env: %+v", err)
}

logrus.Infof("Using configuration: %v", cfg)
logger.Infof("Using configuration: %v", cfg)

level, err := logrus.ParseLevel(cfg.LogLevel)
if err != nil {
logrus.Fatalf("invalid log level %s", cfg.LogLevel)
logger.Fatalf("invalid log level %s", cfg.LogLevel)
}
logrus.SetLevel(level)

// Startup is finished
finish()
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this can be a major. Can we run here sFinish()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are passing combined logger further in manager. Isn't calling sFinish() will close span logger?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can re-recreate the span logger for the next operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done


err = manager.RunNsmgr(traceCtx, cfg)
err = manager.RunNsmgr(ctx, logger, cfg)
if err != nil {
logrus.Fatalf("error executing rootCmd: %v", err)
}
}

func withTraceLogger(ctx context.Context, operation string) (c context.Context, f func()) {
ctx, sLogger, span, sFinish := spanlogger.FromContext(ctx, operation)
ctx, lLogger, lFinish := logruslogger.FromSpan(ctx, span, operation)
return log.WithLog(ctx, sLogger, lLogger), func() {
sFinish()
lFinish()
logger.Fatalf("error executing rootCmd: %v", err)
}
}
4 changes: 3 additions & 1 deletion test/test_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"testing"
"time"

"github.com/networkservicemesh/sdk/pkg/tools/log/logruslogger"

"github.com/edwarnicke/grpcfd"

"github.com/networkservicemesh/sdk/pkg/tools/log"
Expand Down Expand Up @@ -118,7 +120,7 @@ func (s *testSetup) Start() {
s.init()

go func() {
e := manager.RunNsmgr(s.ctx, s.configuration)
e := manager.RunNsmgr(s.ctx, logruslogger.New(s.ctx), s.configuration)
require.Nil(s.t, e)
}()

Expand Down