raft: transport package by LK4D4 · Pull Request #1748 · moby/swarmkit

LK4D4 · 2016-11-14T23:02:53Z

I'm trying to come up with a smaller testable package for raft transport to simplify membership.Cluster and have better coverage. It's just a start, and it doesn't pass the linters or whatever for now. Will appreciate any feedback.
ping @aaronlehmann

aaronlehmann · 2016-11-14T23:33:35Z

manager/state/raft/transport/peer.go

+
+func (p *peer) stop() {
+	close(p.stopped)
+	<-p.done


Seems like this could block for a long time if p.msgc has a lot of messages enqueued. Maybe processLoop should check if p.stopped is closed before selecting on p.msgc.

Or maybe it would make sense to reuse WithContext here, and closing p.stopped would cause the context to be cancelled.

aaronlehmann · 2016-11-14T23:43:42Z

manager/state/raft/transport/peer.go

+	case <-p.stopped:
+		return errors.New("peer stopped")
+	case <-ctx.Done():
+		return ctx.Err()


<-p.stopped and <-ctx.Done should be checked in a different select from p.msgc <- m. Otherwise the p.msgc <-m branch can be randomly chosen.

aaronlehmann · 2016-11-14T23:46:29Z

manager/state/raft/transport/transport.go

+		for _, e := range errs {
+			errStr += "\n" + e.Error()
+		}
+		return errors.Errorf("errors occured during Send: %s", errStr)


I assume these eventually end up in the log. I think it's a lot cleaner to just log each error separately. Logging something containing newlines is pretty bad.

aaronlehmann · 2016-11-14T23:52:18Z

manager/state/raft/transport/peer.go

+			p.active = false
+			p.mu.Unlock()
+		}
+	}()


Not sure if this defer is necessary at all. If the message queue is full, then most likely the node is down and sendProcessMessage will call ReportUnreachable. If the context is cancelled or p.stopped is closed, I don't think calling ReportUnreachable is appropriate.

aaronlehmann · 2016-11-14T23:57:15Z

manager/state/raft/transport/transport.go

+	if err != nil {
+		return err
+	}
+	p, err := newPeer(m.To, addr, t)


It's problematic to use a peer that isn't tracked in the t.peers list. For example, that peer might call ReportUnreachable after raft has shut down.

LK4D4 · 2016-11-15T20:59:26Z

@aaronlehmann PTAL. I'm still not sure how to handle message to unknown peer properly. Now I just create peer for it, which might not be desirable on Send. Other solution would be to have separate goroutine which sends messages to unknown peers. Let me know what do you think.

aaronlehmann · 2016-11-15T21:06:46Z

Now I just create peer for it, which might not be desirable on Send

What is the downside?

codecov-io · 2016-11-15T21:11:48Z

Current coverage is 55.00% (diff: 65.15%)

Merging #1748 into master will increase coverage by 0.18%

@@             master      #1748   diff @@
==========================================
  Files           103        105     +2   
  Lines         17250      17518   +268   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           9456       9636   +180   
- Misses         6649       6724    +75   
- Partials       1145       1158    +13

Powered by Codecov. Last update fef7386...79a5679

LK4D4 · 2016-11-15T21:25:28Z

@aaronlehmann it calls Dial (we probably need to call health check there as well) which might block sending messages for some time.

aaronlehmann · 2016-11-15T21:36:19Z

Yeah, that's not good. If a dedicated goroutine for sending messages to unknown peers would solve the problem, it's worth considering.

LK4D4 · 2016-11-16T00:53:30Z

@aaronlehmann PTAL
I've moved unknown senders to separate goroutine and embedded context more deeply. Also added healthcheck on peer add.

aaronlehmann · 2016-11-16T02:32:18Z

manager/state/raft/transport/transport_test.go

+	require.NoError(t, c.Add(2))
+
+	// set channel to nil to emulate full queue
+	c.Get(1).tr.peers[2].msgc = nil


Data race here

aaronlehmann · 2016-11-16T02:37:52Z

manager/state/raft/transport/transport.go

