[ovs-dev] 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@ovn.org>
CC: Han Zhou <hzhou@ovn.org>
CC: Ilya Maximets <i.maximets@ovn.org>
Reported-by: Girish Moodalbail <gmoodalbail@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@redhat.com>
---
 lib/ovsdb-idl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

On Fri, Jul 10, 2020 at 5:03 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> 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@ovn.org>
> CC: Han Zhou <hzhou@ovn.org>
> CC: Ilya Maximets <i.maximets@ovn.org>
> Reported-by: Girish Moodalbail <gmoodalbail@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@redhat.com>

LGTM,

Acked-by: Numan Siddique <numans@ovn.org>

Thanks
Numan

> ---
>  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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>