diff mbox series

[ovs-dev,v5,3/4] OVN-IC: Make it possible for CMS to detect when the ISB is up-to-date.

Message ID 20240124142740.969176-4-mheib@redhat.com
State Accepted
Headers show
Series OVN-IC: Add basic sequence number status support. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Mohammad Heib Jan. 24, 2024, 2:27 p.m. UTC
Until now, there has been no reliable for the CMS to detect when
changes made to the INB configuration have been passed through
to the ISB, This commit adds this feature to the system,
by adding sequence numbers to the INB and ISB and adding code
in ovn-ic-nbctl, ovn-ic to keep those sequence numbers up-to-date.

The biggest user-visible change from this commit is a new option
'--wait' and new command 'sync' to ovn-ic-nbctl.
With --wait=sb, ovn-ic-nbctl now waits for ovn-ics to update the ISB
database.

Signed-off-by: Mohammad Heib <mheib@redhat.com>
Acked-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Ales Musil <amusil@redhat.com>
---
 utilities/ovn-ic-nbctl.8.xml | 49 ++++++++++++++++++++
 utilities/ovn-ic-nbctl.c     | 86 +++++++++++++++++++++++++++++++++++-
 2 files changed, 133 insertions(+), 2 deletions(-)

Comments

0-day Robot Jan. 24, 2024, 2:45 p.m. UTC | #1
Bleep bloop.  Greetings Mohammad Heib, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#149 FILE: utilities/ovn-ic-nbctl.c:355:
Synchronization command (use with --wait=sb):\n\

WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#155 FILE: utilities/ovn-ic-nbctl.c:361:
  --no-wait, --wait=none      do not wait for OVN reconfiguration (default)\n\

WARNING: Line lacks whitespace around operator
WARNING: Line lacks whitespace around operator
#156 FILE: utilities/ovn-ic-nbctl.c:362:
  --wait=sb                   wait for southbound database update\n\

Lines checked: 268, Warnings: 7, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/utilities/ovn-ic-nbctl.8.xml b/utilities/ovn-ic-nbctl.8.xml
index 4a70994b8..5a1324d2d 100644
--- a/utilities/ovn-ic-nbctl.8.xml
+++ b/utilities/ovn-ic-nbctl.8.xml
@@ -123,9 +123,58 @@ 
       </dd>
     </dl>
 
+    <h1>Synchronization Commands</h1>
+
+    <dl>
+      <dt>sync</dt>
+      <dd>
+        Ordinarily, <code>--wait=sb</code> only waits for changes by the
+        current <code>ovn-ic-nbctl</code> invocation to take effect.
+        This means that, if none of the commands supplied to
+        <code>ovn-ic-nbctl</code> change the database, then the command does
+        not wait at all.  With the <code>sync</code> command, however,
+        <code>ovn-ic-nbctl</code> waits even for earlier changes to the
+        database to propagate down to the southbound database, according to the
+        argument of <code>--wait</code>.
+      </dd>
+    </dl>
+
     <h1>Options</h1>
 
     <dl>
+      <dt><code>--no-wait</code> | <code>--wait=none</code></dt>
+      <dt><code>--wait=sb</code></dt>
+
+      <dd>
+        <p>
+          These options control whether and how <code>ovn-ic-nbctl</code> waits
+          for the OVN system to become up-to-date with changes made in an
+          <code>ovn-ic-nbctl</code> invocation.
+        </p>
+
+        <p>
+          By default, or if <code>--no-wait</code> or <code>--wait=none</code>,
+          <code>ovn-ic-nbctl</code> exits immediately after confirming that
+          changes have been committed to the Interconnect northbound database,
+          without waiting.
+        </p>
+
+        <p>
+          With <code>--wait=sb</code>, before <code>ovn-ic-nbctl</code> exits,
+          it waits for <code>ovn-ics</code> to bring the Interconnect
+          southbound database up-to-date with the Interconnect northbound
+          database updates.
+        </p>
+
+        <p>
+          Ordinarily, <code>--wait=sb</code> only waits for changes by the
+          current <code>ovn-ic-nbctl</code> invocation to take effect.
+          This means that, if none of the commands supplied to
+          <code>ovn-ic-nbctl</code> change the database, then the command
+          does not wait at all.
+          Use the <code>sync</code> command to override this behavior.
+        </p>
+      </dd>
     <dt><code>--db</code> <var>database</var></dt>
     <dd>
       The OVSDB database remote to contact.  If the <env>OVN_IC_NB_DB</env>