+
+// unknownSender sends messages to unknown peers. It creates new peer for each
+// message and discards it after send.
+func (t *Transport) unknownSender(ctx context.Context) {


I don't think this goroutine is giving us anything over the original goroutine-per-unknown-send approach. The problem I had with the original approach is that there was no way to make sure the goroutine was done before shutting down raft, but this seems to have the same problem.

Maybe going back to the original approach of spawning a goroutine every time we need to talk to an unknown sender, plus a wait group that run waits on, would do the trick?

I don't understand why we need to be sure that the raft is up in this case. ReportUnreachable and ReportSnapshot is no-op in those cases. Cancelling request should be enough.

I can move this to run goroutine, so Done will wait until unknown stuff is processed as well.

LK4D4 · 2016-11-16T15:30:03Z

@aaronlehmann PTAL, now you can wait on Done until everything is finished.

aaronlehmann · 2016-11-16T18:46:15Z

That looks much better, thanks.

aaronlehmann · 2016-12-01T00:08:32Z

manager/state/raft/transport/peer.go

+			p.tr.config.Raft.ReportSnapshot(m.To, raft.SnapshotFailure)
+		}
+		p.tr.config.Raft.ReportUnreachable(m.To)
+		if grpc.ErrorDesc(err) == membership.ErrMemberRemoved.Error() {


There is a slight change to this in #1779 that should be replicated here.

aaronlehmann · 2016-12-01T00:09:14Z

manager/state/raft/transport/transport.go

+		case <-ctx.Done():
+			return ctx.Err()
+		case <-t.ctx.Done():
+			return ctx.Err()


return t.ctx.Err() ?

aaronlehmann · 2016-12-01T00:09:48Z

vendor/google.golang.org/grpc/transport/transport.go

 		return streamErrorf(codes.Canceled, "%v", err)
 	}
+	fmt.Printf("%T %v\n", err, err)
+	fmt.Printf("%T %v\n", context.Canceled, context.Canceled)


Committed unintentionally?

aaronlehmann · 2016-12-01T00:10:24Z

I think we should move forward with trying to port the raft code to use the new transport package. If we wait, it will become harder as the code diverges.

LK4D4 · 2016-12-01T01:50:07Z

@aaronlehmann thanks! will do.

LK4D4 · 2016-12-22T18:30:43Z

@aaronlehmann I've replaced the code with transport package. However, I blindly removed stuff with lastSeenHost and not sure that made proper replacement.
@cyli @dperny I'd love some review here. This PR is about separating transport of raft messages from Node structure.

LK4D4 · 2017-01-07T02:04:32Z

Ok, it passes docker integration. Will fix comments at monday.

LK4D4 · 2017-01-09T17:04:46Z

@aaronlehmann @cyli I've split logic from restoreFromSnapshot.
@cyli I wasn't able to simplify stuff with map too much. Full Clear() would require some sort of synchronization with transport and also it would make short "quorum loss" for some functions that checks that. The solution would be to acquire members lock for whole snapshot operation, but I don't like how it looks.
Thanks!

cyli

LGTM

aaronlehmann · 2017-01-09T18:39:48Z

manager/state/raft/raft.go

+					m, ok := oldMembers[removedMember]
+					if !ok {
+						continue
+					}


This should call RemoveMember even if the member is not in oldMembers, because we need to keep track of the fact that this member ID was removed from the cluster.

aaronlehmann · 2017-01-09T18:41:11Z

manager/state/raft/storage.go

 }

