ovsdb-cs: Consider default conditions implicitly acked.

When initializing a monitor table the default monitor condition is
[True] which matches the behavior of the server (to send all rows of
that table).  There's no need to include this default condition in the
initial monitor request so we can consider it implicitly acked by the
server.

This fixes the incorrect (one too large) expected condition sequence
number reported by ovsdb_idl_set_condition() when application is
trying to set a [True] condition for a new table.

Reported-by: Numan Siddique <numans@ovn.org>
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

5 files changed, 133 insertions, 53 deletions

diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
index 9713c7dc7..a7d2f73cc 100644
--- a/lib/ovsdb-cs.c
+++ b/lib/ovsdb-cs.c
@@ -892,7 +892,7 @@ ovsdb_cs_db_get_table(struct ovsdb_cs_db *db, const char *table)
 
     t = xzalloc(sizeof *t);
     t->name = xstrdup(table);
-    t->new_cond = json_array_create_1(json_boolean_create(true));
+    t->ack_cond = json_array_create_1(json_boolean_create(true));
     hmap_insert(&db->tables, &t->hmap_node, hash);
     return t;
 }
diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 8e31e02d7..08846b0d5 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -85,9 +85,9 @@ class Monitor(enum.IntEnum):
 
 class ConditionState(object):
     def __init__(self):
-        self._ack_cond = None
+        self._ack_cond = [True]
         self._req_cond = None
-        self._new_cond = [True]
+        self._new_cond = None
 
     def __iter__(self):
         return iter([self._new_cond, self._req_cond, self._ack_cond])
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index 19493a925..092d9f81a 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -561,9 +561,9 @@ OVSDB_CHECK_IDL([simple idl, conditional, false condition],
                "b": true}}]']],
   [['condition simple []' \
     'condition simple [true]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 004: done
 ]])
@@ -577,13 +577,40 @@ OVSDB_CHECK_IDL([simple idl, conditional, true condition],
                "b": true}}]']],
   [['condition simple []' \
     'condition simple [true]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 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: simple: conditions unchanged
+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",
@@ -598,9 +625,9 @@ OVSDB_CHECK_IDL([simple idl, conditional, multiple clauses in condition],
                "b": true}}]']],
   [['condition simple []' \
     'condition simple [["i","==",1],["i","==",2]]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 003: table simple: i=2 r=3 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
 004: done
@@ -615,9 +642,9 @@ OVSDB_CHECK_IDL([simple idl, conditional, modify as insert due to condition],
                "b": true}}]']],
   [['condition simple []' \
     'condition simple [["i","==",1]]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 004: done
 ]])
@@ -638,11 +665,11 @@ OVSDB_CHECK_IDL([simple idl, conditional, modify as delete due to condition],
        "row": {"i": 2,
                "r": 3.0,
                "b": true}}]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
-004: change conditions
+004: simple: change conditions
 005: empty
 006: {"error":null,"result":[{"uuid":["uuid","<2>"]}]}
 007: table simple: i=2 r=3 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
@@ -673,14 +700,16 @@ OVSDB_CHECK_IDL([simple idl, conditional, multiple tables],
        "table": "link2",
        "row": {"i": 3},
         "uuid-name": "row0"}]']],
