On 12/8/22 16:21, Dumitru Ceara wrote:
> Applications request specific monitor conditions via the
> ovsdb_cs_set_condition() API. This returns the condition sequence
> number at which the client can assume that the server acknowledged
> and applied the new monitor condition.
>
> A corner case is when the client's first request is to set the condition
> to a value that's equivalent to the default one ("<true>"). In such
> cases, because it's requested explicitly by the client, we now
> explicitly send the monitor_cond_change request to the server. This
> avoids cases in which the expected condition sequence number and the
> acked condition sequence number get out of sync.
Sorry, I think I'm lost again. What exactly we're trying to fix here?
[True] is a default condition if no other conditions are supplied for
the table. Why do we need to send it explicitly?
At first I thought that newly created condition from ovsdb_cs_db_get_table()
has to be sent out, but... if it is the first time we're setting
conditions for that table, default condition should be [True], hence
equal to the condition we're trying to set.
The test case is a bit synthetic as it doesn't seem to test the issue
that exists in OVN. The fact that python IDL passes the test without
changes also doesn't make a lot of sense to me.
Could you provide some more data on what exactly is going on and what
we're trying to fix? With jsonrpc logs, if possible.
Thanks.
Best regards, Ilya Maximets.
>
> Reported-by: Numan Siddique <[email protected]>
> Signed-off-by: Dumitru Ceara <[email protected]>
> ---
> lib/ovsdb-cs.c | 1 +
> tests/ovsdb-idl.at | 27 +++++++++++++++++++++++++++
> tests/test-ovsdb.c | 28 ++++++++++++++++++++++++----
> tests/test-ovsdb.py | 20 +++++++++++++++++---
> 4 files changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index a6fbd290c87d..16c8f6a2cf55 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -894,6 +894,7 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const char
> *table)
> t->name = xstrdup(table);
> t->new_cond = json_array_create_1(json_boolean_create(true));
> hmap_insert(&db->tables, &t->hmap_node, hash);
> + db->cond_changed = true;
> return t;
> }
>
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index c2970984bae3..57ed0ac9d12c 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -599,6 +599,33 @@ OVSDB_CHECK_IDL([simple idl, conditional, true
> condition],
> 004: done
> ]])
>
> +dnl This test ensures that the first explicitly set monitor condition
> +dnl is sent to the server.
> +OVSDB_CHECK_IDL([simple idl, conditional, wait for condition],
> + [],
> + [['["idltest",
> + {"op": "insert",
> + "table": "simple",
> + "row": {"i": 1,
> + "r": 2.0,
> + "b": true}}]' \
> + 'condition simple [true]' \
> + '^["idltest",
> + {"op": "insert",
> + "table": "simple",
> + "row": {"i": 2,
> + "r": 4.0,
> + "b": true}}]']],
> + [[000: empty
> +001: {"error":null,"result":[{"uuid":["uuid","<0>"]}]}
> +002: table simple: i=1 r=2 b=true s= u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
> uuid=<0>
> +003: change conditions
> +004: {"error":null,"result":[{"uuid":["uuid","<2>"]}]}
> +005: table simple: i=1 r=2 b=true s= u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
> uuid=<0>
> +005: table simple: i=2 r=4 b=true s= u=<1> ia=[] ra=[] ba=[] sa=[] ua=[]
> uuid=<2>
> +006: done
> +]])
> +
> OVSDB_CHECK_IDL([simple idl, conditional, multiple clauses in condition],
> [['["idltest",
> {"op": "insert",
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 84fe232765aa..23cfd79a433e 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -2627,11 +2627,12 @@ parse_link2_json_clause(struct ovsdb_idl_condition
> *cond,
> }
> }
>
> -static void
> +static unsigned int
> update_conditions(struct ovsdb_idl *idl, char *commands)
> {
> - char *cmd, *save_ptr1 = NULL;
> const struct ovsdb_idl_table_class *tc;
> + unsigned int next_cond_seqno = 0;
> + char *cmd, *save_ptr1 = NULL;
>
> for (cmd = strtok_r(commands, ";", &save_ptr1); cmd;
> cmd = strtok_r(NULL, ";", &save_ptr1)) {
> @@ -2688,9 +2689,11 @@ update_conditions(struct ovsdb_idl *idl, char
> *commands)
> if (next_seqno != new_next_seqno) {
> ovs_fatal(0, "condition expected seqno changed");
> }
> + next_cond_seqno = MAX(next_cond_seqno, next_seqno);
> ovsdb_idl_condition_destroy(&cond);
> json_destroy(json);
> }
> + return next_cond_seqno;
> }
>
> static void
> @@ -2699,6 +2702,7 @@ do_idl(struct ovs_cmdl_context *ctx)
> struct test_ovsdb_pvt_context *pvt = ctx->pvt;
> struct jsonrpc *rpc;
> struct ovsdb_idl *idl;
> + unsigned int next_cond_seqno = 0;
> unsigned int seqno = 0;
> struct ovsdb_symbol_table *symtab;
> size_t n_uuids = 0;
> @@ -2735,7 +2739,8 @@ do_idl(struct ovs_cmdl_context *ctx)
> const char remote_s[] = "set-remote ";
> const char cond_s[] = "condition ";
> if (ctx->argc > 2 && strstr(ctx->argv[2], cond_s)) {
> - update_conditions(idl, ctx->argv[2] + strlen(cond_s));
> + next_cond_seqno =
> + update_conditions(idl, ctx->argv[2] + strlen(cond_s));
> print_and_log("%03d: change conditions", step++);
> i = 3;
> } else {
> @@ -2755,6 +2760,21 @@ do_idl(struct ovs_cmdl_context *ctx)
> if (*arg == '+') {
> /* The previous transaction didn't change anything. */
> arg++;
> + } else if (*arg == '^') {
> + /* Wait for condition change to be acked by the server. */
> + arg++;
> + for (;;) {
> + ovsdb_idl_run(idl);
> + ovsdb_idl_check_consistency(idl);
> + if (ovsdb_idl_get_condition_seqno(idl) == next_cond_seqno) {
> + break;
> + }
> + jsonrpc_run(rpc);
> +
> + ovsdb_idl_wait(idl);
> + jsonrpc_wait(rpc);
> + poll_block();
> + }
> } else {
> /* Wait for update. */
> for (;;) {
> @@ -2789,7 +2809,7 @@ do_idl(struct ovs_cmdl_context *ctx)
> arg + strlen(remote_s),
> ovsdb_idl_is_connected(idl) ? "true" : "false");
> } else if (!strncmp(arg, cond_s, strlen(cond_s))) {
> - update_conditions(idl, arg + strlen(cond_s));
> + next_cond_seqno = update_conditions(idl, arg + strlen(cond_s));
> print_and_log("%03d: change conditions", step++);
> } else if (arg[0] != '[') {
> if (!idl_set(idl, arg, step++)) {
> diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
> index cca1818ea3aa..86184e6d4e4d 100644
> --- a/tests/test-ovsdb.py
> +++ b/tests/test-ovsdb.py
> @@ -627,6 +627,7 @@ def idl_set(idl, commands, step):
>
>
> def update_condition(idl, commands):
> + expected_seqno = 0
> commands = commands[len("condition "):].split(";")
> for command in commands:
> command = command.split(" ")
> @@ -637,7 +638,8 @@ def update_condition(idl, commands):
> table = command[0]
> cond = ovs.json.from_string(command[1])
>
> - idl.cond_change(table, cond)
> + expected_seqno = max(expected_seqno, idl.cond_change(table, cond))
> + return expected_seqno
>
>
> def do_idl(schema_file, remote, *commands):
> @@ -694,6 +696,7 @@ def do_idl(schema_file, remote, *commands):
> else:
> rpc = None
>
> + next_cond_seqno = 0
> symtab = {}
> seqno = 0
> step = 0
> @@ -717,7 +720,7 @@ def do_idl(schema_file, remote, *commands):
>
> commands = list(commands)
> if len(commands) >= 1 and "condition" in commands[0]:
> - update_condition(idl, commands.pop(0))
> + next_cond_seqno = update_condition(idl, commands.pop(0))
> sys.stdout.write("%03d: change conditions\n" % step)
> sys.stdout.flush()
> step += 1
> @@ -732,6 +735,17 @@ def do_idl(schema_file, remote, *commands):
> if command.startswith("+"):
> # The previous transaction didn't change anything.
> command = command[1:]
> + elif command.startswith("^"):
> + # Wait for condition change to be acked by the server.
> + command = command[1:]
> + while idl.cond_seqno != next_cond_seqno and \
> + not idl.run():
> + rpc.run()
> +
> + poller = ovs.poller.Poller()
> + idl.wait(poller)
> + rpc.wait(poller)
> + poller.block()
> else:
> # Wait for update.
> while idl.change_seqno == seqno and not idl.run():
> @@ -753,7 +767,7 @@ def do_idl(schema_file, remote, *commands):
> step += 1
> idl.force_reconnect()
> elif "condition" in command:
> - update_condition(idl, command)
> + next_cond_seqno = update_condition(idl, command)
> sys.stdout.write("%03d: change conditions\n" % step)
> sys.stdout.flush()
> step += 1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
