Cleanup Meta Ticket

[armon]

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

State Races

• 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)
• Verify the current term has not changed when preparing/processing the AppendEntries message Rework goroutines and synchronization #136

Follower Replication:

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

Change Inflight Tracking

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

Improve Membership Tracking

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

Crashes / Restart Issues

• Panic with old snapshots Panic getting logs after restoring from an older snapshot #85
• TrailingLogs set to 0 with restart bug Behaviour on setting TrailingLogs to 0 #86

New Tests

• Config change under loss of quorum: Fixes tests and plumbs more ServerAddress vs. string parameters. #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:

• 1 - Fixup NewRaft() to understand legacy LogAddPeer and LogRemovePeer entries Adds in-place upgrade and manual recovery support. #139
• 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?) Adds in-place upgrade and manual recovery support. #139
• 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 Adds in-place upgrade and manual recovery support. #139
• 4 - Are there any cases where restoring a snapshot and making a non-matching latest/committed config can lead to snapshotting failing indefinitely?
• 5 - Add staging -> voter promotion logic [v2] Add staging->voter promoter logic #161
• 6 - Clean up observer interface to use new types (we could probably just use the new server structure here) Fixes tests and plumbs more ServerAddress vs. string parameters. #127
• 7 - Implement GetConfiguration() Fixes races with Raft configurations member, adds GetConfiguration accessor. #134
• 8- Resolve ServerAddress return type for Leader() Fixes tests and plumbs more ServerAddress vs. string parameters. #127
• 9 - Resolve quorumSize() Rework goroutines and synchronization #136
• 10 - Resolve "barf on new address" Changing addresses for a server #162
• 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. Panic on storage failures #163
• 12 - Resolve "take the req in as a single argument" Fixes races with Raft configurations member, adds GetConfiguration accessor. #134
• 13 - Resolve broken unit tests
• 14 - Add tests for new peer transitions Split apart and test appendConfigurationEntry #132
• 15 - Add old-format snapshot test Add tests for old format snapshots #164
• 16 - Resolve "what is this" in replicate() Rework goroutines and synchronization #136
• 17 - Resolve TODO in updateLastAppended() Rework goroutines and synchronization #136
• 18 - Consider renaming Configuration to something less conflate-y with Config Rename "Configuration" to "Membership" #165
• 19 - Think through the possibility of having two replication threads going to the same peer - see these comments for details
• 20 - Make sure we have a test for installSnapshot that exercises the path that supports the old log messages for peer configuration Add tests for old format snapshots #164
• 21 - Kick the FSM for every log entry via a noop - see these comments for details Dispatch a noop to the FSM from non-FSM logs #166
• 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). Make the stats into a real structure #167
• 23 - Update or delete membership.md now that the work happened. [v2] Update membership.md text and move into code comments #168
• 24 - Add a note to type configurations that latest and committed may be the same, and often are. Rename "Configuration" to "Membership" #165
• 25 - (from Rework goroutines and synchronization #136 (comment)) 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. Clean up heartbeat detection / short circuit code #169
• 26 - in Rework goroutines and synchronization #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) [v2] makePeerInternal() shouldn't default the protocol version #170
• 27 - for @slackpad do a full re-review of protocol version handling post-merge
• 28 - forward-port Adds ability to access user snapshots and restore them. #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 #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 Rework goroutines and synchronization #136 (please annotate).
• In the post-117 checklist, (9 quorumSize), (16 replicate), (17 updateLastAppended) will be resolved by Rework goroutines and synchronization #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!