raft: Add 'constructors' for struct raft, raft_fsm & raft_io

[MathieuBordere]

This allows the raft library to allocate enough space for the structs without depending on the space allocated by the user that could be compiled against an older version of libraft.

I eventually want to restructure the header files and hide internal data structures that should not have been exposed.

[codecov-commenter]

Codecov Report

Merging #287 (bf4c8ff) into master (05a49e0) will decrease coverage by 3.45%. The diff coverage is 89.23%.

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
- Coverage   83.67%   80.21%   -3.46%     
==========================================
  Files          49       49              
  Lines        9213     8457     -756     
  Branches     2464     2362     -102     
==========================================
- Hits         7709     6784     -925     
- Misses        954      983      +29     
- Partials      550      690     +140     

Impacted Files	Coverage Δ
src/raft.c	83.10% <60.00%> (-3.66%)	:arrow_down:
src/fixture.c	91.41% <98.00%> (-1.22%)	:arrow_down:
src/recv_timeout_now.c	46.42% <0.00%> (-20.24%)	:arrow_down:
src/recv_request_vote_result.c	51.92% <0.00%> (-9.90%)	:arrow_down:
src/uv_fs.c	62.58% <0.00%> (-9.56%)	:arrow_down:
src/recv_request_vote.c	73.46% <0.00%> (-9.55%)	:arrow_down:
src/uv_list.c	71.15% <0.00%> (-9.55%)	:arrow_down:
src/recv_append_entries.c	70.00% <0.00%> (-9.37%)	:arrow_down:
src/recv_install_snapshot.c	34.61% <0.00%> (-9.03%)	:arrow_down:
src/recv_append_entries_result.c	71.42% <0.00%> (-8.58%)	:arrow_down:
... and 39 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 05a49e0...bf4c8ff. Read the comment docs.

[stgraber]

Would it make sense to split this up into at least two separate commits. One which handles the type changes within the internal struct and then another which adds the new external symbols and functions?

[freeekanayaka]

I personally think there is value in being able to allocate data structures on the stack and/or being able to embed them in larger data structures. That's a pretty common idiom in C, used in the standard library, libuv and many others. After a lib reaches maturity (e.g. 1.0), ABI breakages typically become infrequent and one could bump the soname in those cases.

One middle ground which allows to accommodate both approaches would be to introduce functions like size_t raft_size(void), size_t raft_fsm_size(void) etc, similar to what libuv does, see for example uv_loop_size and other similar functions.

This would support both uses and styles.

Optionally one could also have the raft_create()/raft_fsm_create()/raft_io_create() functions in this branch as conveniences around raft_size()/raft_fsm_size()/raft_io_size().

YMMV.

[MathieuBordere]

I personally think there is value in being able to allocate data structures on the stack and/or being able to embed them in larger data structures. That's a pretty common idiom in C, used in the standard library, libuv and many others. After a lib reaches maturity (e.g. 1.0), ABI breakages typically become infrequent and one could bump the soname in those cases.

One middle ground which allows to accommodate both approaches would be to introduce functions like size_t raft_size(void), size_t raft_fsm_size(void) etc, similar to what libuv does, see for example uv_loop_size and other similar functions.

This would support both uses and styles.

Optionally one could also have the raft_create()/raft_fsm_create()/raft_io_create() functions in this branch as conveniences around raft_size()/raft_fsm_size()/raft_io_size().

YMMV.

Allocating on the stack and as part of larger data structures is indeed useful. But every time I add a field to e.g. struct raft, the dqlite ppa package should be rebuilt or we might corrupt it. Package maintainers for other distros might not do this and it could lead to crashes. The 2 most important users use snaps, so it's not a big issue for them, but it still doesn't feel nice.

In the past we just added fields to struct raft, I could continue this way until we reach 1.0, it's not really a problem for me.

@stgraber What's your take on this?

[freeekanayaka]

