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

Chengang (Russell Lab) chengang65 at huawei.com
Wed Sep 8 03:01:42 UTC 2021


Hi Ilya,

> On Tuesday, September 7, 2021 7:56 PM, Ilya Maximets [mailto:i.maximets at ovn.org] wrote:
> I had a patch somewhere.  I guess, I need to revise it and send.

Can you info me where to get your patch, I would like to test it and reply you the test result.

Best Regards!
Gang Chen

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at ovn.org]
> Sent: Tuesday, September 7, 2021 7:56 PM
> To: Chengang (Russell Lab) <chengang65 at huawei.com>; Han Zhou
> <hzhou at ovn.org>; Ilya Maximets <i.maximets at ovn.org>
> Cc: dev at openvswitch.org; wangyunjian <wangyunjian at huawei.com>;
> dingxiaoxiong <dingxiaoxiong at huawei.com>; Dumitru Ceara
> <dceara at redhat.com>
> Subject: Re: [ovs-dev] [PATCH] ovsdb-idl: Send "set_db_change_aware" before
> "monitor_cond_since".
> 
> 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.h
> >> tml
> >> 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