On Fri, Nov 22, 2019 at 8:22 PM Frode Nordahl
<frode.nord...@canonical.com> wrote:
>
> Move paused state to ``struct northd_context`` and pass the
> context to paused and status command handlers.
>
> On pause release the OVSDB lock on SB DB.
>
> Re-instante the lock on resume.
>
> Status command will now provide accurate information for 'paused',
> 'active' and 'standby' states.
>
> Merge separate status command test into the pause and resume tests.
>
> Signed-off-by: Frode Nordahl <frode.nord...@canonical.com>
Hi Frode,
Thanks for the patch.
Using ctx in the ctl functions doesn't seem right to me.
When the user runs "ovn-appctl -t ovn-northd pause/resume" and if
ctx->ovnsb_idl is NULL,
then ovn_northd_pause()/ovn_northd_resume() will just ignore these commands.
I think you can call ovsdb_idl_set_lock with the appropriate params in
the main while loop itself
depending on the value of "paused".
Thanks
Numan
> ---
> northd/ovn-northd.8.xml | 9 +++--
> northd/ovn-northd.c | 87 +++++++++++++++++++++++++++--------------
> tests/ovn-northd.at | 24 +++++++-----
> 3 files changed, 79 insertions(+), 41 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 9734e79e6..c6d5d96b9 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -74,13 +74,15 @@
> <dt><code>pause</code></dt>
> <dd>
> Pauses the ovn-northd operation from processing any Northbound and
> - Southbound database changes.
> + Southbound database changes. This will also instruct ovn-northd to
> + drop any lock on SB DB.
> </dd>
>
> <dt><code>resume</code></dt>
> <dd>
> Resumes the ovn-northd operation to process Northbound and
> - Southbound database contents and generate logical flows.
> + Southbound database contents and generate logical flows. This will
> + also instruct ovn-northd to aspire for the lock on SB DB.
> </dd>
>
> <dt><code>is-paused</code></dt>
> @@ -91,7 +93,8 @@
> <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.
> + acquired OVSDB lock on SB DB, "standby" if it has not or "paused" if
> + this instance is paused.
> </dd>
> </dl>
> </p>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index a943e1037..e19515d14 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -65,6 +65,7 @@ struct northd_context {
> struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
> struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
> struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> + bool paused;
> };
>
> /* An IPv4 or IPv6 address */
> @@ -10836,6 +10837,8 @@ add_column_noalert(struct ovsdb_idl *idl,
> ovsdb_idl_omit_alert(idl, column);
> }
>
> +#define OVN_NORTHD_SB_DB_LOCK_NAME "ovn_northd"
> +
> int
> main(int argc, char *argv[])
> {
> @@ -10843,8 +10846,10 @@ main(int argc, char *argv[])
> struct unixctl_server *unixctl;
> int retval;
> bool exiting;
> - bool paused;
> bool had_lock;
> + struct northd_context ctx;
> +
> + memset(&ctx, 0, sizeof(ctx));
>
> fatal_ignore_sigpipe();
> ovs_cmdl_proctitle_init(argc, argv);
> @@ -10866,11 +10871,11 @@ main(int argc, char *argv[])
> exit(EXIT_FAILURE);
> }
> unixctl_command_register("exit", "", 0, 0, ovn_northd_exit, &exiting);
> - unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, &paused);
> - unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, &paused);
> + unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, &ctx);
> + unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, &ctx);
> unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused,
> - &paused);
> - unixctl_command_register("status", "", 0, 0, ovn_northd_status,
> &had_lock);
> + &ctx);
> + unixctl_command_register("status", "", 0, 0, ovn_northd_status, &ctx);
>
> daemonize_complete();
>
> @@ -11075,23 +11080,21 @@ main(int argc, char *argv[])
> /* Ensure that only a single ovn-northd is active in the deployment by
> * 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");
> + ovsdb_idl_set_lock(ovnsb_idl_loop.idl, OVN_NORTHD_SB_DB_LOCK_NAME);
>
> /* Main loop. */
> exiting = false;
> - paused = false;
> + ctx.paused = false;
> had_lock = false;
> while (!exiting) {
> - if (!paused) {
> - struct northd_context ctx = {
> - .ovnnb_idl = ovnnb_idl_loop.idl,
> - .ovnnb_txn = ovsdb_idl_loop_run(&ovnnb_idl_loop),
> - .ovnsb_idl = ovnsb_idl_loop.idl,
> - .ovnsb_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop),
> - .sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
> - .sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp,
> - .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
> - };
> + if (!ctx.paused) {
> + ctx.ovnnb_idl = ovnnb_idl_loop.idl;
> + ctx.ovnnb_txn = ovsdb_idl_loop_run(&ovnnb_idl_loop);
> + ctx.ovnsb_idl = ovnsb_idl_loop.idl;
> + ctx.ovnsb_txn = ovsdb_idl_loop_run(&ovnsb_idl_loop);
> + ctx.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name;
> + ctx.sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp;
> + ctx.sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp;
>
> if (!had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> VLOG_INFO("ovn-northd lock acquired. "
> @@ -11125,6 +11128,11 @@ main(int argc, char *argv[])
> ovsdb_idl_run(ovnsb_idl_loop.idl);
> ovsdb_idl_wait(ovnnb_idl_loop.idl);
> ovsdb_idl_wait(ovnsb_idl_loop.idl);
> + /*
> + * the lock is dropped on pause, avoid incorrect message logged
> + * about lock lost when resumed.
> + */
> + had_lock = false;
> }
>
> unixctl_server_run(unixctl);
> @@ -11159,30 +11167,41 @@ ovn_northd_exit(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
>
> static void
> ovn_northd_pause(struct unixctl_conn *conn, int argc OVS_UNUSED,
> - const char *argv[] OVS_UNUSED, void *pause_)
> + const char *argv[] OVS_UNUSED, void *ctx_)
> {
> - bool *pause = pause_;
> - *pause = true;
> + struct northd_context *ctx = ctx_;
> +
> + if (!ctx->paused && ctx->ovnsb_idl != NULL) {
> + /* Drop our aspiration for the lock while paused */
> + ovsdb_idl_set_lock(ctx->ovnsb_idl, NULL);
> + ctx->paused = true;
> + VLOG_INFO("This ovn-northd instance is now paused.");
> + }
>
> unixctl_command_reply(conn, NULL);
> }
>
> static void
> ovn_northd_resume(struct unixctl_conn *conn, int argc OVS_UNUSED,
> - const char *argv[] OVS_UNUSED, void *pause_)
> + const char *argv[] OVS_UNUSED, void *ctx_)
> {
> - bool *pause = pause_;
> - *pause = false;
> + struct northd_context *ctx = ctx_;
> +
> + if (ctx->paused && ctx->ovnsb_idl != NULL) {
> + /* Reinstate our aspiration for lock */
> + ovsdb_idl_set_lock(ctx->ovnsb_idl, OVN_NORTHD_SB_DB_LOCK_NAME);
> + ctx->paused = false;
> + }
>
> unixctl_command_reply(conn, NULL);
> }
>
> static void
> ovn_northd_is_paused(struct unixctl_conn *conn, int argc OVS_UNUSED,
> - const char *argv[] OVS_UNUSED, void *paused_)
> + const char *argv[] OVS_UNUSED, void *ctx_)
> {
> - bool *paused = paused_;
> - if (*paused) {
> + struct northd_context *ctx = ctx_;
> + if (ctx->paused) {
> unixctl_command_reply(conn, "true");
> } else {
> unixctl_command_reply(conn, "false");
> @@ -11191,15 +11210,25 @@ ovn_northd_is_paused(struct unixctl_conn *conn, int
> argc OVS_UNUSED,
>
> static void
> ovn_northd_status(struct unixctl_conn *conn, int argc OVS_UNUSED,
> - const char *argv[] OVS_UNUSED, void *had_lock_)
> + const char *argv[] OVS_UNUSED, void *ctx_)
> {
> - bool *had_lock = had_lock_;
> + struct northd_context *ctx = ctx_;
> + char *status;
> +
> + if (ctx->paused) {
> + status = "paused";
> + } else if (ctx->ovnsb_idl != NULL) {
> + status = ovsdb_idl_has_lock(ctx->ovnsb_idl) ? "active" : "standby";
> + } else {
> + status = "unknown";
> + }
> +
> /*
> * 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");
> + ds_put_format(&s, "Status: %s\n", status);
> unixctl_command_reply(conn, ds_cstr(&s));
> ds_destroy(&s);
> }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 1c94f2f65..d73a22f68 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -899,22 +899,18 @@ 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 ovn-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
>
> AT_CHECK([test xfalse = x`as northd ovn-appctl -t ovn-northd is-paused`])
> +AT_CHECK([as northd ovn-appctl -t ovn-northd status], [0], [Status: active
> +])
> AT_CHECK([test xfalse = x`as northd-backup ovn-appctl -t ovn-northd \
> is-paused`])
> +AT_CHECK([as northd-backup ovn-appctl -t ovn-northd status], [0],
> +[Status: standby
> +])
>
> ovn-nbctl ls-add sw0
>
> @@ -931,7 +927,12 @@ OVS_WAIT_UNTIL([
> as northd ovs-appctl -t ovn-northd pause
> as northd-backup ovs-appctl -t ovn-northd pause
> AT_CHECK([test xtrue = x`as northd ovn-appctl -t ovn-northd is-paused`])
> +AT_CHECK([as northd ovn-appctl -t ovn-northd status], [0], [Status: paused
> +])
> AT_CHECK([test xtrue = x`as northd-backup ovn-appctl -t ovn-northd
> is-paused`])
> +AT_CHECK([as northd-backup ovn-appctl -t ovn-northd status], [0],
> +[Status: paused
> +])
>
> ovn-nbctl ls-add sw0
>
> @@ -944,8 +945,13 @@ OVS_WAIT_UNTIL([
> as northd ovs-appctl -t ovn-northd resume
> as northd-backup ovs-appctl -t ovn-northd resume
> AT_CHECK([test xfalse = x`as northd ovn-appctl -t ovn-northd is-paused`])
> +AT_CHECK([as northd ovn-appctl -t ovn-northd status], [0], [Status: active
> +])
> AT_CHECK([test xfalse = x`as northd-backup ovn-appctl -t ovn-northd \
> is-paused`])
> +AT_CHECK([as northd-backup ovn-appctl -t ovn-northd status], [0],
> +[Status: standby
> +])
>
> OVS_WAIT_UNTIL([
> ovn-sbctl lflow-list sw0
> --
> 2.24.0
>
> .
>
> /
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
