Cleanup Meta Ticket

[armon]

Here are the list of issues, grouped together if they might make sense as a single PR.

State Races

• [x] Raft state should use locks for any fields that are accessed together (e.g. index and term)

Multi-Row Fetching

• [ ] Replace single row lookups with multi row lookups (LogStore / LogCache) (look at cases around log truncation)
• [x] Verify the current term has not changed when preparing/processing the AppendEntries message #136

Follower Replication:

• [x] replicateTo should verify leadership is current during looping
• [x] Check for any hot loops that do not break on stopCh

Change Inflight Tracking

• [x] Remove majorityQuorum
• [x] Inflight tracker should map Node -> Last Commit Index (match index)
• [x] Votes should be ignored from peers that are not part of peer set
• [x] precommit may not be necessary with new inflight (likely will be cleaned up via #117)

Improve Membership Tracking

• [x] Peer changes should have separate channel and do not pipeline (we don't want more than one peer change in flight at a time) #117
• [x] Peers.json should track index and any AddPeer or RemovePeer are ignored from older indexes - #117

Crashes / Restart Issues

• [ ] Panic with old snapshots #85
• [ ] TrailingLogs set to 0 with restart bug #86

New Tests

• [x] Config change under loss of quorum: #127
• Setup cluster with {A, B, C, D}
• Assume leader is A
• Partition {C, D}
• Remove {B}
• Test should fail to remove B (quorum cannot be reached)

/cc: @superfell @ongardie @sean- @ryanuber @slackpad

[ongardie]

Thanks for writing that up, @armon. I'll take a shot at "Change inflight tracking".

[ongardie]

I'll start on 'precommit may not be necessary with new inflight' (though I only have about 20min right now, ugh).

[slackpad]

Ok I'm going to go ahead and merge #117. Here's the list of remaining tasks I identified from that. Most of these are already marked inline with TODO comments:

• [x] 1 - Fixup NewRaft() to understand legacy LogAddPeer and LogRemovePeer entries #139
• [x] 2 - Work through live upgrade scenario now that we have a new config format and ignore old config messages (will we need to support the legacy LogAddPeer and LogRemovePeer to make the transition work?) #139
• [x] 3 - We need a mechanism for manual peer updates after an outage - there's currently no equivalent of "editing peers.json" so it doesn't seem possible to recover from all outage scenarios; there's one broken test related to this as well: TestRaft_SnapshotRestore_PeerChange #139
• [x] 4 - Are there any cases where restoring a snapshot and making a non-matching latest/committed config can lead to snapshotting failing indefinitely?
• [x] 5 - Add staging -> voter promotion logic #161
• [x] 6 - Clean up observer interface to use new types (we could probably just use the new server structure here) #127
• [x] 7 - Implement GetConfiguration() #134
• [x] 8- Resolve ServerAddress return type for Leader() #127
• [x] 9 - Resolve quorumSize() #136
• [x] 10 - Resolve "barf on new address" #162
• [x] 11 - Resolve "leaving r.getLastLog() in the wrong state" and panic any time the stores fail and we're not sure what ought to happen. #163
• [x] 12 - Resolve "take the req in as a single argument" #134
• [x] 13 - Resolve broken unit tests
• [x] 14 - Add tests for new peer transitions #132
• [x] 15 - Add old-format snapshot test #164
• [x] 16 - Resolve "what is this" in replicate() #136
• [x] 17 - Resolve TODO in updateLastAppended() #136
• [x] 18 - Consider renaming Configuration to something less conflate-y with Config #165
• [x] 19 - Think through the possibility of having two replication threads going to the same peer - see these comments for details
• [x] 20 - Make sure we have a test for installSnapshot that exercises the path that supports the old log messages for peer configuration #164
• [x] 21 - Kick the FSM for every log entry via a noop - see these comments for details #166
• [x] 22 - Clean up how configuration comes out in Stats(), the "%+v" formatting is weird / ugly. Also add a test to make sure it shows up (fixed a bug where the logic was wrong). #167
• [x] 23 - Update or delete membership.md now that the work happened. #168
• [x] 24 - Add a note to type configurations that latest and committed may be the same, and often are. #165
• [x] 25 - (from https://github.com/hashicorp/raft/pull/136#issuecomment-241119421) net_transport.go tries to detect a heartbeat based on it being all zeros (no longer true with this PR), then directly invokes a function that's probably not safe to run concurrently. I think we should disable it for now and revisit later. #169
• [x] 26 - in #136 the final merge made makePeerInternal() default the protocol version to max if 0 is passed in - we should always configure a protocol version at this level (and 0 is a valid version) #170
• [x] 27 - for @slackpad do a full re-review of protocol version handling post-merge
• [x] 28 - forward-port #159 to issue-84-integration branch

[slackpad]

@ongardie got one trivial PR up #127 and will be sweeping through the unit tests tomorrow fixing the integration tests as well as the commented out ones.

[ongardie]

Cool, thanks @slackpad. I've got a busy day tomorrow but can help out later in the week.

[slackpad]

Cool I will keep adding the unit test work to the same PR in that case.

On Jun 27, 2016, at 11:50 PM, Diego Ongaro notifications@github.com wrote:

Cool, thanks @slackpad. I've got a busy day tomorrow but can help out later in the week.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[slackpad]

Ok I think #127 is in good shape for merge. Tomorrow I'm going to spend some time on these two items:

Work through live upgrade scenario now that we have a new config format and ignore old config messages (will we need to support the legacy LogAddPeer and LogRemovePeer to make the transition work?) We need a mechanism for manual peer updates after an outage - there's currently no equivalent of "editing peers.json" so it doesn't seem possible to recover from all outage scenarios; there's one broken test related to this as well: TestRaft_SnapshotRestore_PeerChange

Will experiment a little and write up a spec so we can figure out the right way forward with these. We will need a way for Consul to take the existing peers.json and call some new Raft APIs in order to start up the cluster as well, since the peers.json had the only state related to the peers if a snapshot had been taken. I don't think I want to reintroduce the peer store into Raft, but Consul will need to parse this file outside of Raft and call some bootstrap-y thing to get going. I think this problem is closely related to the items above.

[ongardie-sfdc]

@slackpad, small request: could you number the post-117 checklist above? It'd help me track things.

[slackpad]

@ongardie numbered the todo list

[ongardie-sfdc]

BTW I'm trying to refactor a ton of stuff that'll help or fix the following:

• "5 - Add staging -> voter promotion logic"
• "16 - Resolve "what is this" in replicate()"
• "17 - Resolve in updateLastAppended()"
• "19 - Think through the possibility of having two replication threads going to the same peer - see these comments for details"

so save those for me for now :)

[slackpad]

@ongardie thanks for the heads up. I'm working on 1, 2, 3 which should not conflict with your stuff too much. Should have a PR up soon so you can see what I'm thinking wrt. to these. Have a good weekend!

[ongardie-sfdc]

@slackpad, now that I know you don't have too much outstanding, can we move runFSM out to fsm.go and runSnapshots, shouldSnapshot, takeSnapshot out to snapshot.go? See where I'm going with this? Every goroutine that has its own independent state goes in a different file. Later on, they get kicked off of the Raft struct so that they're not reaching into things they shouldn't.

[slackpad]

Yep - I was starting to think about that, too. I'll go ahead and make that change and commit it now so we have a good basis for further work.

[slackpad]

@ongardie the split is done under https://github.com/hashicorp/raft/pull/135 - went ahead and merged it.

[ongardie-sfdc]

Thanks @slackpad!

[ongardie-sfdc]

Some bookkeeping:

• In the top checklist, "Verify the current term has not changed when preparing/processing the AppendEntries message" will be resolved by #136 (please annotate).
• In the post-117 checklist, (9 quorumSize), (16 replicate), (17 updateLastAppended) will be resolved by #136 (please annotate).
• New: Update or delete membership.md now that the work happened.
• New: Add a note to type configurations that latest and committed may be the same, and often are.
• In the post-117 checklist, expand (11 getLastLog) to panic any time the stores fail and we're not sure what ought to happen.

I think that's all I'm sitting on.

[slackpad]

@ongardie-sfdc thanks for the update and the detailed reviews! I reconciled all your changes with the checklists (added new numbered items for the two new ones).

[slackpad]

After merging the version and recovery stuff into issue-84-integration, I cut https://github.com/hashicorp/raft/tree/library-v2-stage-one. I'll integrate and ship the next version of Consul against this branch and keep it open for small fixes I can pick from here if needed. This'll free up the issue-84-integration branch for the goroutine changes and a possible period of instability as that settles in.

After giving the community a heads up wrt. the interface changes we can take issue-84-integration into master, and possibly make library-v2-stage-two as an interim bake and integrate target for future Consul versions.

[superfell]

Am seeing some issues with AddPeer after last nights merge, Is there a test somewhere that adds a new peer to a cluster, I did a scan though but didn't spot one for that. @ongardie @slackpad

[slackpad]

Hi @superfell - there's a test here that hits it - https://github.com/hashicorp/raft/blob/issue-84-integration/raft_test.go#L2217-L2258. It's likely that you are calling AddPeer() but haven't set the ProtcolVersion back via configuration to a version that supports it - if you set it to 1 things should work as before.

[slackpad]

Also, if you don't need to support any of the old stuff, I'd keep the ProtocolVersion at 3 and swap out your AddPeer() call with a call to AddVoter().

[superfell]

Ahh, the setting of protocolVer in DefaultConfig() was messing with me.

[slackpad]

Everything is done or tracked elsewhere - closing!