| Message ID | 1482227237-61638-2-git-send-email-azhou@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Dec 20, 2016 at 01:47:16AM -0800, Andy Zhou wrote: > From: andy zhou <azhou@ovn.org> > > When generating conditional monitoring update request, current code > failed to update idl's 'request-id'. This bug causes the reply > message of the update request, regardless an ACK or a NACK, be > logged as an unexpected message at the debug level and ignored by > the core idl logic. > > In addition, the idl should not generate another conditional > monitoring update request when there is an outstanding request. > So that the requests and their reply are properly serialized. > > When the conditional monitoring is nacked by the server, drop idl > into a client visible error state. > > Signed-off-by: Andy Zhou <azhou@ovn.org> When the condition is changed when an update request is already in flight, and the update request completes, will the code properly send a new condition change at that point?
On Thu, Dec 22, 2016 at 9:21 AM, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Dec 20, 2016 at 01:47:16AM -0800, Andy Zhou wrote: > > From: andy zhou <azhou@ovn.org> > > > > When generating conditional monitoring update request, current code > > failed to update idl's 'request-id'. This bug causes the reply > > message of the update request, regardless an ACK or a NACK, be > > logged as an unexpected message at the debug level and ignored by > > the core idl logic. > > > > In addition, the idl should not generate another conditional > > monitoring update request when there is an outstanding request. > > So that the requests and their reply are properly serialized. > > > > When the conditional monitoring is nacked by the server, drop idl > > into a client visible error state. > > > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > When the condition is changed when an update request is already in > flight, and the update request completes, will the code properly send > a new condition change at that point? > When an ack is received, the request_id will be freed and set to NULL, thus unblocking ovsdb_idl_send_cond_changge() to send out the next condition change. Am I still missing something?
On Wed, Jan 4, 2017 at 3:55 PM, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Dec 22, 2016 at 12:12:05PM -0800, Andy Zhou wrote: > > On Thu, Dec 22, 2016 at 9:21 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > > On Tue, Dec 20, 2016 at 01:47:16AM -0800, Andy Zhou wrote: > > > > From: andy zhou <azhou@ovn.org> > > > > > > > > When generating conditional monitoring update request, current code > > > > failed to update idl's 'request-id'. This bug causes the reply > > > > message of the update request, regardless an ACK or a NACK, be > > > > logged as an unexpected message at the debug level and ignored by > > > > the core idl logic. > > > > > > > > In addition, the idl should not generate another conditional > > > > monitoring update request when there is an outstanding request. > > > > So that the requests and their reply are properly serialized. > > > > > > > > When the conditional monitoring is nacked by the server, drop idl > > > > into a client visible error state. > > > > > > > > Signed-off-by: Andy Zhou <azhou@ovn.org> > > > > > > When the condition is changed when an update request is already in > > > flight, and the update request completes, will the code properly send > > > a new condition change at that point? > > > > > > > When an ack is received, the request_id will be freed and set to NULL, > thus > > unblocking ovsdb_idl_send_cond_changge() to send out the next condition > > change. > > > > Am I still missing something? > > I asked the question because I hadn't looked, and it seemed like an easy > mistake to make. I agree that it seems OK. However, I think that the > loop in ovsdb_idl_run() can reset request_id without calling > ovsdb_idl_send_cond_change() afterward. That'll happen on the next trip > through the main loop but at least in theory that could happen much > later, so perhaps we should call it right away, e.g.: > > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c > index 341951d..82c8584 100644 > --- a/lib/ovsdb-idl.c > +++ b/lib/ovsdb-idl.c > @@ -458,6 +458,7 @@ ovsdb_idl_run(struct ovsdb_idl *idl) > > case IDL_S_MONITORING_COND: > /* Conditional monitor clauses were updated. */ > + ovsdb_idl_send_cond_change(idl); > break; > > case IDL_S_MONITORING: > > Probably the VLOG calls should be rate-limited. > Thanks for the suggestion. I folded this change into v2.
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c index 218b6b3..341951d 100644 --- a/lib/ovsdb-idl.c +++ b/lib/ovsdb-idl.c @@ -456,8 +456,11 @@ ovsdb_idl_run(struct ovsdb_idl *idl) idl->schema = NULL; break; - case IDL_S_MONITORING: case IDL_S_MONITORING_COND: + /* Conditional monitor clauses were updated. */ + break; + + case IDL_S_MONITORING: case IDL_S_NO_SCHEMA: default: OVS_NOT_REACHED(); @@ -496,14 +499,23 @@ ovsdb_idl_run(struct ovsdb_idl *idl) idl->state = IDL_S_MONITOR_REQUESTED; } } else if (msg->type == JSONRPC_ERROR + && idl->state == IDL_S_MONITORING_COND + && idl->request_id + && json_equal(idl->request_id, msg->id)) { + json_destroy(idl->request_id); + idl->request_id = NULL; + VLOG_ERR("%s: conditional monitor update failed", + jsonrpc_session_get_name(idl->session)); + idl->state = IDL_S_NO_SCHEMA; + } else if (msg->type == JSONRPC_ERROR && idl->state == IDL_S_SCHEMA_REQUESTED && idl->request_id && json_equal(idl->request_id, msg->id)) { - json_destroy(idl->request_id); - idl->request_id = NULL; - VLOG_ERR("%s: requested schema not found", - jsonrpc_session_get_name(idl->session)); - idl->state = IDL_S_NO_SCHEMA; + json_destroy(idl->request_id); + idl->request_id = NULL; + VLOG_ERR("%s: requested schema not found", + jsonrpc_session_get_name(idl->session)); + idl->state = IDL_S_NO_SCHEMA; } else if ((msg->type == JSONRPC_ERROR || msg->type == JSONRPC_REPLY) && ovsdb_idl_txn_process_reply(idl, msg)) { @@ -1091,8 +1103,11 @@ ovsdb_idl_send_cond_change(struct ovsdb_idl *idl) struct json *params, *json_uuid; struct jsonrpc_msg *request; + /* When 'idl-request_id' is not NULL, there is an outstanding + * conditional monitoring update request that we have not heard + * from the server yet. Don't generate another request in this case. */ if (!idl->cond_changed || !jsonrpc_session_is_connected(idl->session) || - idl->state != IDL_S_MONITORING_COND) { + idl->state != IDL_S_MONITORING_COND || idl->request_id) { return; } @@ -1128,7 +1143,8 @@ ovsdb_idl_send_cond_change(struct ovsdb_idl *idl) params = json_array_create_3(json_uuid, json_string_create(uuid), monitor_cond_change_requests); - request = jsonrpc_create_request("monitor_cond_change", params, NULL); + request = jsonrpc_create_request("monitor_cond_change", params, + &idl->request_id); jsonrpc_session_send(idl->session, request); } idl->cond_changed = false;