-  [[000: change conditions
+  [[000: link1: change conditions
+000: link2: change conditions
+000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
-004: change conditions
+004: link1: change conditions
 005: table link1: i=0 k=0 ka=[] l2= uuid=<2>
 005: table simple: i=1 r=2 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
-006: change conditions
+006: link2: change conditions
 007: {"error":null,"result":[{"uuid":["uuid","<3>"]}]}
 008: table link1: i=0 k=0 ka=[] l2= uuid=<2>
 008: table link2: i=3 l1= uuid=<3>
@@ -1237,10 +1266,10 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak refer
       {"op": "delete",
       "table": "simple6",
       "where": []}]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: table simple6: inserted row: name=first_row weak_ref=[] uuid=<0>
 001: table simple6: updated columns: name weak_ref
-002: change conditions
+002: simple: change conditions
 003: table simple6: name=first_row weak_ref=[<1>] uuid=<0>
 003: table simple: inserted row: i=0 r=0 b=false s=row1_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 003: table simple: updated columns: s
@@ -1279,19 +1308,19 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan rows, cond
       {"op": "delete",
       "table": "simple6",
       "where": []}]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: table simple6: inserted row: name=first_row weak_ref=[] uuid=<0>
 001: table simple6: updated columns: name weak_ref
-002: change conditions
+002: simple: change conditions
 003: table simple6: name=first_row weak_ref=[<1>] uuid=<0>
 003: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 003: table simple: updated columns: s
-004: change conditions
+004: simple: change conditions
 005: table simple6: name=first_row weak_ref=[] uuid=<0>
 005: table simple: deleted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 005: table simple: inserted row: i=0 r=0 b=false s=row1_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<3>
 005: table simple: updated columns: s
-006: change conditions
+006: simple: change conditions
 007: table simple6: name=first_row weak_ref=[<1>] uuid=<0>
 007: table simple: deleted row: i=0 r=0 b=false s=row1_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<3>
 007: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
@@ -1333,14 +1362,14 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, references, condi
       {"op": "delete",
       "table": "simple6",
       "where": []}]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: table simple6: inserted row: name=first_row weak_ref=[] uuid=<0>
 001: table simple6: updated columns: name weak_ref
-002: change conditions
+002: simple: change conditions
 003: table simple6: name=first_row weak_ref=[<1>] uuid=<0>
 003: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 003: table simple: updated columns: s
-004: change conditions
+004: simple: change conditions
 005: table simple6: name=first_row weak_ref=[<3>] uuid=<0>
 005: table simple: deleted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
 005: table simple: inserted row: i=1 r=0 b=false s=row1_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<3>
@@ -1376,7 +1405,8 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, references, singl
       {"op": "insert",
        "table": "simple",
        "row": {"s": "row0_s"}}]']],
