On Mon, Nov 18, 2019 at 8:11 AM Numan Siddique <[email protected]> wrote: > > On Sat, Nov 16, 2019 at 2:46 AM Frode Nordahl > <[email protected]> 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 <[email protected]> > > 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. ... -- 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
