On 9/8/21 5:01 AM, Chengang (Russell Lab) wrote: > Hi Ilya, > >> On Tuesday, September 7, 2021 7:56 PM, Ilya Maximets >> [mailto:[email protected]] 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.
I never sent it anywhere, but here it is: https://github.com/igsilya/ovs/commit/d37f663994f2cc3c2de6ed39d81b890fe0aa4f3f The final version of this patch will need changes, because we can't simply switch all the clients from requests to notifications, because new clients will not be able to use db_change_aware feature with old servers. Need to think how to perform this transition. > > Best Regards! > Gang Chen > >> -----Original Message----- >> From: Ilya Maximets [mailto:[email protected]] >> Sent: Tuesday, September 7, 2021 7:56 PM >> To: Chengang (Russell Lab) <[email protected]>; Han Zhou >> <[email protected]>; Ilya Maximets <[email protected]> >> Cc: [email protected]; wangyunjian <[email protected]>; >> dingxiaoxiong <[email protected]>; Dumitru Ceara >> <[email protected]> >> 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:[email protected]] On Behalf Of >>>> Dumitru Ceara >>>> Sent: Friday, July 10, 2020 7:33 PM >>>> To: [email protected] >>>> Cc: Han Zhou <[email protected]>; Ilya Maximets <[email protected]> >>>> 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 <[email protected]> >>>> CC: Han Zhou <[email protected]> >>>> CC: Ilya Maximets <[email protected]> >>>> Reported-by: Girish Moodalbail <[email protected]> >>>> 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 <[email protected]> >>>> --- >>>> 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 >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
