[ovs-dev] [PATCH v4 2/3] ovsdb-idl: Avoid inconsistent IDL state with OVSDB_MONITOR_V3.

[Han Zhou]

On Fri, Apr 24, 2020 at 1:49 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> On 4/24/20 10:23 PM, Han Zhou wrote:
> >
> >
> > On Thu, Apr 23, 2020 at 12:04 PM Ilya Maximets <i.maximets at ovn.org
<mailto:i.maximets at ovn.org>> wrote:
> >>
> >> On 4/23/20 8:22 PM, Ilya Maximets wrote:
> >> > On 4/22/20 8:38 PM, Dumitru Ceara wrote:
> >> >> Assuming an ovsdb client connected to a database using
OVSDB_MONITOR_V3
> >> >> (i.e., "monitor_cond_since" method) with the initial monitor
condition
> >> >> MC1.
> >> >>
> >> >> Assuming the following two transactions are executed on the
> >> >> ovsdb-server:
> >> >> TXN1: "insert record R1 in table T1"
> >> >> TXN2: "insert record R2 in table T2"
> >> >>
> >> >> If the client's monitor condition MC1 for table T2 matches R2 then
the
> >> >> client will receive the following update3 message:
> >> >> method="update3", "insert record R2 in table T2", last-txn-id=TXN2
> >> >>
> >> >> At this point, if the presence of the new record R2 in the IDL
triggers
> >> >> the client to update its monitor condition to MC2 and add a clause
for
> >> >> table T1 which matches R1, a monitor_cond_change message is sent to
the
> >> >> server:
> >> >> method="monitor_cond_change", "clauses from MC2"
> >> >>
> >> >> In normal operation the ovsdb-server will reply with a new update3
> >> >> message of the form:
> >> >> method="update3", "insert record R1 in table T1", last-txn-id=TXN2
> >> >>
> >> >> However, if the connection drops in the meantime, this last update
might
> >> >> get lost.
> >> >>
> >> >> It might happen that during the reconnect a new transaction happens
> >> >> that modifies the original record R1:
> >> >> TXN3: "modify record R1 in table T1"
> >> >>
> >> >> When the client reconnects, it will try to perform a fast resync by
> >> >> sending:
> >> >> method="monitor_cond_since", "clauses from MC2", last-txn-id=TXN2
> >> >>
> >> >> Because TXN2 is still in the ovsdb-server transaction history, the
> >> >> server replies with the changes from the most recent transactions
only,
> >> >> i.e., TXN3:
> >> >> result="true", last-txbb-id=TXN3, "modify record R1 in table T1"
> >> >>
> >> >> This causes the IDL on the client in to end up in an inconsistent
> >> >> state because it has never seen the update that created R1.
> >> >>
> >> >> Such a scenario is described in:
> >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1808580#c22
> >> >>
> >> >> To avoid hitting this issue, whenever a reconnect happens (either
> >> >> due to an IDL retry or due to network issues), we clear db->last_id
> >> >> in ovsdb_idl_restart_fsm() in any of the following cases:
> >> >> - a monitor condition has been changed locally and the corresponding
> >> >>   request was not sent yet (i.e., idl->data.cond_changed == true).
> >> >> - a monitor_cond_change request is in flight.
> >> >>
> >> >> This ensures that updates of type "insert" that happened before the
last
> >> >> transaction known by the IDL but didn't match old monitor
conditions are
> >> >> sent upon reconnect if the monitor condition has changed to include
them
> >> >> in the meantime.
> >> >>
> >> >> CC: Han Zhou <hzhou at ovn.org <mailto:hzhou at ovn.org>>
> >> >> Fixes: 403a6a0cb003 ("ovsdb-idl: Fast resync from server when
connection reset.")
> >> >> Signed-off-by: Dumitru Ceara <dceara at redhat.com <mailto:
dceara at redhat.com>>
> >> >> ---
> >> >>  lib/ovsdb-idl.c |   19 +++++++++++++++++++
> >> >>  1 file changed, 19 insertions(+)
> >> >>
> >> >> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> >> >> index 1535ad7..67c4745 100644
> >> >> --- a/lib/ovsdb-idl.c
> >> >> +++ b/lib/ovsdb-idl.c
> >> >> @@ -690,6 +690,25 @@ ovsdb_idl_send_request(struct ovsdb_idl *idl,
struct jsonrpc_msg *request)
> >> >>  static void
> >> >>  ovsdb_idl_restart_fsm(struct ovsdb_idl *idl)
> >> >>  {
> >> >> +    /* If there's an outstanding request of type
monitor_cond_change and
> >> >> +     * we're in monitor_cond_since mode then we can't trust that
all relevant
> >> >> +     * updates from transaction idl->data.last_id have been
received as we
> >> >> +     * might have relaxed the monitor condition with our last
request and
> >> >> +     * might be missing previously not monitored records.
> >> >> +     *
> >> >> +     * Same reasoning applies for the case when a monitor
condition has been
> >> >> +     * changed locally but the monitor_cond_change request was not
sent yet.
> >> >> +     *
> >> >> +     * In both cases, clear last_id to make sure that the next time
> >> >> +     * monitor_cond_since is sent (i.e., after reconnect) we get
the complete
> >> >> +     * view of the database.
> >> >> +     */
> >> >> +    if (idl->data.cond_changed ||
> >> >> +            (idl->request_id &&
> >> >> +                idl->data.monitoring ==
OVSDB_IDL_MONITORING_COND_SINCE)) {
> >> >> +        uuid_zero(&idl->data.last_id);
> >> >
> >> > As Han pointed out during OVN IRC meeting,  when there is a change
to SB, e.g.
> >> > creating a new DP, it causes all HVs changing the condition, and at
the same time
> >> > one of the SB servers might be disconnected triggering this bug on a
big number
> >> > of nodes at once.  If all of these nodes will request the full
database at the
> >> > same time, this might be an issue.
> >> >
> >> > One possibility that came to mind is that we could store 'last_id'
for the
> >> > previous successful cond_change and use it instead of 0 on
re-connection if there
> >> > was in-flight cond_change.  This way we could reduce the pressure on
SB server.
> >>
> >> Thinking more about this, this idea should not work because with new
conditions we
> >> might need to receive changes that happened even before the last
successful cond_change
> >> because we might relax conditions incrementally.
> >>
> >> So, current solution seems the most correct for now.  I'd like to hear
some other
> >> thoughts about this issue with massive re-connections, though, if any.
> >>
> >
> > Ideally, this can be solved by below mechanism:
> > 1. If there is an uncompleted cond_change (either not sent, or
in-flight) while server
> > disconnected, the IDL firsty reconnect with old condition with last_id
being the current
> > data position, so that server knows the old condition first.
> > 2. Then it retry the cond_change request with the new condition. Server
will send all
> > needed updates for the delta between old and new condition.
>
> Yeah, we've been discussing same approach today with Dumitru.
>
> > This may need more complex logic in the IDL implementation, if not too
complex.
>
> Implementation suggestion might be to store "new_conditions" along with
"conditions":
>
> - set_conditions() should set "new_conditions" if they are not equal to
"conditions"
>   or current "new_conditions".
> - monitor_cond_since should always use old "conditions".
> - monitor_cond_change should always send "new_conditions".
>   On reply from the server "conditions" should be freed, "new_conditions"
moved to
>   "conditions" and "new_conditions" cleared.
>
> In this case, on re-connection, both "conditions" and "new_conditions"
will be present
> and two sequential requests (monitor_cond_since, monitor_cond_change)
will be triggered
> naturally by the existing code.
>

Sounds great. Looking forward to v5.

> >
> > Thanks,
> > Han
> >
> >> >
> >> >> +    }
> >> >> +
> >> >>      ovsdb_idl_send_schema_request(idl, &idl->server);
> >> >>      ovsdb_idl_transition(idl, IDL_S_SERVER_SCHEMA_REQUESTED);
> >> >>      idl->data.monitoring = OVSDB_IDL_NOT_MONITORING;
> >> >>
> >> >
> >>
>