| Message ID | 20191118154416.CE3CD21549@silver.osuosl.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn,v3] northd: Add `status` management command | expand |
Hello Numan, Have you had a chance to take a look at my updated proposal? Based on your feedback of `is-active` could be confusing since we have `is-paused` which is used for something else, the best option I could think of was a generic `status` command. I have made the proposal in such a way that we can add more to the status command later if we want to. I guess we could make the status pick up the paused state too, but that would mean moving the paused state into the northd_context struct and passing that to the status handler or something similar which would be a bit more involved change. What do you think? https://patchwork.ozlabs.org/patch/1196828/ -- Frode Nordahl On Mon, Nov 18, 2019 at 4:44 PM Frode Nordahl <frode.nordahl@canonical.com> wrote: > > Allow the operator to query whether a ovn-northd process is > currently active for the standalone and clustered DB use case. > > At present this information is only available in the log. > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > --- > northd/ovn-northd.8.xml | 9 ++++++++- > northd/ovn-northd.c | 20 +++++++++++++++++++- > tests/ovn-northd.at | 9 +++++++++ > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 78b1e84ad..d7833cda7 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -87,13 +87,20 @@ > <dd> > Returns "true" if ovn-northd is currently paused, "false" otherwise. > </dd> > + > + <dt><code>status</code></dt> > + <dd> > + Prints this server's status. Status will be "active" if ovn-northd has > + acquired OVSDB lock on NB DB, "standby" otherwise. > + </dd> > </dl> > </p> > > <h1>Active-Standby for High Availability</h1> > <p> > You may run <code>ovn-northd</code> more than once in an OVN deployment. > - OVN will automatically ensure that only one of them is active at a time. > + When connected to a standalone or clustered DB setup, OVN will > + automatically ensure that only one of them is active at a time. > If multiple instances of <code>ovn-northd</code> are running and the > active <code>ovn-northd</code> fails, one of the hot standby instances > of <code>ovn-northd</code> will automatically take over. > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 41e97f841..83ad6d518 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_status; > > struct northd_context { > struct ovsdb_idl *ovnnb_idl; > @@ -10838,6 +10839,7 @@ main(int argc, char *argv[]) > int retval; > bool exiting; > bool paused; > + bool had_lock; > > fatal_ignore_sigpipe(); > ovs_cmdl_proctitle_init(argc, argv); > @@ -10863,6 +10865,7 @@ 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("status", "", 0, 0, ovn_northd_status, &had_lock); > > daemonize_complete(); > > @@ -11068,11 +11071,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 = { > @@ -11180,3 +11183,18 @@ ovn_northd_is_paused(struct unixctl_conn *conn, int argc OVS_UNUSED, > unixctl_command_reply(conn, "false"); > } > } > + > +static void > +ovn_northd_status(struct unixctl_conn *conn, int argc OVS_UNUSED, > + const char *argv[] OVS_UNUSED, void *had_lock_) > +{ > + bool *had_lock = had_lock_; > + /* > + * Use a labelled formatted output so we can add more to the status command > + * later without breaking any consuming scripts > + */ > + struct ds s = DS_EMPTY_INITIALIZER; > + ds_put_format(&s, "Status: %s\n", *had_lock ? "active" : "standby"); > + unixctl_command_reply(conn, ds_cstr(&s)); > + ds_destroy(&s); > +} > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index da566f900..17e60b051 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -899,6 +899,15 @@ OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > AT_CLEANUP > > +AT_SETUP([ovn -- ovn-northd status]) > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > +ovn_start > + > +AT_CHECK([as northd ovs-appctl -t ovn-northd status], [0], [Status: active > +]) > + > +AT_CLEANUP > + > AT_SETUP([ovn -- ovn-northd pause and resume]) > AT_SKIP_IF([test $HAVE_PYTHON = no]) > ovn_start > -- > 2.24.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Nov 20, 2019 at 3:10 PM Frode Nordahl <frode.nordahl@canonical.com> wrote: > > Hello Numan, > > Have you had a chance to take a look at my updated proposal? > > Based on your feedback of `is-active` could be confusing since we have > `is-paused` which is used for something else, the best option I could > think of was a generic `status` command. I have made the proposal in > such a way that we can add more to the status command later if we want > to. > Thanks for the patch. I applied this to master with one small change **** diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 17e60b051..c73fd9003 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -903,7 +903,7 @@ AT_SETUP([ovn -- ovn-northd status]) AT_SKIP_IF([test $HAVE_PYTHON = no]) ovn_start -AT_CHECK([as northd ovs-appctl -t ovn-northd status], [0], [Status: active +AT_CHECK([as northd ovn-appctl -t ovn-northd status], [0], [Status: active ]) AT_CLEANUP **** > I guess we could make the status pick up the paused state too, but > that would mean moving the paused state into the northd_context struct > and passing that to the status handler or something similar which > would be a bit more involved change. > > What do you think? > I thought about that. but applied the patch as we can address this ambiguity in a separate patch, May be status can say - "paused", "active" and "standby" "paused" - when it is passed "active" - when it has lock and is not paused "standby" - When it has no lock. What do you think about this ? Thanks Numan > https://patchwork.ozlabs.org/patch/1196828/ > > -- > Frode Nordahl > > > On Mon, Nov 18, 2019 at 4:44 PM Frode Nordahl > <frode.nordahl@canonical.com> wrote: > > > > Allow the operator to query whether a ovn-northd process is > > currently active for the standalone and clustered DB use case. > > > > At present this information is only available in the log. > > > > Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> > > --- > > northd/ovn-northd.8.xml | 9 ++++++++- > > northd/ovn-northd.c | 20 +++++++++++++++++++- > > tests/ovn-northd.at | 9 +++++++++ > > 3 files changed, 36 insertions(+), 2 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 78b1e84ad..d7833cda7 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -87,13 +87,20 @@ > > <dd> > > Returns "true" if ovn-northd is currently paused, "false" otherwise. > > </dd> > > + > > + <dt><code>status</code></dt> > > + <dd> > > + Prints this server's status. Status will be "active" if ovn-northd has > > + acquired OVSDB lock on NB DB, "standby" otherwise. > > + </dd> > > </dl> > > </p> > > > > <h1>Active-Standby for High Availability</h1> > > <p> > > You may run <code>ovn-northd</code> more than once in an OVN deployment. > > - OVN will automatically ensure that only one of them is active at a time. > > + When connected to a standalone or clustered DB setup, OVN will > > + automatically ensure that only one of them is active at a time. > > If multiple instances of <code>ovn-northd</code> are running and the > > active <code>ovn-northd</code> fails, one of the hot standby instances > > of <code>ovn-northd</code> will automatically take over. > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 41e97f841..83ad6d518 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_status; > > > > struct northd_context { > > struct ovsdb_idl *ovnnb_idl; > > @@ -10838,6 +10839,7 @@ main(int argc, char *argv[]) > > int retval; > > bool exiting; > > bool paused; > > + bool had_lock; > > > > fatal_ignore_sigpipe(); > > ovs_cmdl_proctitle_init(argc, argv); > > @@ -10863,6 +10865,7 @@ 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("status", "", 0, 0, ovn_northd_status, &had_lock); > > > > daemonize_complete(); > > > > @@ -11068,11 +11071,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 = { > > @@ -11180,3 +11183,18 @@ ovn_northd_is_paused(struct unixctl_conn *conn, int argc OVS_UNUSED, > > unixctl_command_reply(conn, "false"); > > } > > } > > + > > +static void > > +ovn_northd_status(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[] OVS_UNUSED, void *had_lock_) > > +{ > > + bool *had_lock = had_lock_; > > + /* > > + * Use a labelled formatted output so we can add more to the status command > > + * later without breaking any consuming scripts > > + */ > > + struct ds s = DS_EMPTY_INITIALIZER; > > + ds_put_format(&s, "Status: %s\n", *had_lock ? "active" : "standby"); > > + unixctl_command_reply(conn, ds_cstr(&s)); > > + ds_destroy(&s); > > +} > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index da566f900..17e60b051 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -899,6 +899,15 @@ OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > > > AT_CLEANUP > > > > +AT_SETUP([ovn -- ovn-northd status]) > > +AT_SKIP_IF([test $HAVE_PYTHON = no]) > > +ovn_start > > + > > +AT_CHECK([as northd ovs-appctl -t ovn-northd status], [0], [Status: active > > +]) > > + > > +AT_CLEANUP > > + > > AT_SETUP([ovn -- ovn-northd pause and resume]) > > AT_SKIP_IF([test $HAVE_PYTHON = no]) > > ovn_start > > -- > > 2.24.0 > > > > _______________________________________________ > > 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 >
On Wed, Nov 20, 2019 at 2:20 PM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Nov 20, 2019 at 3:10 PM Frode Nordahl > <frode.nordahl@canonical.com> wrote: > > > > Hello Numan, > > > > Have you had a chance to take a look at my updated proposal? > > > > Based on your feedback of `is-active` could be confusing since we have > > `is-paused` which is used for something else, the best option I could > > think of was a generic `status` command. I have made the proposal in > > such a way that we can add more to the status command later if we want > > to. > > > > Thanks for the patch. I applied this to master with one small change Excellent, thank you Numan. > > I guess we could make the status pick up the paused state too, but > > that would mean moving the paused state into the northd_context struct > > and passing that to the status handler or something similar which > > would be a bit more involved change. > > > > What do you think? > > > > I thought about that. but applied the patch as we can address this > ambiguity in a > separate patch, > > May be status can say - "paused", "active" and "standby" > "paused" - when it is passed > "active" - when it has lock and is not paused > "standby" - When it has no lock. > > What do you think about this ? Yes, I think that is a good idea. I'll take a pass at it in a separate patch. There is a remote corner case here as users in effect can run two ovn-northd processes, pause one while it still has the lock and this status will not be visible. But, as far as I can tell from the documentation this is not the intended use case for the pause command, so it might be OK. Alternatively we can either make that scenario visible with a "paused(active)" and "paused(standby)" status, or change the behaviour of the pause command to also drop the lock.
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 78b1e84ad..d7833cda7 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -87,13 +87,20 @@ <dd> Returns "true" if ovn-northd is currently paused, "false" otherwise. </dd> + + <dt><code>status</code></dt> + <dd> + Prints this server's status. Status will be "active" if ovn-northd has + acquired OVSDB lock on NB DB, "standby" otherwise. + </dd> </dl> </p> <h1>Active-Standby for High Availability</h1> <p> You may run <code>ovn-northd</code> more than once in an OVN deployment. - OVN will automatically ensure that only one of them is active at a time. + When connected to a standalone or clustered DB setup, OVN will + automatically ensure that only one of them is active at a time. If multiple instances of <code>ovn-northd</code> are running and the active <code>ovn-northd</code> fails, one of the hot standby instances of <code>ovn-northd</code> will automatically take over. diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 41e97f841..83ad6d518 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_status; struct northd_context { struct ovsdb_idl *ovnnb_idl; @@ -10838,6 +10839,7 @@ main(int argc, char *argv[]) int retval; bool exiting; bool paused; + bool had_lock; fatal_ignore_sigpipe(); ovs_cmdl_proctitle_init(argc, argv); @@ -10863,6 +10865,7 @@ 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("status", "", 0, 0, ovn_northd_status, &had_lock); daemonize_complete(); @@ -11068,11 +11071,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 = { @@ -11180,3 +11183,18 @@ ovn_northd_is_paused(struct unixctl_conn *conn, int argc OVS_UNUSED, unixctl_command_reply(conn, "false"); } } + +static void +ovn_northd_status(struct unixctl_conn *conn, int argc OVS_UNUSED, + const char *argv[] OVS_UNUSED, void *had_lock_) +{ + bool *had_lock = had_lock_; + /* + * Use a labelled formatted output so we can add more to the status command + * later without breaking any consuming scripts + */ + struct ds s = DS_EMPTY_INITIALIZER; + ds_put_format(&s, "Status: %s\n", *had_lock ? "active" : "standby"); + unixctl_command_reply(conn, ds_cstr(&s)); + ds_destroy(&s); +} diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index da566f900..17e60b051 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -899,6 +899,15 @@ OVS_APP_EXIT_AND_WAIT([ovn-northd]) AT_CLEANUP +AT_SETUP([ovn -- ovn-northd status]) +AT_SKIP_IF([test $HAVE_PYTHON = no]) +ovn_start + +AT_CHECK([as northd ovs-appctl -t ovn-northd status], [0], [Status: active +]) + +AT_CLEANUP + AT_SETUP([ovn -- ovn-northd pause and resume]) AT_SKIP_IF([test $HAVE_PYTHON = no]) ovn_start
Allow the operator to query whether a ovn-northd process is currently active for the standalone and clustered DB use case. At present this information is only available in the log. Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com> --- northd/ovn-northd.8.xml | 9 ++++++++- northd/ovn-northd.c | 20 +++++++++++++++++++- tests/ovn-northd.at | 9 +++++++++ 3 files changed, 36 insertions(+), 2 deletions(-)