| Message ID | 20191115211603.86BF72052A@silver.osuosl.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,ovn,v2] northd: Add `is-active` management command | expand |
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 >
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. ...
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 >
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"); + } +}
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(-)