Config reload [WIP] #499

tylertreat · 2017-05-31T02:25:52Z

This is a rough first pass at config reload (issue #338). This is just a tracer that implements reload support for the trace config option to prove out a working end-to-end example. If we like the overall approach, we can start implementing support for additional config options in a piecemeal fashion. This PR is mostly just to get early feedback.

This PR does a few things:

Adds a Reload function to Server which can be called to reload configuration from a file.
Adds support for reloading trace option. Attempting to change any other options will result in a reload failure.
If reload fails, the server continues using the previous config.
Adds a SIGHUP handler which invokes config reload.
Some small refactors to reuse code and minimize changes.

Open questions:

If the new config file is invalid, should we dump the last good config to a .bak file (or do something else)?
How should we signal config reload on Windows?

@nats-io/core

tylertreat · 2017-05-31T02:33:43Z

Anyone know what's up with the unit tests? go test is returning with exit status 1, but it looks like all the tests passed.

kozlovic · 2017-05-31T02:38:12Z

All builds fail at "TestRouteTLSHandshakeError", you may want to look if your changes have introduced a call to os.Exit() or something that this test may trigger.

coveralls · 2017-05-31T18:00:31Z

Coverage decreased (-0.1%) to 91.003% when pulling 43ee0ce on config_reload into d606944 on master.

derekcollison · 2017-06-01T16:12:58Z

server/log.go

@@ -15,9 +15,9 @@ var trace int32
 var debug int32

 var log = struct {
-	sync.Mutex
+	*sync.Mutex


Why this change?

Unit test is storing the value of log before the test and replacing it after to prevent side effects, so govet complains because it's copying the mutex value (motivation for #501).

derekcollison · 2017-06-01T16:21:25Z

server/server.go

@@ -148,6 +147,18 @@ func New(opts *Options) *Server {
 	return s
 }

+func (s *Server) getOpts() *Options {
+	s.optsMu.RLock()
+	defer s.optsMu.RUnlock()


This might be expensive for when getOpts is in a fast path. Defer is better, but still slow. Just explicitly unlock like setOpts does.

derekcollison · 2017-06-01T16:22:33Z

server/signal.go

 		return
 	}
 	c := make(chan os.Signal, 1)

-	signal.Notify(c, syscall.SIGINT, syscall.SIGUSR1)
+	signal.Notify(c, syscall.SIGINT, syscall.SIGUSR1, syscall.SIGHUP)


I would not add this in until we have programmatic tests via API for all options we care about etc. and good test coverage. We should not expose to end users too soon IMO.

I agree with @derekcollison. let's build up the features for supporting it and then add in the reload once we have the critical items (Certs and Auth).

Derek, you agree we can merge all the bits to support in prior to having the signal and making the feature available in the server, correct?

Defer adds a bit of overhead which can affect fast-paths.

coveralls · 2017-06-01T17:01:17Z

Coverage decreased (-0.1%) to 91.023% when pulling c8280aa on config_reload into d606944 on master.

petemiron

I'll approve once we have agreement on merge/release strategy and confirmation of symlink working xplatform.

petemiron · 2017-06-01T18:47:56Z

server/reload.go

+		}
+		switch strings.ToLower(field.Name) {
+		case "trace":
+			opts = append(opts, &traceOption{newValue.(bool)})


I think it would be clearer to name opts something like diff_opts. took me a second to get why you were appending.

petemiron · 2017-06-01T18:53:32Z

server/reload_test.go

+	}
+	processOptions(golden)
+
+	if err := os.Symlink("./configs/reload/test.conf", config); err != nil {


does this work as expected on windows? cc @ColinSullivan1

It should (https://golang.org/src/os/file_windows.go#L522), though I don't have a Windows box to test on.

It works on windows so long as you have correct privileges - you'll want to add a note someplace, probably the README.md.

Without the right privileges:

reload_test.go:175: Error creating symlink: symlink .\configs\reload\test.conf g:\src\github.com\nats-io\gnatsd\server\tmp.conf: A required privilege is not held by the client. FAIL

On a network drive (generally not allowed by default):

--- FAIL: TestConfigReload (0.00s) reload_test.go:175: Error creating symlink: symlink .\configs\reload\test.conf g:\src\github.com\nats-io\gnatsd\server\tmp.conf: Incorrect function. FAIL

Locally, with privileges (simple case running on C: as admin), you'll succeed.

=== RUN TestConfigReload [3792] [INF] Reloaded server configuration --- PASS: TestConfigReload (0.00s)

@ColinSullivan1 Thanks for checking that. I can add a note to the README if we think this is acceptable. Since this is just for development purposes, this seems OK to me. What do you guys think?

IMO this is OK; nothing unusual (at least for Windows).

petemiron · 2017-06-01T18:55:53Z

server/signal.go

 		return
 	}
 	c := make(chan os.Signal, 1)

-	signal.Notify(c, syscall.SIGINT, syscall.SIGUSR1)
+	signal.Notify(c, syscall.SIGINT, syscall.SIGUSR1, syscall.SIGHUP)


I agree with @derekcollison. let's build up the features for supporting it and then add in the reload once we have the critical items (Certs and Auth).

Derek, you agree we can merge all the bits to support in prior to having the signal and making the feature available in the server, correct?

Do not expose config reloading yet until we are ready.

coveralls · 2017-06-01T19:31:02Z

Coverage decreased (-0.2%) to 90.997% when pulling 677ff0a on config_reload into d606944 on master.

ColinSullivan1 · 2017-06-01T21:28:44Z

Regarding a config reload on windows. some other mechanism will be needed (that signal is not really supported). Either a REST API, or something else. We have the same issue with log rotation.

tylertreat · 2017-06-01T21:47:01Z

@ColinSullivan1 Yep, since we're not exposing the signals yet, I think we can defer the decision for what we want to do on Windows for now. That can come at a later time when we're ready to expose the feature.

Add a note clarifying symlink behavior on Windows for developers.

coveralls · 2017-06-01T22:15:39Z

Coverage decreased (-0.2%) to 90.906% when pulling cef5a70 on config_reload into d606944 on master.

ColinSullivan1 · 2017-06-02T15:52:44Z

README.md

+FAIL
+```
+
+Similarly, this can fail when creating a symlink on a network drive, which is typically now allowed by default:


typo, "not"

ColinSullivan1 · 2017-06-02T15:56:47Z

server/reload.go

@@ -0,0 +1,99 @@
+package server


Can you add a copyright?

// Copyright 2017 Apcera Inc. All rights reserved.

ColinSullivan1 · 2017-06-02T15:59:06Z

server/reload_test.go

@@ -0,0 +1,210 @@
+package server


ColinSullivan1 · 2017-06-02T16:02:23Z

server/reload_test.go

+	processOptions(golden)
+
+	if err := os.Symlink("./configs/reload/test.conf", config); err != nil {
+		t.Fatalf("Error creating symlink: %v", err)


I'm torn on this, but maybe skip the test here (and in others below), with some output? This makes assumptions about the privileges of the user the test is running under, where the test is running, etc. It's convenience vs ensuring a test runs locally. wdyt?

IMO it's better to fail loudly, otherwise the test might get skipped and the user wouldn't notice the warning message. We could add additional information to the Fatal message.

I can go with that... IMO an "ensure you have privileges" would go a long way.

ColinSullivan1 · 2017-06-02T16:09:25Z

server/server.go

@@ -584,7 +596,7 @@ func (s *Server) createClient(conn net.Conn) *client {
 	}
 	// If there is a max connections specified, check that adding
 	// this new client would not push us over the max
-	if s.opts.MaxConn > 0 && len(s.clients) >= s.opts.MaxConn {
+	if s.getOpts().MaxConn > 0 && len(s.clients) >= s.getOpts().MaxConn {


Do you think it is worth optimizing in this function to snapshot MaxConn, TLS options, and AuthTimeout? I realize establishing the connection and TLS are already expensive, but am thinking of a thundering herd of clients reconnecting.

derekcollison · 2017-06-02T18:01:35Z

@tylertreat Let's rebase to master. Keep signalling removed and make sure we cover Windows builds, etc. Then we are good to go.

kozlovic

My main concerns are about making a deep copy of Options, or at least be sure of the way options are accessed, and the use of multiple getOpts() within the same function. Regardless of the safety of accessing a copy or the "live" options, it may be logically incorrect to get the user name and password from two calls to getOpts() when the options are changed in between.

kozlovic · 2017-06-05T10:01:45Z

main.go

@@ -149,13 +148,16 @@ func main() {
 		usage()
 	}

+	// Snapshot flag options.
+	*server.FlagSnapshot = *opts


Do we need to copy the values since they may change after? If so, then we need a clone function for options. This assignment simply copy some of the fields, but for instance, the array of users will still point to the original, same for the tls.Config, or if we later add maps, etc...

kozlovic · 2017-06-05T10:07:14Z

server/auth.go

@@ -30,13 +30,13 @@ func (s *Server) configureAuthorization() {
 	}
 	// Check for multiple users first
 	// This just checks and sets up the user map if we have multiple users.
-	if s.opts.Users != nil {
+	if s.getOpts().Users != nil {


I see that we have replaced all references of s.opts with s.getOpts(), is it because s.opts pointer can change during the reload? Again, think of some of the types in the options that are currently not copied. You are protecting against the value of the pointer not being accessed concurrently, but fields pointed by s.opts or s.getOpts would still point to objects that are shared and may incorrectly be concurrently accessed. Of course, if we clone and make sure that values set/reset during reload are on a copy and then swap the pointer, we are ok.

I also have an issue with the same function invoking getOpts(), not only for performance - which still may make a difference - but because if conceptually we assume that the options may change between 2 calls to getOpts(), then we should snapshot once per function. Again, this assumes that the getOpts() would return an immutable structure, otherwise, there is no reason to do that.

kozlovic · 2017-06-05T10:10:17Z

server/reload.go

+	}
+	s.setOpts(newOpts)
+	s.applyOptions(changed)
+	return nil


There may be cases that applying an option may fail - in the long run - so I would make applyOptions return an error, and therefore move the s.setOpts() only after we know that we have a value new options set to swap with the old one.

The problem with failing an option application is we could end up in an inconsistent state—some new options are applied, others are not—unless we also introduce a "revert" operation to revert any applied options.

No because we apply the options to a temporary Options structure, and do not swap the server's options with the new one until after success.

There are two parts to swapping Options: 1) swapping the Options pointer itself and 2) mutating the server as necessary (e.g. for trace we need to update the server logger. 2) could result in an inconsistent state of the server if something fails.

I see, but if we apply the new options to the server's options that a caller has a pointer to (getOpts()), there is a risk that the caller accesses values that are being changed on the fly.

Agreed, there is, but I'm not sure how we can apply a new config atomically.

kozlovic · 2017-06-05T10:13:24Z

server/reload.go

+// error.
+func (s *Server) diffOptions(newOpts *Options) ([]option, error) {
+	var (
+		oldConfig = reflect.ValueOf(s.opts).Elem()


Either we make the entry API threadsafe (with special mutex) or we don't access s.opts directly here. I think it would be safer to make the whole Reload() API protected by its own mutex?

kozlovic · 2017-06-05T10:14:52Z

server/reload.go

+	return diffOpts, nil
+}
+
+func (s *Server) applyOptions(opts []option) {


As indicated above, I believe that there will be cases where applying an option may produce an error. We could add later of course, since those are internal APIs.

…_reload

coveralls · 2017-06-05T22:54:57Z

Coverage increased (+0.5%) to 91.3% when pulling 1e79911 on config_reload into cba3170 on master.

coveralls · 2017-06-05T22:56:05Z

Coverage increased (+0.6%) to 91.407% when pulling 1e79911 on config_reload into cba3170 on master.

coveralls · 2017-06-05T23:02:09Z

Coverage increased (+0.2%) to 90.925% when pulling 1e79911 on config_reload into cba3170 on master.

tylertreat · 2017-06-05T23:02:42Z

I believe I've addressed everyone's feedback. @kozlovic can you take another look?

kozlovic

Concerns addressed except for the question about when we switch the server's options when applying the changes, which I understand is a moot point since there is always the risk that a part of the code is accessing options while options are being updated.

kozlovic · 2017-06-06T07:58:30Z

server/log.go

-		s.SetLogger(fileLog, s.opts.Debug, s.opts.Trace)
+		fileLog := logger.NewFileLogger(opts.LogFile,
+			opts.Logtime, s.getOpts().Debug, opts.Trace, true)
+		s.SetLogger(fileLog, s.getOpts().Debug, opts.Trace)


Still using s.getOpts() here...

coveralls · 2017-06-06T13:57:49Z

Coverage increased (+0.4%) to 91.193% when pulling 4f28bf7 on config_reload into cba3170 on master.

Feedback addressed

tylertreat · 2017-06-06T14:04:36Z

@derekcollison I think this is good to go.

Tyler Treat added 2 commits May 30, 2017 16:18

First pass at implementing config reload

9902c3d

Add tests around config reload

b95dd5d

ColinSullivan1 added waffle:needs review labels May 31, 2017

Fix unit tests

43ee0ce

tylertreat force-pushed the config_reload branch from e4bb091 to 43ee0ce Compare May 31, 2017 17:52

tylertreat mentioned this pull request May 31, 2017

Get rid of global logger #500

Closed

2 tasks

derekcollison reviewed Jun 1, 2017

View reviewed changes

tylertreat assigned petemiron and ColinSullivan1 Jun 1, 2017

Get rid of defer in Server.getOpts()

c8280aa

Defer adds a bit of overhead which can affect fast-paths.

petemiron reviewed Jun 1, 2017

View reviewed changes

Tyler Treat added 2 commits June 1, 2017 14:11

Remove SIGHUP handling

e109c3f

Do not expose config reloading yet until we are ready.

Rename opts to diffOpts in diffOptions for clarity

677ff0a

Add Development section to README

cef5a70

Add a note clarifying symlink behavior on Windows for developers.

ColinSullivan1 previously requested changes Jun 2, 2017

View reviewed changes

kozlovic requested changes Jun 5, 2017

View reviewed changes

Tyler Treat added 2 commits June 5, 2017 13:41

Merge branch 'master' of github.com:nats-io/gnatsd into config_reload

c468abd

Delete bench results file

ea03083

Tyler Treat added 3 commits June 5, 2017 13:58

Merge branch 'config_reload' of github.com:nats-io/gnatsd into config…

b03bdd0

…_reload

Address code review feedback

cc30af8

Undelete bench_results.txt

1e79911

tylertreat force-pushed the config_reload branch from 0f8dea9 to 1e79911 Compare June 5, 2017 22:49

kozlovic approved these changes Jun 6, 2017

View reviewed changes

Remove use of getOpts() in log.go

4f28bf7

derekcollison merged commit 2b7ab35 into master Jun 6, 2017

derekcollison deleted the config_reload branch June 6, 2017 15:12

derekcollison removed the in progress label Jun 6, 2017

tylertreat mentioned this pull request Jun 27, 2017

Implement config reload #524

Merged

6 tasks

ColinSullivan1 mentioned this pull request Jul 11, 2017

Hot configuration reload #338

Closed

Config reload [WIP] #499

Config reload [WIP] #499

Conversation

tylertreat commented May 31, 2017 • edited Loading

tylertreat commented May 31, 2017 • edited Loading

kozlovic commented May 31, 2017

coveralls commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 1, 2017

petemiron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 1, 2017

ColinSullivan1 commented Jun 1, 2017

tylertreat commented Jun 1, 2017

coveralls commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison commented Jun 2, 2017

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylertreat Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 5, 2017

coveralls commented Jun 5, 2017

coveralls commented Jun 5, 2017

tylertreat commented Jun 5, 2017

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 6, 2017

tylertreat commented Jun 6, 2017

tylertreat commented May 31, 2017 •

edited

Loading

tylertreat commented May 31, 2017 •

edited

Loading

tylertreat Jun 5, 2017 •

edited

Loading