On Wed, Jul 30, 2025 at 09:23:52AM -0400, Mike Pattrick wrote:
> On Wed, Jul 30, 2025 at 6:04 AM Felix Huettner via dev
> <ovs-dev@openvswitch.org> wrote:
> >
> > peer->next_index - raft->log_start might underflow if we did just
> > snapshot and therefor raft->log_start is greater than peer->next_index.
> > If we do not actually send data (e.g. on a heartbeat) that is fine for
> > now, however in all other cases we would actually need to send a
> > install_snapshot_request instead.
> >
> > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> > ---
> > ovsdb/raft.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> > index 9694c94e6..9f09b0ede 100644
> > --- a/ovsdb/raft.c
> > +++ b/ovsdb/raft.c
> > @@ -2858,6 +2858,12 @@ raft_send_append_request(struct raft *raft,
> > {
> > ovs_assert(raft->role == RAFT_LEADER);
> >
> > + /* If n > 0 then we need to have the data still available that the peer
> > + * needs next. If the data is gone due to compaction we should have
> > send
> > + * an install_snapshot_request instead. */
> > + ovs_assert(!n || raft->log_start <= peer->next_index);
> > + ovs_assert(!n || peer->next_index + n <= raft->log_end);
Hi Mike,
thanks for the review.
>
> ovs_assert can be a NOP in some cases. Shouldn't this be either an
> ovs_fatal or returning an error code?
I added them during debugging my changes later in the series as i messed
up the log_start calculation there.
I think these can not actually happen and are only something that is
helpful during development.
But as i am not certain i want to see if i can prove this below:
So there are the following call sites of raft_send_append_request where
n!=0:
* raft_command_initiate
* raft_handle_install_snapshot_reply
* raft_handle_append_reply
Taking a look at each individual thing:
raft_command_initiate:
We can not hit the first assertion about log_start, since we only send
the append_request if next_index == the index of the new entry.
We can also not hit the second assertion since log_end must always then
be larger then next_index, otherwise next_index == the index of the new
entry would not hold.
raft_handle_install_snapshot_reply:
Explicitly checks for the first assert before calling raft_send_append_request
and in that case sends a install_snapshot_request instead.
"n" is calculated in a way that the second assert will also not hit.
raft_handle_append_reply:
The first assert can not be hit as that is also checked explicitly and a
install_snapshot_request is used instead.
The second assert can also not be hit, since there is an explicit check
for that.
So form my perspective there is no realistic reason to actually hit this
case during runtime and it is more of a development guard.
Thanks a lot,
Felix
>
> -M
>
> > +
> > const union raft_rpc rq = {
> > .append_request = {
> > .common = {
> > @@ -2872,7 +2878,8 @@ raft_send_append_request(struct raft *raft,
> > - raft->log_start].term
> > : raft->snap.term),
> > .leader_commit = raft->commit_index,
> > - .entries = &raft->entries[peer->next_index - raft->log_start],
> > + .entries = n ? &raft->entries[
> > + peer->next_index - raft->log_start] : NULL,
> > .n_entries = n,
> > },
> > };
> > --
> > 2.43.0
> >
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