IIUC the raft_io and raft_fsm should be already be safely extendable, since the have the version field. Internally the raft code checks the version and avoids accessing fields that were added in versions higher than the one specified (please correct me if I'm missing something).

The raft structure does not have a version field, but it also changes slowly (2 times in the last year) so perhaps a soname bump would do it. It's frequent for packages of C libraries to update as consequence of a soname bump, so it would be a familiar thing for packagers to do.

All that being said, it's also perfectly fine to add the helpers above (perhaps just raft_size() and optionally raft_create(), assuming that raft_io/raft_fsm are covered by version) and use them in dqlite.

Given that you guys only support the snap and always rebuild dqlite, I'd say that bumping the soname and leaving this to packagers would be acceptable. A couple of updates in a year (and hopefully less as we mature) doesn't seem that much. YMMV

[MathieuBordere]

IIUC the raft_io and raft_fsm should be already be safely extendable, since the have the version field. Internally the raft code checks the version and avoids accessing fields that were added in versions higher than the one specified (please correct me if I'm missing something).

raft_fsm is fine, that one is filled in dqlite here , raft_io is not safe (at least for dqlite) as it is filled in raft itself here (I notice now that I forgot to bump the version to 2 after adding the async_work member).

[freeekanayaka]

IIUC the raft_io and raft_fsm should be already be safely extendable, since the have the version field. Internally the raft code checks the version and avoids accessing fields that were added in versions higher than the one specified (please correct me if I'm missing something).

raft_fsm is fine, that one is filled in dqlite here , raft_io is not safe (at least for dqlite) as it is filled in raft itself here (I notice now that I forgot to bump the version to 2 after adding the async_work member).

Ok, then I think a little change is due. I'd suggest that caller of raft_uv_init must set the version field of the io struct that is passing, and raft_uv_init must honor that, initializing only the fields supported by that version.

All functions that initialize a raft_io object with a specific implementation should also implement this protocol (I think raft_uv_init is the only public one we have at the moment, but perhaps we should document this convention in the struct raft_io docstring).

[MathieuBordere]

IIUC the raft_io and raft_fsm should be already be safely extendable, since the have the version field. Internally the raft code checks the version and avoids accessing fields that were added in versions higher than the one specified (please correct me if I'm missing something).

raft_fsm is fine, that one is filled in dqlite here , raft_io is not safe (at least for dqlite) as it is filled in raft itself here (I notice now that I forgot to bump the version to 2 after adding the async_work member).

Ok, then I think a little change is due. I'd suggest that caller of raft_uv_init must set the version field of the io struct that is passing, and raft_uv_init must honor that, initializing only the fields supported by that version.

All functions that initialize a raft_io object with a specific implementation should also implement this protocol (I think raft_uv_init is the only public one we have at the moment, but perhaps we should document this convention in the struct raft_io docstring).

Yes, I think I can live with the versioning approach for raft_io and raft_fsm, but raft is a bit different imo, it's used for a lot of internal bookkeeping, I believe its internals should be hidden from the user. e.g. I'm working on a feature to track a node's capabilities (handy for clusters with heterogeneous raft versions), there's really only 1 place where I can save that state and it's in struct raft, bumping the soname feels a bit overkill for changing internal fields like that.

[freeekanayaka]

IIUC the raft_io and raft_fsm should be already be safely extendable, since the have the version field. Internally the raft code checks the version and avoids accessing fields that were added in versions higher than the one specified (please correct me if I'm missing something).

raft_fsm is fine, that one is filled in dqlite here , raft_io is not safe (at least for dqlite) as it is filled in raft itself here (I notice now that I forgot to bump the version to 2 after adding the async_work member).

Ok, then I think a little change is due. I'd suggest that caller of raft_uv_init must set the version field of the io struct that is passing, and raft_uv_init must honor that, initializing only the fields supported by that version. All functions that initialize a raft_io object with a specific implementation should also implement this protocol (I think raft_uv_init is the only public one we have at the moment, but perhaps we should document this convention in the struct raft_io docstring).

Yes, I think I can live with the versioning approach for raft_io and raft_fsm, but raft is a bit different imo, it's used for a lot of internal bookkeeping, I believe its internals should be hidden from the user. e.g. I'm working on a feature to track a node's capabilities (handy for clusters with heterogeneous raft versions), there's really only 1 place where I can save that state and it's in struct raft, bumping the soname feels a bit overkill for changing internal fields like that.

What do you think of adding size_t raft_size(void) (same approach as libuv) ? That could be used by dqlite to allocate the memory for struct raft dynamically.

BTW, for tracking what features a specific libraft version is capable of, something static might be more appropriate than additional dynamic state in struc raft. It could be as easy as defining a RAFT_CAPABILITIES symbol (or whatever name/format), and possibly exposing that to remote nodes using a new RPC method (e.g. RAFT_IO_INFO or similar).

[freeekanayaka]

And/or something similar to http://docs.libuv.org/en/v1.x/version.html might do it too, e.g. unsigned int raft_version(void) to allow dqlite probe what the underlying libraft is capable of.

[stgraber]

Given that you guys only support the snap and always rebuild dqlite, I'd say that bumping the soname and leaving this to packagers would be acceptable. A couple of updates in a year (and hopefully less as we mature) doesn't seem that much. YMMV

So the "problem" is that we now have quite a few distros with native packages for libraft and libdqlite. That includes Debian, ArchLinux, OpenSUSE, AltLinux, Gentoo, ...

Pretty much anywhere where we have a native LXD package. Though some of those are still working on decoupling those two libs from their LXD package.

The way I usually see it is that new projects can get away with frequent breakages in their early days when they're still on a 0 major SOVER. Once you've released a stable SOVER, then it's reasonable for downstreams to assume some level of API maturity and that a major SOVER bump would effectively only come as the result of an API redesign which may happen every few years but that the rest is handled with minor bumps to denote backward compatible symbol additions.

And this isn't just academic, we have already received some complaints from downstream distros about our recent, unexpected, SOVER bump.

I'm also in the camp of people that consider anything with a high SOVER to be an indicator of potential poor quality, especially if the project is still actively bumping it every few months.

I'm not opposed to having to do one more bump so we can sort out our API, making sure that our functions are generally all extensible without symbol changes and that all our structs are carefully aligned and either properly versioned or always require their size be passed so we can tell what's set and what's not.

We've now had liblxc (SOVER 1) operate in that way for a decade without any need for breakages despite frequent additions to its API and structs. I'd like to see the same here.

@brauner what's your view on this?

[freeekanayaka]

Given that you guys only support the snap and always rebuild dqlite, I'd say that bumping the soname and leaving this to packagers would be acceptable. A couple of updates in a year (and hopefully less as we mature) doesn't seem that much. YMMV

So the "problem" is that we now have quite a few distros with native packages for libraft and libdqlite. That includes Debian, ArchLinux, OpenSUSE, AltLinux, Gentoo, ...

Pretty much anywhere where we have a native LXD package. Though some of those are still working on decoupling those two libs from their LXD package.

The way I usually see it is that new projects can get away with frequent breakages in their early days when they're still on a 0 major SOVER. Once you've released a stable SOVER, then it's reasonable for downstreams to assume some level of API maturity and that a major SOVER bump would effectively only come as the result of an API redesign which may happen every few years but that the rest is handled with minor bumps to denote backward compatible symbol additions.

And this isn't just academic, we have already received some complaints from downstream distros about our recent, unexpected, SOVER bump.

I'm also in the camp of people that consider anything with a high SOVER to be an indicator of potential poor quality, especially if the project is still actively bumping it every few months.

These are certainly valid points. Adding size_t raft_size(void), together with requiring users to set the desired raft_io->version before calling raft_uv_init, as noted above, would avoid sover bumps for the forseeable future AFAIU.

[stgraber]

These are certainly valid points. Adding size_t raft_size(void), together with requiring users to set the desired raft_io->version before calling raft_uv_init, as noted above, would avoid sover bumps for the forseeable future AFAIU.

I have a suspicion that @brauner is going to prefer the func(args, len) type approach where every time the struct is passed, its size is passed as well. This avoids the client having to know about struct versions, it can just fill what it knows about and sends the struct length through. Then the library can know what option was introduced at what struct length.

[MathieuBordere]

I'm going to create a new issue for this, where we can discuss it in a more structured way.

[freeekanayaka]

These are certainly valid points. Adding size_t raft_size(void), together with requiring users to set the desired raft_io->version before calling raft_uv_init, as noted above, would avoid sover bumps for the forseeable future AFAIU.

I have a suspicion that @brauner is going to prefer the func(args, len) type approach where every time the struct is passed, its size is passed as well. This avoids the client having to know about struct versions, it can just fill what it knows about and sends the struct length through. Then the library can know what option was introduced at what struct length.

As long as that results in a single-function change (like raft_init(..., sizet_t size)), I think that's ok, I don't have a preference for either. Changing all functions to be like that might be more work than value, but YMMV.

[freeekanayaka]

I'd also consider adding struct raft->version field similarly to what is done for struct raft_io and struct raft_fsm, that has the advantage of not coupling the semantics with the object size, for instance you could replace a field or a group of fields (e.g. a sub-struct) with another, with the struct size remaining the same.