diff --git a/utilities/ovn-ic-nbctl.c b/utilities/ovn-ic-nbctl.c
index 721dc4586..4317c385a 100644
--- a/utilities/ovn-ic-nbctl.c
+++ b/utilities/ovn-ic-nbctl.c
@@ -58,6 +58,13 @@  static bool oneline;
 /* --dry-run: Do not commit any changes. */
 static bool dry_run;
 
+/* --wait=TYPE: Wait for configuration change to take effect? */
+static enum nbctl_wait_type wait_type = NBCTL_WAIT_NONE;
+
+/* Should we wait (if specified by 'wait_type') even if the commands don't
+ * change the database at all */
+static bool force_wait = false;
+
 /* --timeout: Time to wait for a connection to 'db'. */
 static unsigned int timeout;
 
@@ -161,6 +168,8 @@  parse_options(int argc, char *argv[], struct shash *local_options)
         OPT_DB = UCHAR_MAX + 1,
         OPT_ONELINE,
         OPT_NO_SYSLOG,
+        OPT_NO_WAIT,
+        OPT_WAIT,
         OPT_DRY_RUN,
         OPT_LOCAL,
         OPT_COMMANDS,
@@ -173,6 +182,8 @@  parse_options(int argc, char *argv[], struct shash *local_options)
     static const struct option global_long_options[] = {
         {"db", required_argument, NULL, OPT_DB},
         {"no-syslog", no_argument, NULL, OPT_NO_SYSLOG},
+        {"no-wait", no_argument, NULL, OPT_NO_WAIT},
+        {"wait", required_argument, NULL, OPT_WAIT},
         {"dry-run", no_argument, NULL, OPT_DRY_RUN},
         {"oneline", no_argument, NULL, OPT_ONELINE},
         {"timeout", required_argument, NULL, 't'},
@@ -234,7 +245,19 @@  parse_options(int argc, char *argv[], struct shash *local_options)
         case OPT_DRY_RUN:
             dry_run = true;
             break;
-
+        case OPT_NO_WAIT:
+            wait_type = NBCTL_WAIT_NONE;
+            break;
+        case OPT_WAIT:
+            if (!strcmp(optarg, "none")) {
+                wait_type = NBCTL_WAIT_NONE;
+            } else if (!strcmp(optarg, "sb")) {
+                wait_type = NBCTL_WAIT_SB;
+            } else {
+                ctl_fatal("argument to --wait must be "
+                          "\"none\", \"sb\" ");
+            }
+            break;
         case OPT_LOCAL:
             if (shash_find(local_options, options[idx].name)) {
                 ctl_fatal("'%s' option specified multiple times",
@@ -329,9 +352,14 @@  set the SSL configuration\n\
 %s\
 %s\
 \n\
+Synchronization command (use with --wait=sb):\n\
+  sync                     wait even for earlier changes to take effect\n\
+\n\
 Options:\n\
   --db=DATABASE               connect to DATABASE\n\
                               (default: %s)\n\
+  --no-wait, --wait=none      do not wait for OVN reconfiguration (default)\n\
+  --wait=sb                   wait for southbound database update\n\
   --no-leader-only            accept any cluster member, not just the leader\n\
   -t, --timeout=SECS          wait at most SECS seconds\n\
   --dry-run                   do not commit changes to database\n\
@@ -380,6 +408,12 @@  ic_nbctl_init(struct ctl_context *ctx OVS_UNUSED)
 {
 }
 
+static void
+ic_nbctl_pre_sync(struct ctl_context *base OVS_UNUSED)
+{
+    force_wait = true;
+}
+
 static void
 ic_nbctl_ts_add(struct ctl_context *ctx)
 {
@@ -705,6 +739,33 @@  ic_nbctl_context_done_command(struct ic_nbctl_context *ic_nbctl_ctx,
     ctl_context_done_command(&ic_nbctl_ctx->base, command);
 }
 
+static void
+ic_nbctl_validate_sequence_numbers(int expected_cfg,
+                                   struct ovsdb_idl *idl)
+{
+    if (wait_type == NBCTL_WAIT_NONE) {
+        if (force_wait) {
+            VLOG_INFO("\"sync\" command has no effect without --wait");
+        }
+        return;
+    }
+
+    const struct icnbrec_ic_nb_global *inb = icnbrec_ic_nb_global_first(idl);
+    ovsdb_idl_enable_reconnect(idl);
+    for (;;) {
+        ovsdb_idl_run(idl);
+        ICNBREC_IC_NB_GLOBAL_FOR_EACH (inb, idl) {
+            int64_t cur_cfg = inb->sb_ic_cfg;
+            int64_t delta = cur_cfg - expected_cfg;
+            if (cur_cfg >= expected_cfg && delta < INT32_MAX) {
+                return;
+            }
+        }
+        ovsdb_idl_wait(idl);
+        poll_block();
+    }
+}
+
 static void
 ic_nbctl_context_done(struct ic_nbctl_context *ic_nbctl_ctx,
                    struct ctl_command *command)
@@ -748,6 +809,7 @@  do_ic_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
     struct ic_nbctl_context ic_nbctl_ctx;
     struct ctl_command *c;
     struct shash_node *node;
+    int64_t next_cfg = 0;
 
     txn = the_idl_txn = ovsdb_idl_txn_create(idl);
     if (dry_run) {
@@ -762,6 +824,17 @@  do_ic_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
         icnbrec_ic_nb_global_insert(txn);
     }
 
+    if (wait_type != NBCTL_WAIT_NONE) {
+
+        /* Deal with potential overflows. */
+        if (inb->nb_ic_cfg == LLONG_MAX) {
+            icnbrec_ic_nb_global_set_nb_ic_cfg(inb, 0);
+        }
+        ovsdb_idl_txn_increment(txn, &inb->header_,
+                                &icnbrec_ic_nb_global_col_nb_ic_cfg,
+                                force_wait);
+    }
+
     symtab = ovsdb_symbol_table_create();
     for (c = commands; c < &commands[n_commands]; c++) {
         ds_init(&c->output);
@@ -807,6 +880,10 @@  do_ic_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
     }
 
     status = ovsdb_idl_txn_commit_block(txn);
+    if (wait_type != NBCTL_WAIT_NONE && status == TXN_SUCCESS) {
+        next_cfg = ovsdb_idl_txn_get_increment_new_value(txn);
+    }
+
     if (status == TXN_UNCHANGED || status == TXN_SUCCESS) {
         for (c = commands; c < &commands[n_commands]; c++) {
             if (c->syntax->postprocess) {
@@ -884,6 +961,11 @@  do_ic_nbctl(const char *args, struct ctl_command *commands, size_t n_commands,
         shash_destroy_free_data(&c->options);
     }
     free(commands);
+
+    if (status != TXN_UNCHANGED) {
+        ic_nbctl_validate_sequence_numbers(next_cfg, idl);
+    }
+
     ovsdb_idl_txn_destroy(txn);
     ovsdb_idl_destroy(idl);
 
@@ -924,7 +1006,7 @@  ic_nbctl_exit(int status)
 
 static const struct ctl_command_syntax ic_nbctl_commands[] = {
     { "init", 0, 0, "", NULL, ic_nbctl_init, NULL, "", RW },
-
+    { "sync", 0, 0, "", ic_nbctl_pre_sync, NULL, NULL, "", RO },
     /* transit switch commands. */
     { "ts-add", 1, 1, "SWITCH", NULL, ic_nbctl_ts_add, NULL, "--may-exist", RW },
     { "ts-del", 1, 1, "SWITCH", NULL, ic_nbctl_ts_del, NULL, "--if-exists", RW },