fre. 22. nov. 2019, 19:10 skrev Numan Siddique <num...@ovn.org>:
> 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.
>
Thank you for the review, Numan.
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 don't actually expect the ``ctx->ovnsb_idl`` to ever be NULL. It's just a
bone marrow reaction to always check before handing off something that can
be dereferenced.
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".
>
I see what you mean, and having calls on the idl outside the main loop may
quickly get us into trouble I guess.
The problem I'm left with is that the status command function would need
access to both the ``paused`` and ``had_lock`` states.
What would you think about putting those in a new struct that we pass to
the pause, resume and status command functions?
--
Frode Nordahl
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
