On 4/24/20 10:23 PM, Han Zhou wrote: > > > On Thu, Apr 23, 2020 at 12:04 PM Ilya Maximets <i.maxim...@ovn.org > <mailto:i.maxim...@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 <hz...@ovn.org <mailto:hz...@ovn.org>> >> >> Fixes: 403a6a0cb003 ("ovsdb-idl: Fast resync from server when connection >> >> reset.") >> >> Signed-off-by: Dumitru Ceara <dce...@redhat.com >> >> <mailto:dce...@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. > > 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; >> >> >> > >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
