raft: set n.Node and n.isMember to correct value in stop/removeNode #1288

runshenzhu · 2016-08-01T19:43:40Z

while n.IsStopped() returns value by checking if n.Node is nil, we didn't set n.Node correctly in n.stop(). This PR fixes it by using isMember to tell if the node is stopped.

Also, it should fix #1272.

ping @abronan @LK4D4

Signed-off-by: Runshen Zhu runshen.zhu@gmail.com

codecov-io · 2016-08-01T19:52:31Z

Current coverage is 55.18% (diff: 67.74%)

Merging #1288 into master will decrease coverage by <.01%

@@             master      #1288   diff @@
==========================================
  Files            80         80          
  Lines         12562      12565     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6933       6934     +1   
- Misses         4674       4681     +7   
+ Partials        955        950     -5

Powered by Codecov. Last update e021d14...e7a941e

LK4D4 · 2016-08-01T19:58:36Z

@runshenzhu can't we just set isMember to 0? Also with mutexes atomic sorta loses its original value, so isMember can be just bool.

runshenzhu · 2016-08-01T20:18:47Z

@LK4D4 I'm not sure if an atomic value could prevent it from race, without using lock.

Suppose that isMember is set to 0 in stop, but n.IsLeader still could call n.Node.Status() after stopping, if lock is not hold.

func (n *Node) IsLeader() bool {
    // n.isMember is 1, but node is stopping
    if !n.IsMember() || n.Node == nil {
        return false
    }

    // node is stopped, and n.isMember is set to 0

    if n.Node.Status().Lead == n.Config.ID {
        return true
    }
    return false
}

LK4D4 · 2016-08-01T20:21:42Z

@runshenzhu I'm pretty sure that atomic can't prevent it, that's why I propose to replace it with simple bool and use it as guard.

runshenzhu · 2016-08-01T21:16:18Z

updated: set n.isMember to 0 in n.stop() and n.applyRemoveNode()

@LK4D4 I tried to follow your suggestion of removing atomic but then go complains about race of accessing n.isMember in n.applyRemoveNode().

And we can't simply use lock to protect it, because if the leaving node is leader, a read lock is already held in Leave(). In this case, we firstly should release read lock temporarily in n.applyRemoveNode() and then to get the write lock.

Here I keep n.isMember as atomic because it simplifies the code in n.applyRemoveNode(). But I could change it to bool if you think bool is better.

LK4D4 · 2016-08-01T21:29:08Z

manager/state/raft/raft.go