-  [[000: change conditions
+  [[000: simple6: conditions unchanged
+000: simple: conditions unchanged
 001: table simple6: inserted row: name=row0_s6 weak_ref=[<0>] uuid=<1>
 001: table simple6: updated columns: name weak_ref
 001: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<0>
@@ -1418,7 +1448,8 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, weak references,
       {"op": "insert",
        "table": "simple",
        "row": {"s": "row0_s"}}]']],
-  [[000: change conditions
+  [[000: simple6: conditions unchanged
+000: simple: conditions unchanged
 001: table simple6: inserted row: name=row0_s6 weak_ref=[<0>] uuid=<1>
 001: table simple6: updated columns: name weak_ref
 001: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<0>
@@ -1458,7 +1489,9 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, strong references
       {"op": "insert",
        "table": "simple",
        "row": {"s": "row0_s"}}]']],
-  [[000: change conditions
+  [[000: simple3: conditions unchanged
+000: simple4: conditions unchanged
+000: simple: conditions unchanged
 001: table simple3: inserted row: name=row0_s3 uset=[] uref=[<0>] uuid=<1>
 001: table simple3: updated columns: name uref
 001: table simple4: inserted row: name=row0_s4 uuid=<0>
@@ -1493,12 +1526,14 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, strong references
       {"op": "insert",
        "table": "simple",
        "row": {"s": "row0_s"}}]']],
-  [[000: change conditions
+  [[000: simple3: conditions unchanged
+000: simple4: conditions unchanged
+000: simple: conditions unchanged
 001: table simple3: inserted row: name=row0_s3 uset=[] uref=[<0>] uuid=<1>
 001: table simple3: updated columns: name uref
 001: table simple4: inserted row: name=row0_s4 uuid=<0>
 001: table simple4: updated columns: name
-002: change conditions
+002: simple4: change conditions
 003: table simple3: name=row0_s3 uset=[] uref=[] uuid=<1>
 003: table simple4: deleted row: name=row0_s4 uuid=<0>
 004: {"error":null,"result":[{"uuid":["uuid","<2>"]}]}
@@ -1529,10 +1564,12 @@ OVSDB_CHECK_IDL([simple idl, initially populated, strong references, conditional
       {"op": "insert",
        "table": "simple",
        "row": {"s": "row0_s"}}]']],
-  [[000: change conditions
+  [[000: simple3: conditions unchanged
+000: simple4: conditions unchanged
+000: simple: conditions unchanged
 001: table simple3: name=row0_s3 uset=[] uref=[<0>] uuid=<1>
 001: table simple4: name=row0_s4 uuid=<0>
-002: change conditions
+002: simple4: change conditions
 003: table simple3: name=row0_s3 uset=[] uref=[] uuid=<1>
 004: {"error":null,"result":[{"uuid":["uuid","<2>"]}]}
 005: table simple3: name=row0_s3 uset=[] uref=[] uuid=<1>
@@ -2341,11 +2378,11 @@ OVSDB_CHECK_CLUSTER_IDL([simple idl, monitor_cond_since, cluster disconnect],
        "table": "simple",
        "where": [["i", "==", 1]],
        "row": {"r": 2.0 }}]']],
-  [[000: change conditions
+  [[000: simple: change conditions
 001: empty
-002: change conditions
+002: simple: change conditions
 003: table simple: i=2 r=1 b=true s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<1>
-004: change conditions
+004: simple: change conditions
 005: reconnect
 006: table simple
 007: {"error":null,"result":[{"count":1}]}
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index c7bc32a24..e725cd498 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -2605,11 +2605,12 @@ parse_link2_json_clause(struct ovsdb_idl_condition *cond,
     }
 }
 
-static void
-update_conditions(struct ovsdb_idl *idl, char *commands)
+static unsigned int
+update_conditions(struct ovsdb_idl *idl, char *commands, int step)
 {
-    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)) {
@@ -2660,15 +2661,20 @@ update_conditions(struct ovsdb_idl *idl, char *commands)
         unsigned int seqno = ovsdb_idl_get_condition_seqno(idl);
         unsigned int next_seqno = ovsdb_idl_set_condition(idl, tc, &cond);
         if (seqno == next_seqno ) {
-            ovs_fatal(0, "condition unchanged");
+            print_and_log("%03d: %s: conditions unchanged",
+                          step, table_name);
+        } else {
+            print_and_log("%03d: %s: change conditions", step, table_name);
         }
         unsigned int new_next_seqno = ovsdb_idl_set_condition(idl, tc, &cond);
         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
@@ -2676,6 +2682,7 @@ do_idl(struct ovs_cmdl_context *ctx)
 {
     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;
@@ -2711,8 +2718,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));
-        print_and_log("%03d: change conditions", step++);
+        next_cond_seqno =
+            update_conditions(idl, ctx->argv[2] + strlen(cond_s), step++);
         i = 3;
     } else {
         i = 2;
@@ -2731,6 +2738,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 (;;) {
@@ -2765,8 +2787,8 @@ 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));
-            print_and_log("%03d: change conditions", step++);
+            next_cond_seqno = update_conditions(idl, arg + strlen(cond_s),
+                                                step++);
         } else if (arg[0] != '[') {
             idl_set(idl, arg, step++);
         } else {
diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
index 402cacbe9..eded9140e 100644
--- a/tests/test-ovsdb.py
+++ b/tests/test-ovsdb.py
@@ -616,7 +616,8 @@ def idl_set(idl, commands, step):
     sys.stdout.flush()
 
 
-def update_condition(idl, commands):
+def update_condition(idl, commands, step):
+    next_cond_seqno = 0
     commands = commands[len("condition "):].split(";")
     for command in commands:
         command = command.split(" ")
@@ -627,7 +628,20 @@ def update_condition(idl, commands):
         table = command[0]
         cond = ovs.json.from_string(command[1])
 
-        idl.cond_change(table, cond)
+        next_seqno = idl.cond_change(table, cond)
+        if idl.cond_seqno == next_seqno:
+            sys.stdout.write("%03d: %s: conditions unchanged\n" %
+                             (step, table))
+        else:
+            sys.stdout.write("%03d: %s: change conditions\n" %
+                             (step, table))
+        sys.stdout.flush()
+
+        assert next_seqno == idl.cond_change(table, cond), \
+            "condition expected seqno changed"
+        next_cond_seqno = max(next_cond_seqno, next_seqno)
+
+    return next_cond_seqno
 
 
 def do_idl(schema_file, remote, *commands):
@@ -684,6 +698,7 @@ def do_idl(schema_file, remote, *commands):
     else:
         rpc = None
 
+    next_cond_seqno = 0
     symtab = {}
     seqno = 0
     step = 0
@@ -707,9 +722,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))
-        sys.stdout.write("%03d: change conditions\n" % step)
-        sys.stdout.flush()
+        next_cond_seqno = update_condition(idl, commands.pop(0), step)
         step += 1
 
     for command in commands:
@@ -722,6 +735,16 @@ 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():
@@ -743,9 +766,7 @@ def do_idl(schema_file, remote, *commands):
             step += 1
             idl.force_reconnect()
         elif "condition" in command:
-            update_condition(idl, command)
-            sys.stdout.write("%03d: change conditions\n" % step)
-            sys.stdout.flush()
+            next_cond_seqno = update_condition(idl, command, step)
             step += 1
         elif not command.startswith("["):
             idl_set(idl, command, step)