API update

[MathieuBordere]

Proposal to update the raft API to allow for future extensions without breaking binaries linked against older versions.

Probably easiest to review by commit, it's draft so input is more then welcome.

[codecov-commenter]

Codecov Report

Merging #303 (af13184) into master (fde5378) will increase coverage by 0.16%. The diff coverage is 86.22%.

:exclamation: Current head af13184 differs from pull request most recent head 4c8f360. Consider uploading reports for the commit 4c8f360 to get more accurate results

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
+ Coverage   83.93%   84.10%   +0.16%     
==========================================
  Files          50       50              
  Lines        9400     9411      +11     
  Branches     2508     2500       -8     
==========================================
+ Hits         7890     7915      +25     
+ Misses        953      940      -13     
+ Partials      557      556       -1     

Impacted Files	Coverage Δ
src/recv.c	76.78% <0.00%> (-1.98%)	:arrow_down:
src/recv_request_vote_result.c	58.18% <0.00%> (-3.64%)	:arrow_down:
src/state.c	54.16% <0.00%> (ø)
src/uv.c	84.30% <25.00%> (-0.55%)	:arrow_down:
src/uv_tcp.c	91.02% <40.00%> (-3.50%)	:arrow_down:
src/start.c	75.72% <66.66%> (ø)
src/raft.c	85.03% <73.33%> (-1.74%)	:arrow_down:
src/client.c	73.62% <80.00%> (ø)
src/membership.c	71.31% <80.00%> (ø)
src/replication.c	77.68% <92.30%> (ø)
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[MathieuBordere]

Maybe I'm missing something -- if we're adding a version field to raft_uv_transport to prepare for evolving it in the future, why don't we also add a reserved field as for some of the other structs?

Maybe the difference is somewhat arbitrary, but I consider those structs with reserved space 'internal' structs, meaning that raft uses them for whatever it wants, but they can still be allocated by the user or embedded in another struct (was one of the design decisions), and that's why we need enough room to add functionality.

The versioned structs are 'external', meaning the client fills out their functionality, they too can be allocated by the client or embedded in another struct. Based on the version, as long as the raft library doesn't read/write members that do not correspond to the version, memory accesses should be safe. In theory we could add padding to those structs, but I don't think it is needed imo.

So for internal structs we don't want the version approach because it will hamper us to add transparent functionality purely because the user did not allocate enough space for it.

[cole-miller]

@MathieuBordere Thanks, makes sense to me!

[MathieuBordere]

I'm working on the dqlite disk-mode in parallel, I'm currently figuring out a decent proposal for the snapshot mechanism, and analyze if anything needs to be added to this PR for that.

[MathieuBordere]

I think this is ready for review, let's maybe not merge it on a Friday as it would bump the .so number and break builds. Will remove draft state on Monday for this reason.

[MathieuBordere]

~~I'm probably going to add the fix and the changes to struct raft for #250 to this PR otherwise we'll have to immediately start using the reserved fields and can't put the last committed configuration close to the struct raft members it logically belongs near to.~~

edit: I can probably just add a field struct raft_configuration committed_configuration to struct raft and not use it immediately. I think we all agree we need to keep a copy of the last committed configuration around, and it's logical to store it in struct raft. Let me know what you think of that @cole-miller.

edit2: Or maybe we should call it previous_configuration. Because on startup, a raft node doesn't know which log entries are committed. In that way it's imaginable that a node loads 2 (or more) configurations from the log entries and either (a) both are committed or (b) the first one is committed and the last one is not committed. In this case we need a way to save the second to last configuration to possibly revert to it. In case (a) it would not be correct to set committed_configuration imo as that one should always contain the last committed configuration. (I hope I'm being clear)

[MathieuBordere]

I just remembered that for canonical/dqlite#420 we want to support support including a request ID in various places like struct raft_apply, I don't think that's addressed by the current branch?

Edit: and see @freeekanayaka's comment in that issue about client sessions, which might raise other ABI/API concerns

Thanks, I'll take a look!

[cole-miller]

@MathieuBordere here's how I imagine the config stuff working, hopefully we're on the same page. Writing it out in detail to clarify my own thinking & so that you can catch if I'm misunderstanding something :)

We add a new field to struct raft that I'll call known_good_config; this represents a configuration that we will roll back to in the event that the CHANGE log entry that controls the current raft->configuration is truncated out of the log. Here is how we update the new field in various situations:

• at raft_start, we initially don't know any config information, so both configuration and known_good_config are empty
• at raft_bootstrap or raft_recover, known_good_config is initialized to the passed-in config, just like configuration is
• when loading a snapshot causes us to increase our last known log index (i.e. we're replaying the log from disk at startup, or a leader is getting us up to speed), we set known_good_config to the config stored with the snapshot -- this is because an uncommitted config should never be snapshotted (possibly we do snapshot uncommitted configs currently; that should be fixed)
• when installing a snapshot that doesn't cover new entries, I don't think an update is required, because even if known_good_config refers to a log entry that gets snapshotted we've already cached it when that log entry was applied (and an entry that hasn't been applied shouldn't be snapshotted)
• when applying a CHANGE entry to the log, known_good_config is updated to that config

I think that should ensure that we always have a config stored in memory that can be safely rolled back to.

[MathieuBordere]

. . .

• at raft_start, we initially don't know any config information, so both configuration and known_good_config are empty

. . .

At raft_start we do know the configuration, if there's a snapshot, we know the configuration in the snapshot was committed. If there are no other config entries in succeeding log entries, we fill configuration and known_good_configuration out with the configuration from the snapshot. If there is a snapshot and 1 config entry in the log, the config entry in the log can be committed or uncommitted, we can still fill out known_good_config with the one from the snapshot and configuration with the one from the log and set configuration_uncommitted_index, waiting for the entry to be committed or rolled back. If there are 2 or more entries in the log succeeding the snapshot, we know that the second to last entry must have been a committed config (as a raft node won't accept a new config change unless the previous one is committed or truncated/rolled back from the log) and we use it as known_good_configuration and the last one as configuration and set configuration_uncommitted_index.

[MathieuBordere]

I've added client_id, req_id and reserved fields to RAFT__REQUEST now. I also added RAFT__REQUEST to the other requests, raft_change and raft_transfer, those 2 only use the data* field but I thought it was nicer to make the requests uniform. Let me know if you don't like the change.

Do we need anything else right now to support client sessions without breaking ABI/API in the future?

[MathieuBordere]

Should I bump to 0.17.0? https://github.com/canonical/raft/blob/fde5378a44ad5d7ed2225f5beaabdb20af230746/configure.ac#L2 I'm inclined to do so (without making a release though).

[cole-miller]

@MathieuBordere Got it, I think we're on the same page about the config stuff now. (We don't have to call the new field known_good_config, I just made that up.)

[MathieuBordere]

So I've added 3 id's to RAFT__REQUEST now and changed the type to a 16 byte array to accommodate UUID's.

[MathieuBordere]

I propose to merge this next Monday, unless @freeekanayaka has any objections.