[ovs-dev,ovn,v2] northd: Add `is-active` management command
diff mbox series

Message ID 20191115211603.86BF72052A@silver.osuosl.org
State Superseded
Headers show
Series
  • [ovs-dev,ovn,v2] northd: Add `is-active` management command
Related show

Commit Message

Frode Nordahl Nov. 15, 2019, 7:51 p.m. UTC
When ovn-northd is connected to clustered OVN DB servers a OVSDB
locking feature is used to ensure only one ovn-northd process is
active at a time.

This patch adds a `is-active` management command that allows the
operator to query whether a ovn-northd process is currently active
or not.

At present this information is only available in the log.

Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
---
 northd/ovn-northd.8.xml | 19 +++++++++++++++++++
 northd/ovn-northd.c     | 18 +++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

Comments

Numan Siddique Nov. 18, 2019, 7:11 a.m. UTC | #1
On Sat, Nov 16, 2019 at 2:46 AM Frode Nordahl
<frode.nordahl@canonical.com> wrote:
>
> When ovn-northd is connected to clustered OVN DB servers a OVSDB
> locking feature is used to ensure only one ovn-northd process is
> active at a time.
>
> This patch adds a `is-active` management command that allows the
> operator to query whether a ovn-northd process is currently active
> or not.
>
> At present this information is only available in the log.
>
> Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>

Thanks for the patch. This command would be useful.
HA for ovn-northd (active/standby) is supported using the lock
mechanism and it doesn't
matter if ovn-northd connects to the clustered db or standalone. This
patch assumes
that locking feature is used only for clustered deployment which is wrong.

Also we have commands - "is-paused", "pause" and "resume". "is-active"
command could confuse
the user.

So can you please update the documentation properly ?

Thanks
Numan


> ---
>  northd/ovn-northd.8.xml | 19 +++++++++++++++++++
>  northd/ovn-northd.c     | 18 +++++++++++++++++-
>  2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 344cc0dbf..e712000f3 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -87,6 +87,12 @@
>        <dd>
>          Returns "true" if ovn-northd is currently paused, "false" otherwise.
>        </dd>
> +
> +      <dt><code>is-active</code></dt>
> +      <dd>
> +        When ovn-northd is connected to clustered OVN DB servers, this returns
> +        "true" if ovn-northd is currently active, "false" otherwise.

You could probably say  something like -
"Returns true if ovn-northd has acquired OVSDB lock and is currently
active, false otherwise.

> +      </dd>
>        </dl>
>      </p>
>
> @@ -130,6 +136,19 @@
>        DB changes.
>      </p>
>
> +    <h2> Active-Standby with clustered OVN DB servers</h2>
> +    <p>
> +      When <code>ovn-northd</code> is connected to clustered OVN DB servers a
> +      OVSDB locking feature will be used to ensure only one
> +      <code>ovn-northd</code> process is active at a time.
> +    </p>
> +
> +    <p>
> +      The <code>ovn-northd</code> daemon will write an entry in its log when
> +      it becomes active.  You may query the active status at any time with
> +      the <code>is-active</code> management command.
> +    </p>

We probably don't need this section as the documentation here [1]
already mentions about it.

[1] - https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.8.xml#L95

Thanks
Numan


