On Mon, Nov 18, 2019 at 1:21 PM Frode Nordahl <[email protected]> wrote: > > 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?
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 > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > 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