@@ -486,7 +492,7 @@ func (n *Node) stop() {

 // IsLeader checks if we are the leader or not
 func (n *Node) IsLeader() bool {
-	if !n.IsMember() {
+	if !n.IsMember() || n.Node == nil {


why we still need nil check?

runshenzhu · 2016-08-01T21:58:34Z

update: remove the nil check. PTAL @LK4D4 @abronan

LK4D4 · 2016-08-01T22:00:31Z

I still think that those checks need some more thoughts.
This PR LGTM, at least should prevent deadlocks in Status()

aaronlehmann · 2016-08-01T22:43:35Z

manager/state/raft/raft.go

+
+	if n.Node != nil {
+		n.Stop()
+		n.Node = nil


Is setting Node to nil still necessary?

@aaronlehmann I think n.IsStopped() needs it.

Sorry, still confused about this. Now that we set both n.Node = nil and isMember to 0, isn't IsStopped basically the same thing as IsMember? I don't understand why we need both. It looks like the only place we use only IsStopped is in LeaderAddr, but I don't understand why that couldn't use IsMember instead.

aaronlehmann · 2016-08-01T22:48:02Z

As @LK4D4 mentioned earlier, I don't understand why isMember is an atomic variable. It seems like it needs a lock around it anyway so there aren't time of check/time of use races. Should it just be a regular bool, and have everything that relies on it be protected by stopMu?

runshenzhu · 2016-08-01T22:53:32Z

@aaronlehmann It will be a little complicated to use n.stopMu in n.applyRemoveNode(). If the leaving node is leader, a read lock is already held in Leave(). In this case, we firstly should release read lock temporarily in n.applyRemoveNode() and then to get the write lock.

Using atomic will simplify the case.

runshenzhu · 2016-08-01T22:58:35Z

update: close n.removeRaftCh in n.applyRemoveNode()

aaronlehmann · 2016-08-01T23:03:53Z

manager/state/raft/raft.go

+		n.removeRaftOnce.Do(func() {
+			atomic.StoreUint32(&n.isMember, 0)
+			close(n.removeRaftCh)
+		})


We should probably factor the n.removeRaftOnce.Do call into a function, since the same code appears in two places.

aaronlehmann · 2016-08-01T23:08:51Z

IsLeader can be called from outside code (see protobuf/plugin/raftproxy/raftproxy.go). Since the lock won't be held in this situation, don't we need something to protect it against a situation where isMember is set to 0 right after being checked?

runshenzhu · 2016-08-02T00:36:37Z

@aaronlehmann updated to use RLock(), and refactor it to remove redundant code.

aaronlehmann · 2016-08-02T01:44:10Z

@runshenzhu: Any thoughts on this comment? #1288 (comment)

runshenzhu · 2016-08-02T02:11:30Z

@aaronlehmann As a quick fix, n.IsLeader() could hold n.stopMu.RLock to protect itself. This lock is re-enterable and could solve access race.

runshenzhu · 2016-08-02T06:59:32Z

update: using read lock to protect n.IsLeader.

This is a quick fix for #1288 comment. For a long term, we should figure out a better way to make sure n.IsLeader is running under a safe context.

aaronlehmann · 2016-08-02T18:12:09Z

Sounds good. You can just call it isLeader.

runshenzhu · 2016-08-02T18:53:08Z

update:

add 2 new methods: leader() and isLeader(), which don't hold any locks.
grab read lock in Leader() and IsLeader()

aaronlehmann · 2016-08-02T19:04:43Z

LGTM

aaronlehmann · 2016-08-04T18:23:31Z

ping @LK4D4 @abronan for more review.

Any thoughts on whether this should be included in 1.12.1?

LK4D4 · 2016-08-04T18:37:56Z

It sounds like it can be a serious issue. But I don't know if it should be included in minor release.

abronan · 2016-08-04T20:39:03Z

Let's include #1310 first and then test this one on top of it.

LK4D4 · 2016-08-04T23:49:48Z

@runshenzhu would you mind to rebase?

runshenzhu · 2016-08-05T02:42:07Z

rebased.

@LK4D4 @aaronlehmann @abronan PTAL

abronan · 2016-08-05T17:34:13Z

Do not merge yet, seeing a panic when stopping nodes, probably a race on setting the node to nil (1 on 3 crashed):

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x60 pc=0x12a520e]

goroutine 1313 [running]:
panic(0x1b17a20, 0xc82000a0d0)
    /usr/local/go/src/runtime/panic.go:481 +0x3e6
github.com/docker/swarmkit/manager/state/raft.(*Node).GetVersion(0xc820080480, 0x1)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/manager/state/raft/raft.go:939 +0x3e
github.com/docker/swarmkit/manager/state/store.(*MemoryStore).update(0xc821059260, 0x7f0e08303228, 0xc820080480, 0xc8213394e0, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/manager/state/store/memory.go:222 +0x9d
github.com/docker/swarmkit/manager/state/store.(*MemoryStore).Update(0xc821059260, 0xc8213394e0, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/manager/state/store/memory.go:270 +0x54
github.com/docker/swarmkit/manager/state/store.ViewAndWatch(0xc821059260, 0xc821339820, 0xc820ec3d90, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/manager/state/store/memory.go:703 +0xed
github.com/docker/swarmkit/manager/dispatcher.(*Dispatcher).Session(0xc821326000, 0xc820142088, 0x7f0e08240850, 0xc820d42a10, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/manager/dispatcher/dispatcher.go:801 +0x7b8
github.com/docker/swarmkit/api.(*authenticatedWrapperDispatcherServer).Session(0xc821308ac0, 0xc820142088, 0x7f0e08240850, 0xc820d42a10, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/api/dispatcher.pb.go:207 +0x16c
github.com/docker/swarmkit/api.(*raftProxyDispatcherServer).Session(0xc821315c40, 0xc820142088, 0x7f0e08240850, 0xc820d42a10, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/api/dispatcher.pb.go:1121 +0xc8
github.com/docker/swarmkit/api._Dispatcher_Session_Handler(0x1c32580, 0xc821315c40, 0x7f0e082407e0, 0xc821ce7880, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/github.com/docker/swarmkit/api/dispatcher.pb.go:667 +0x175
google.golang.org/grpc.(*Server).processStreamingRPC(0xc821328000, 0x7f0e0bd2e5e8, 0xc821cf2240, 0xc8220b36c0, 0xc821308d60, 0x2b34440, 0xc820d3bc80, 0x0, 0x0)
    /go/src/github.com/docker/docker/vendor/src/google.golang.org/grpc/server.go:602 +0x47a
google.golang.org/grpc.(*Server).handleStream(0xc821328000, 0x7f0e0bd2e5e8, 0xc821cf2240, 0xc8220b36c0, 0xc820d3bc80)
    /go/src/github.com/docker/docker/vendor/src/google.golang.org/grpc/server.go:686 +0x114e
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc820d42630, 0xc821328000, 0x7f0e0bd2e5e8, 0xc821cf2240, 0xc8220b36c0)
    /go/src/github.com/docker/docker/vendor/src/google.golang.org/grpc/server.go:348 +0xa0
created by google.golang.org/grpc.(*Server).serveStreams.func1
    /go/src/github.com/docker/docker/vendor/src/google.golang.org/grpc/server.go:349 +0x9a

aaronlehmann · 2016-08-05T17:36:24Z

The panic above is because n.Node is being set to nil, and GetVersion is not protected against this condition.

I'm not convinced that we should set n.Node to nil. I think it's safer and easier to use the atomic variable isMember instead (but I might be missing something).

runshenzhu · 2016-08-05T17:56:09Z

One concern of using isMember was that its value is set to 0 before the node is actually stopped.

Updated to use isMember as the indicator and postpone setting its value to the very end of stopping node.

But I'm wondering that should we check if node is stopped in GetVersion? and What's the correct return value of GetVersion() if the node is stopped, nil or &api.Version{}?

aaronlehmann · 2016-08-05T18:12:34Z

manager/state/raft/raft.go

@@ -963,10 +990,8 @@ func (n *Node) IsMember() bool {

 // IsStopped checks if the raft node is stopped or not
 func (n *Node) IsStopped() bool {


Can't we remove this function completely?

Signed-off-by: Runshen Zhu <runshen.zhu@gmail.com>

runshenzhu · 2016-08-05T18:26:35Z

updated to remove n.IsStopped() and protect GetVersion() with lock

runshenzhu · 2016-08-05T18:28:47Z

manager/state/raft/raft.go

+	defer n.stopMu.RUnlock()
+
+	if !n.IsMember() {
+		return nil


notice that if node is stopped, nil is returned. I think this check is necessary, as @abronan's test shown above, this method could be called after node is stopped. In this case, without the check, n.Node.Status() is executed after raft node is stopped.

Seems okay.

abronan · 2016-08-06T00:08:34Z

LGTM

aaronlehmann · 2016-08-06T00:35:44Z

LGTM

runshenzhu · 2016-08-06T07:25:04Z

LOL, there is a very long fix/conversation list for this PR. Thanks for everyone's patient help, comment and review.

GordonTheTurtle added the status/0-triage label Aug 1, 2016

runshenzhu force-pushed the fix-stop branch from 7d0b4f7 to b251d67 Compare August 1, 2016 20:59

LK4D4 reviewed Aug 1, 2016
View reviewed changes

runshenzhu force-pushed the fix-stop branch from b251d67 to 480bb18 Compare August 1, 2016 21:49

runshenzhu changed the title ~~raft: set n.Node to nil in stop()~~ raft: set n.Node and n.isMember to correct value in stop/removeNode Aug 1, 2016

aaronlehmann reviewed Aug 1, 2016
View reviewed changes

runshenzhu force-pushed the fix-stop branch from 480bb18 to b5e8906 Compare August 1, 2016 22:57

aaronlehmann reviewed Aug 1, 2016
View reviewed changes

runshenzhu force-pushed the fix-stop branch from b5e8906 to e28f30c Compare August 2, 2016 00:11

runshenzhu added area/raft and removed status/0-triage labels Aug 2, 2016

runshenzhu force-pushed the fix-stop branch from e28f30c to 4deff63 Compare August 2, 2016 06:56

runshenzhu force-pushed the fix-stop branch from 4deff63 to 79aca80 Compare August 2, 2016 07:02

runshenzhu force-pushed the fix-stop branch 3 times, most recently from ac00684 to dcebb13 Compare August 2, 2016 18:49

LK4D4 added the priority/P2 label Aug 4, 2016

runshenzhu force-pushed the fix-stop branch from dcebb13 to 7f39194 Compare August 5, 2016 01:59

runshenzhu force-pushed the fix-stop branch 4 times, most recently from 2b5aca5 to 0fb4cb7 Compare August 5, 2016 17:52

aaronlehmann reviewed Aug 5, 2016
View reviewed changes

set n.Node and n.isMember to correct value in stop/removeNode

e7a941e

Signed-off-by: Runshen Zhu <runshen.zhu@gmail.com>

runshenzhu force-pushed the fix-stop branch from 0fb4cb7 to e7a941e Compare August 5, 2016 18:24

runshenzhu reviewed Aug 5, 2016
View reviewed changes

aaronlehmann merged commit 3aa5322 into moby:master Aug 6, 2016

aaronlehmann mentioned this pull request Aug 6, 2016

Flaky raft test: TestUpdateNodeDemoteLeader #1337

Closed

		@@ -963,10 +990,8 @@ func (n *Node) IsMember() bool {

		// IsStopped checks if the raft node is stopped or not
		func (n *Node) IsStopped() bool {

raft: set n.Node and n.isMember to correct value in stop/removeNode #1288

raft: set n.Node and n.isMember to correct value in stop/removeNode #1288

Conversation

runshenzhu commented Aug 1, 2016 • edited Loading

codecov-io commented Aug 1, 2016 • edited Loading

Current coverage is 55.18% (diff: 67.74%)

LK4D4 commented Aug 1, 2016

runshenzhu commented Aug 1, 2016

LK4D4 commented Aug 1, 2016

runshenzhu commented Aug 1, 2016

LK4D4 Aug 1, 2016

Choose a reason for hiding this comment

runshenzhu commented Aug 1, 2016

LK4D4 commented Aug 1, 2016

aaronlehmann Aug 1, 2016

Choose a reason for hiding this comment

runshenzhu Aug 1, 2016

Choose a reason for hiding this comment

aaronlehmann Aug 5, 2016

Choose a reason for hiding this comment

aaronlehmann commented Aug 1, 2016

runshenzhu commented Aug 1, 2016

runshenzhu commented Aug 1, 2016

aaronlehmann Aug 1, 2016

Choose a reason for hiding this comment

aaronlehmann commented Aug 1, 2016

runshenzhu commented Aug 2, 2016

aaronlehmann commented Aug 2, 2016

runshenzhu commented Aug 2, 2016 • edited Loading

runshenzhu commented Aug 2, 2016

aaronlehmann commented Aug 2, 2016

runshenzhu commented Aug 2, 2016

aaronlehmann commented Aug 2, 2016

aaronlehmann commented Aug 4, 2016

LK4D4 commented Aug 4, 2016

abronan commented Aug 4, 2016

LK4D4 commented Aug 4, 2016

runshenzhu commented Aug 5, 2016

abronan commented Aug 5, 2016

aaronlehmann commented Aug 5, 2016

runshenzhu commented Aug 5, 2016 • edited Loading

aaronlehmann Aug 5, 2016

Choose a reason for hiding this comment

runshenzhu commented Aug 5, 2016 • edited Loading

runshenzhu Aug 5, 2016 • edited Loading

Choose a reason for hiding this comment

aaronlehmann Aug 5, 2016

Choose a reason for hiding this comment

abronan commented Aug 6, 2016

aaronlehmann commented Aug 6, 2016

runshenzhu commented Aug 6, 2016

runshenzhu commented Aug 1, 2016 •

edited

Loading

codecov-io commented Aug 1, 2016 •

edited

Loading

runshenzhu commented Aug 2, 2016 •

edited

Loading

runshenzhu commented Aug 5, 2016 •

edited

Loading

runshenzhu commented Aug 5, 2016 •

edited

Loading

runshenzhu Aug 5, 2016 •

edited

Loading