> +
>      <h1>Logical Flow Table Structure</h1>
>
>      <p>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 6742bc002..31a744f4d 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -55,6 +55,7 @@ static unixctl_cb_func ovn_northd_exit;
>  static unixctl_cb_func ovn_northd_pause;
>  static unixctl_cb_func ovn_northd_resume;
>  static unixctl_cb_func ovn_northd_is_paused;
> +static unixctl_cb_func ovn_northd_is_active;
>
>  struct northd_context {
>      struct ovsdb_idl *ovnnb_idl;
> @@ -10425,6 +10426,7 @@ main(int argc, char *argv[])
>      int retval;
>      bool exiting;
>      bool paused;
> +    bool had_lock;
>
>      fatal_ignore_sigpipe();
>      ovs_cmdl_proctitle_init(argc, argv);
> @@ -10450,6 +10452,8 @@ main(int argc, char *argv[])
>      unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, &paused);
>      unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused,
>                               &paused);
> +    unixctl_command_register("is-active", "", 0, 0, ovn_northd_is_active,
> +                             &had_lock);
>
>      daemonize_complete();
>
> @@ -10636,11 +10640,11 @@ main(int argc, char *argv[])
>       * acquiring a lock called "ovn_northd" on the southbound database
>       * and then only performing DB transactions if the lock is held. */
>      ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_northd");
> -    bool had_lock = false;
>
>      /* Main loop. */
>      exiting = false;
>      paused = false;
> +    had_lock = false;
>      while (!exiting) {
>          if (!paused) {
>              struct northd_context ctx = {
> @@ -10748,3 +10752,15 @@ ovn_northd_is_paused(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          unixctl_command_reply(conn, "false");
>      }
>  }
> +
> +static void
> +ovn_northd_is_active(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +             const char *argv[] OVS_UNUSED, void *had_lock_)
> +{
> +    bool *had_lock = had_lock_;
> +    if (*had_lock) {
> +        unixctl_command_reply(conn, "true");
> +    } else {
> +        unixctl_command_reply(conn, "false");
> +    }
> +}
> --
> 2.20.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Frode Nordahl Nov. 18, 2019, 7:51 a.m. UTC | #2
On Mon, Nov 18, 2019 at 8:11 AM Numan Siddique <numans@ovn.org> wrote:
>
> On Sat, Nov 16, 2019 at 2:46 AM Frode Nordahl
> <frode.nordahl@canonical.com> wrote:
> >
> > When ovn-northd is connected to clustered OVN DB servers a OVSDB
> > locking feature is used to ensure only one ovn-northd process is
> > active at a time.
> >
> > This patch adds a `is-active` management command that allows the
> > operator to query whether a ovn-northd process is currently active
> > or not.
> >
> > At present this information is only available in the log.
> >
> > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
>
> Thanks for the patch. This command would be useful.
> HA for ovn-northd (active/standby) is supported using the lock
> mechanism and it doesn't
> matter if ovn-northd connects to the clustered db or standalone. This
> patch assumes
> that locking feature is used only for clustered deployment which is wrong.

Thank you for the review Numan, you are right, the lock will be used
for both scenarios.

> Also we have commands - "is-paused", "pause" and "resume". "is-active"
> command could confuse
> the user.

Yes, I thought a bit about that and it will indeed be a bit confusing
as the commands are used for two different methods of northd HA.

How would you feel about naming the command "has-lock" instead, which
would accurately describe what the management command actually tests
for?

> So can you please update the documentation properly ?

Will do.

> Thanks
> Numan
>
>
> > ---
> >  northd/ovn-northd.8.xml | 19 +++++++++++++++++++
> >  northd/ovn-northd.c     | 18 +++++++++++++++++-
> >  2 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index 344cc0dbf..e712000f3 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -87,6 +87,12 @@
> >        <dd>
> >          Returns "true" if ovn-northd is currently paused, "false" otherwise.
> >        </dd>
> > +
> > +      <dt><code>is-active</code></dt>
> > +      <dd>
> > +        When ovn-northd is connected to clustered OVN DB servers, this returns
> > +        "true" if ovn-northd is currently active, "false" otherwise.
>
> You could probably say  something like -
> "Returns true if ovn-northd has acquired OVSDB lock and is currently
> active, false otherwise.

Yes, that makes sense.

> > +      </dd>
> >        </dl>
> >      </p>
> >
> > @@ -130,6 +136,19 @@
> >        DB changes.
> >      </p>
> >
> > +    <h2> Active-Standby with clustered OVN DB servers</h2>
> > +    <p>
> > +      When <code>ovn-northd</code> is connected to clustered OVN DB servers a
> > +      OVSDB locking feature will be used to ensure only one
> > +      <code>ovn-northd</code> process is active at a time.
> > +    </p>
> > +
> > +    <p>
> > +      The <code>ovn-northd</code> daemon will write an entry in its log when
> > +      it becomes active.  You may query the active status at any time with
> > +      the <code>is-active</code> management command.
> > +    </p>
>
> We probably don't need this section as the documentation here [1]
> already mentions about it.
>
> [1] - https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.8.xml#L95

When I read the existing section I get the impression that ovn-northd
only supports active/passive with replicated OVSDB server and manual
pause of non-active northd daemons.  So the intention of the change
was to make that a bit more clear.

What do you think about a change like this in the first paragraph:
...
When connected to clustered DB servers, OVN will automatically ensure that only
one of them is active at a time.
...
Numan Siddique Nov. 18, 2019, 10:39 a.m. UTC | #3
On Mon, Nov 18, 2019 at 1:21 PM Frode Nordahl
<frode.nordahl@canonical.com> wrote:
>
> On Mon, Nov 18, 2019 at 8:11 AM Numan Siddique <numans@ovn.org> wrote:
> >
> > On Sat, Nov 16, 2019 at 2:46 AM Frode Nordahl
> > <frode.nordahl@canonical.com> wrote:
> > >
> > > When ovn-northd is connected to clustered OVN DB servers a OVSDB
> > > locking feature is used to ensure only one ovn-northd process is
> > > active at a time.
> > >
> > > This patch adds a `is-active` management command that allows the
> > > operator to query whether a ovn-northd process is currently active
> > > or not.
> > >
> > > At present this information is only available in the log.
> > >
> > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
> >
> > Thanks for the patch. This command would be useful.
> > HA for ovn-northd (active/standby) is supported using the lock
> > mechanism and it doesn't
> > matter if ovn-northd connects to the clustered db or standalone. This
> > patch assumes
> > that locking feature is used only for clustered deployment which is wrong.
>
> Thank you for the review Numan, you are right, the lock will be used
> for both scenarios.
>
> > Also we have commands - "is-paused", "pause" and "resume". "is-active"
> > command could confuse
> > the user.
>
> Yes, I thought a bit about that and it will indeed be a bit confusing
> as the commands are used for two different methods of northd HA.
>
> How would you feel about naming the command "has-lock" instead, which
> would accurately describe what the management command actually tests
> for?

Well lock mechanism is something internal to ovn-northd and ovsdb-server. So not
sure that is the right command name.

>
> > So can you please update the documentation properly ?
>
> Will do.
>
> > Thanks
> > Numan
> >
> >
> > > ---
> > >  northd/ovn-northd.8.xml | 19 +++++++++++++++++++
> > >  northd/ovn-northd.c     | 18 +++++++++++++++++-
> > >  2 files changed, 36 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > > index 344cc0dbf..e712000f3 100644
> > > --- a/northd/ovn-northd.8.xml
> > > +++ b/northd/ovn-northd.8.xml
> > > @@ -87,6 +87,12 @@
> > >        <dd>
> > >          Returns "true" if ovn-northd is currently paused, "false" otherwise.
> > >        </dd>
> > > +
> > > +      <dt><code>is-active</code></dt>
> > > +      <dd>
> > > +        When ovn-northd is connected to clustered OVN DB servers, this returns
> > > +        "true" if ovn-northd is currently active, "false" otherwise.
> >
> > You could probably say  something like -
> > "Returns true if ovn-northd has acquired OVSDB lock and is currently
> > active, false otherwise.
>
> Yes, that makes sense.
>
> > > +      </dd>
> > >        </dl>
> > >      </p>
> > >
> > > @@ -130,6 +136,19 @@
> > >        DB changes.
> > >      </p>
> > >
> > > +    <h2> Active-Standby with clustered OVN DB servers</h2>
> > > +    <p>
> > > +      When <code>ovn-northd</code> is connected to clustered OVN DB servers a
> > > +      OVSDB locking feature will be used to ensure only one
> > > +      <code>ovn-northd</code> process is active at a time.
> > > +    </p>
> > > +
> > > +    <p>
> > > +      The <code>ovn-northd</code> daemon will write an entry in its log when
> > > +      it becomes active.  You may query the active status at any time with
> > > +      the <code>is-active</code> management command.
> > > +    </p>
> >
> > We probably don't need this section as the documentation here [1]
> > already mentions about it.
> >
> > [1] - https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.8.xml#L95
>
> When I read the existing section I get the impression that ovn-northd
> only supports active/passive with replicated OVSDB server and manual
> pause of non-active northd daemons.  So the intention of the change
> was to make that a bit more clear.
>
> What do you think about a change like this in the first paragraph:
> ...
> When connected to clustered DB servers, OVN will automatically ensure that only
> one of them is active at a time.

Sounds fine to me.

The reason why the same lock mechanism works for both standalone and
clustered setup is
because all the ovn-northd's in the cluster setup will connect to the
leader of the cluster
and hence only one ovn-northd will get the lock and that becomes active.

Thanks
Numan

> ...
>
> --
> Frode Nordahl
>
> > Thanks
> > Numan
> >
> >
> > > +
> > >      <h1>Logical Flow Table Structure</h1>
> > >
> > >      <p>
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 6742bc002..31a744f4d 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -55,6 +55,7 @@ static unixctl_cb_func ovn_northd_exit;
> > >  static unixctl_cb_func ovn_northd_pause;
> > >  static unixctl_cb_func ovn_northd_resume;
> > >  static unixctl_cb_func ovn_northd_is_paused;
> > > +static unixctl_cb_func ovn_northd_is_active;
> > >
> > >  struct northd_context {
> > >      struct ovsdb_idl *ovnnb_idl;
> > > @@ -10425,6 +10426,7 @@ main(int argc, char *argv[])
> > >      int retval;
> > >      bool exiting;
> > >      bool paused;
> > > +    bool had_lock;
> > >
> > >      fatal_ignore_sigpipe();
> > >      ovs_cmdl_proctitle_init(argc, argv);
> > > @@ -10450,6 +10452,8 @@ main(int argc, char *argv[])
> > >      unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, &paused);
> > >      unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused,
> > >                               &paused);
> > > +    unixctl_command_register("is-active", "", 0, 0, ovn_northd_is_active,
> > > +                             &had_lock);
> > >
> > >      daemonize_complete();
> > >
> > > @@ -10636,11 +10640,11 @@ main(int argc, char *argv[])
> > >       * acquiring a lock called "ovn_northd" on the southbound database
> > >       * and then only performing DB transactions if the lock is held. */
> > >      ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_northd");
> > > -    bool had_lock = false;
> > >
> > >      /* Main loop. */
> > >      exiting = false;
> > >      paused = false;
> > > +    had_lock = false;
> > >      while (!exiting) {
> > >          if (!paused) {
> > >              struct northd_context ctx = {
> > > @@ -10748,3 +10752,15 @@ ovn_northd_is_paused(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > >          unixctl_command_reply(conn, "false");
> > >      }
> > >  }
> > > +
> > > +static void
> > > +ovn_northd_is_active(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > > +             const char *argv[] OVS_UNUSED, void *had_lock_)
> > > +{
> > > +    bool *had_lock = had_lock_;
> > > +    if (*had_lock) {
> > > +        unixctl_command_reply(conn, "true");
> > > +    } else {
> > > +        unixctl_command_reply(conn, "false");
> > > +    }
> > > +}
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch
diff mbox series

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 344cc0dbf..e712000f3 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -87,6 +87,12 @@ 
       <dd>
         Returns "true" if ovn-northd is currently paused, "false" otherwise.
       </dd>
+
+      <dt><code>is-active</code></dt>
+      <dd>
+        When ovn-northd is connected to clustered OVN DB servers, this returns
+        "true" if ovn-northd is currently active, "false" otherwise.
+      </dd>
       </dl>
     </p>
 
@@ -130,6 +136,19 @@ 
       DB changes.
     </p>
 
+    <h2> Active-Standby with clustered OVN DB servers</h2>
+    <p>
+      When <code>ovn-northd</code> is connected to clustered OVN DB servers a
+      OVSDB locking feature will be used to ensure only one
+      <code>ovn-northd</code> process is active at a time.
+    </p>
+
+    <p>
+      The <code>ovn-northd</code> daemon will write an entry in its log when
+      it becomes active.  You may query the active status at any time with
+      the <code>is-active</code> management command.
+    </p>
+
     <h1>Logical Flow Table Structure</h1>
 
     <p>
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 6742bc002..31a744f4d 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -55,6 +55,7 @@  static unixctl_cb_func ovn_northd_exit;
 static unixctl_cb_func ovn_northd_pause;
 static unixctl_cb_func ovn_northd_resume;
 static unixctl_cb_func ovn_northd_is_paused;
+static unixctl_cb_func ovn_northd_is_active;
 
 struct northd_context {
     struct ovsdb_idl *ovnnb_idl;
@@ -10425,6 +10426,7 @@  main(int argc, char *argv[])
     int retval;
     bool exiting;
     bool paused;
+    bool had_lock;
 
     fatal_ignore_sigpipe();
     ovs_cmdl_proctitle_init(argc, argv);
@@ -10450,6 +10452,8 @@  main(int argc, char *argv[])
     unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, &paused);
     unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused,
                              &paused);
+    unixctl_command_register("is-active", "", 0, 0, ovn_northd_is_active,
+                             &had_lock);
 
     daemonize_complete();
 
@@ -10636,11 +10640,11 @@  main(int argc, char *argv[])
      * acquiring a lock called "ovn_northd" on the southbound database
      * and then only performing DB transactions if the lock is held. */
     ovsdb_idl_set_lock(ovnsb_idl_loop.idl, "ovn_northd");
-    bool had_lock = false;
 
     /* Main loop. */
     exiting = false;
     paused = false;
+    had_lock = false;
     while (!exiting) {
         if (!paused) {
             struct northd_context ctx = {
@@ -10748,3 +10752,15 @@  ovn_northd_is_paused(struct unixctl_conn *conn, int argc OVS_UNUSED,
         unixctl_command_reply(conn, "false");
     }
 }
+
+static void
+ovn_northd_is_active(struct unixctl_conn *conn, int argc OVS_UNUSED,
+             const char *argv[] OVS_UNUSED, void *had_lock_)
+{
+    bool *had_lock = had_lock_;
+    if (*had_lock) {
+        unixctl_command_reply(conn, "true");
+    } else {
+        unixctl_command_reply(conn, "false");
+    }
+}