[ovs-dev] [PATCH] ovsdb-idl: Send "set_db_change_aware" before "monitor_cond_since".

Ilya Maximets i.maximets at ovn.org
Tue Sep 7 11:56:04 UTC 2021


On 9/6/21 3:51 PM, Chengang (Russell Lab) wrote:
> Hi Han and Ilya,
> 
> This patch fix the issue: IDL client closes the connection after got the response to the monitor_cond_since request, without wait for the reponse to the set_db_change_aware request. Warning message will be add to ovsdb-server.log
> 
> It makes user confused the issue will occurred even with the command "ovs-vsctl show" or ovs-vsctl other commands.
> 
> Is there any other ideas on this issue?

The root cause of a problem is that set_db_change_aware request is hacked
into the IDL state machine and IDL doesn't wait for reply and doesn't
handle the reply in any way.  Moving this hack around the IDl code doesn't
seem to be a good solution as it interferes with the rest of the state
machine which we definitely don't want to accidentally break.

The correct way to eliminate these benign warnings, as we discussed previously,
is to create a new set_db_change_aware notification (not a request), so it will
not require sending reply from a server.  This will fix the problem and will
also make IDL code logically correct.  The downside of this is that if new
clients will connect to old servers, they will not be able to use the feature
if notification is used.  So, there should be some transition period until we
can enable use of notifications by default.

I had a patch somewhere.  I guess, I need to revise it and send.

> If this patch can fix it, can we back port this patch to 2.13.0 as it has the same issue?

The issue exists starting from 2.9, however, the correct solution (to use
notifications) doesn't seem backportable.

Best regards, Ilya Maximets.

> 
> Best Regards!
> Gang Chen
> 
>> -----Original Message-----
>> From: dev [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Dumitru
>> Ceara
>> Sent: Friday, July 10, 2020 7:33 PM
>> To: dev at openvswitch.org
>> Cc: Han Zhou <hzhou at ovn.org>; Ilya Maximets <i.maximets at ovn.org>
>> Subject: [ovs-dev] [PATCH] ovsdb-idl: Send "set_db_change_aware" before
>> "monitor_cond_since".
>>
>> For short lived IDL clients (e.g., ovn-sbctl) if the client sends
>> monitor_cond_since before set_db_change_aware, the client might close the
>> DB connection immediately after it received the reply for monitor_cond_since
>> and before the server has a chance to reply to set_db_change_aware.
>>
>> E.g., from the logs of the ovsdb-server:
>> 2020-07-10T09:29:52.649Z|04479|jsonrpc|DBG|unix#72: received request,
>> method="monitor_cond_since", params=["OVN_Southbound",
>> ["monid","OVN_Southbound"],{"SB_Global":[{"columns":["options"]}]},
>> "00000000-0000-0000-0000-000000000000"], id=2
>> 2020-07-10T09:29:52.649Z|04480|jsonrpc|DBG|unix#72: send reply,
>> result=[false,"00000000-0000-0000-0000-000000000000",
>> {"SB_Global":{"6ad26b48-a742-4fe1-8671-3975e2146ce6":{"initial":
>> {"options":["map",[["mac_prefix","be:85:cb"],["svc_monitor_mac",
>> "52:58:b5:19:8c:40"]]]}}}}], id=2
>> 2020-07-10T09:29:52.649Z|04482|jsonrpc|DBG|unix#72: received request,
>> method="set_db_change_aware", params=[true], id=3
>>
>> <<< IDL client closes the connection here because it already got the response
>> to the monitor_cond_since request.
>>
>> 2020-07-10T09:29:59.023Z|04483|jsonrpc|DBG|unix#72: send reply, result={},
>> id=3
>> 2020-07-10T09:29:59.023Z|04484|stream_fd|DBG|send: Broken pipe
>> 2020-07-10T09:29:59.023Z|04485|jsonrpc|WARN|unix#72: send error: Broken
>> pipe
>>
>> While this is not a critical issue, it can be easily mitigated by changing the IDL
>> client to always send "set_db_change_aware" before "monitor_cond_since".
>> This way we ensure that a well behaving IDL client doesn't close the connection
>> too early, avoiding the error logs in ovsdb-server.
>>
>> This patch moves the code to send monitor_cond_since(data) from function
>> ovsdb_idl_check_server_db() to ovsdb_idl_process_response() as we can
>> transition to IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED only upon
>> receiving a reply for monitor_cond(server).
>>
>> CC: Ben Pfaff <blp at ovn.org>
>> CC: Han Zhou <hzhou at ovn.org>
>> CC: Ilya Maximets <i.maximets at ovn.org>
>> Reported-by: Girish Moodalbail <gmoodalbail at gmail.com>
>> Reported-at:
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2020-July/050343.html
>> Fixes: 1b1d2e6daa56 ("ovsdb: Introduce experimental support for clustered
>> databases.")
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>> ---
>>  lib/ovsdb-idl.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index ef3b97b..c6427f5 100644
>> --- a/lib/ovsdb-idl.c
>> +++ b/lib/ovsdb-idl.c
>> @@ -770,6 +770,10 @@ ovsdb_idl_process_response(struct ovsdb_idl *idl,
>> struct jsonrpc_msg *msg)
>>
>> OVSDB_IDL_MM_MONITOR_COND);
>>              if (ovsdb_idl_check_server_db(idl)) {
>>                  ovsdb_idl_send_db_change_aware(idl);
>> +                ovsdb_idl_send_monitor_request(
>> +                    idl, &idl->data,
>> OVSDB_IDL_MM_MONITOR_COND_SINCE);
>> +                ovsdb_idl_transition(
>> +                    idl,
>> IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED);
>>              }
>>          } else {
>>              ovsdb_idl_send_schema_request(idl, &idl->data); @@ -2057,9
>> +2061,6 @@ ovsdb_idl_check_server_db(struct ovsdb_idl *idl)
>>      if (idl->state == IDL_S_SERVER_MONITOR_COND_REQUESTED) {
>>          json_destroy(idl->data.schema);
>>          idl->data.schema = json_from_string(database->schema);
>> -        ovsdb_idl_send_monitor_request(idl, &idl->data,
>> -
>> OVSDB_IDL_MM_MONITOR_COND_SINCE);
>> -        ovsdb_idl_transition(idl,
>> IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED);
>>      }
>>      return true;
>>  }
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



More information about the dev mailing list