-func (n *Node) restoreFromSnapshot(data []byte, forceNewCluster bool) error {
+func (n *Node) restoreFromSnapshot(data []byte, forceNewCluster bool) (api.ClusterSnapshot, error) {


forceNewCluster is not used anymore.

LK4D4 · 2017-01-09T18:58:32Z

@aaronlehmann Fixed, thanks! Also, I have 52 sequential passes of integration test on my machine so far.

LK4D4 · 2017-01-10T22:23:22Z

@aaronlehmann PTAL. I've added address change handling.

aaronlehmann · 2017-01-11T00:57:13Z

manager/state/raft/raft.go

+type hostsStore struct {
+	mu    sync.Mutex
+	hosts map[uint64]string
+}


What is this for?

aaronlehmann · 2017-01-11T00:58:10Z

manager/state/raft/raft.go


-// updateMember submits a configuration change to change a member's address.
-func (n *Node) updateMember(ctx context.Context, addr string, raftID uint64, nodeID string) error {
+// updateNodeBlocking runs synchronous job to update node addres in whole cluster.


typo: address

aaronlehmann · 2017-01-11T01:03:04Z

manager/state/raft/raft.go

-				if err := n.restoreFromSnapshot(rd.Snapshot.Data, false); err != nil {
+				snapCluster, err := n.clusterSnapshot(rd.Snapshot.Data)
+				if err != nil {
 					log.G(ctx).WithError(err).Error("failed to restore from snapshot")


Should this clear snapCluster? It seems bad to use it below if an error was returned.

aaronlehmann · 2017-01-11T01:24:23Z

manager/state/raft/transport/transport.go

+	if err := p.updateAddr(addr); err != nil {
+		return err
+	}
+	log.G(t.ctx).Debugf("peer %x updated to address %s, it will be used if old failed", id, addr)


In my testing, with a three node cluster that has one node down, I'm seeing this log line every second in each of the two remaining managers' logs (referring to the other remaining manager in each case). This doesn't seem right because neither address has changed.

aaronlehmann · 2017-01-11T01:31:49Z

I tested the address change detection and it seems to work, except for the spammy log message.

LK4D4 · 2017-01-11T20:12:20Z

@aaronlehmann I've fixed message and other your comments. Thanks for review and testing!

aaronlehmann

LGTM

This package is separate grpc transport layer for raft package. Before we used membership package + one very big method in raft package. Signed-off-by: Alexander Morozov <lk4d4math@gmail.com>

LK4D4 · 2017-01-12T17:21:18Z

Ok, I'm trying last time with docker and then merging.

LK4D4 · 2017-01-12T18:08:33Z

Only TestSwarmNetworkPlugin fails which is expected after #1856

LK4D4 added the status/1-design-review label Nov 14, 2016

aaronlehmann reviewed Nov 14, 2016

View reviewed changes

LK4D4 force-pushed the raft_transport branch from ffcf2fd to 1e956bc Compare November 15, 2016 20:56

LK4D4 force-pushed the raft_transport branch from 1e956bc to 9923164 Compare November 16, 2016 00:52

aaronlehmann reviewed Nov 16, 2016

View reviewed changes

LK4D4 force-pushed the raft_transport branch 2 times, most recently from 8d6fc76 to 542aad9 Compare November 16, 2016 15:29

aaronlehmann reviewed Dec 1, 2016

View reviewed changes

LK4D4 force-pushed the raft_transport branch from 542aad9 to 262958c Compare December 22, 2016 18:28

LK4D4 force-pushed the raft_transport branch 3 times, most recently from 8bc8c79 to 5b73465 Compare December 27, 2016 23:49

LK4D4 force-pushed the raft_transport branch from b49cfbc to b69b544 Compare January 9, 2017 16:26

LK4D4 added status/2-code-review and removed status/1-design-review labels Jan 9, 2017

LK4D4 force-pushed the raft_transport branch 2 times, most recently from 5d49d47 to c047a61 Compare January 9, 2017 17:24

cyli approved these changes Jan 9, 2017

View reviewed changes

aaronlehmann reviewed Jan 9, 2017

View reviewed changes

LK4D4 force-pushed the raft_transport branch from c047a61 to 7c2808d Compare January 9, 2017 18:45

LK4D4 force-pushed the raft_transport branch 3 times, most recently from acee090 to b576841 Compare January 10, 2017 22:05

aaronlehmann reviewed Jan 11, 2017

View reviewed changes

LK4D4 force-pushed the raft_transport branch from b576841 to 94b227b Compare January 11, 2017 20:08

aaronlehmann approved these changes Jan 11, 2017

View reviewed changes

raft: introducing transport package

79a5679

This package is separate grpc transport layer for raft package. Before we used membership package + one very big method in raft package. Signed-off-by: Alexander Morozov <lk4d4math@gmail.com>

LK4D4 force-pushed the raft_transport branch from 94b227b to 79a5679 Compare January 12, 2017 17:08

LK4D4 merged commit 50f82a9 into moby:master Jan 12, 2017

LK4D4 deleted the raft_transport branch January 12, 2017 18:42

Conversation

LK4D4 commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Nov 15, 2016

Uh oh!

aaronlehmann commented Nov 15, 2016

Uh oh!

codecov-io commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 55.00% (diff: 65.15%)

Uh oh!

LK4D4 commented Nov 15, 2016

Uh oh!

aaronlehmann commented Nov 15, 2016

Uh oh!

LK4D4 commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Nov 16, 2016

Uh oh!

aaronlehmann commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Dec 1, 2016

Uh oh!

LK4D4 commented Dec 1, 2016

Uh oh!

LK4D4 commented Dec 22, 2016

Uh oh!

LK4D4 commented Jan 7, 2017

Uh oh!

LK4D4 commented Jan 9, 2017

Uh oh!

cyli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Jan 9, 2017

Uh oh!

LK4D4 commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jan 11, 2017

Uh oh!

LK4D4 commented Jan 11, 2017

Uh oh!

codecov-io commented Nov 15, 2016 •

edited

Loading