Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
fre. 22. nov. 2019, 19:10 skrev Numan Siddique :
> On Fri, Nov 22, 2019 at 8:22 PM Frode Nordahl
> 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
>
> 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 @@
> >pause
> >
> > 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.
> >
> >
> >resume
> >
> > 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.
> >
> >
> >is-paused
> > @@ -91,7 +93,8 @@
> >status
> >
> > 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.
> >
> >
> >
> > 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(, 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,
> );
> > -unixctl_command_register("pause", "", 0, 0, ovn_northd_pause,
> );
> > -unixctl_command_register("resume", "", 0, 0, ovn_northd_resume,
> );
> > +unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
> > +unixctl_command_register("resume", "", 0, 0, ovn_northd_resume,
> );
> > unixctl_command_register("is-paused", "", 0, 0,
> ovn_northd_is_paused,
> > - );
> > -unixctl_command_register("status", "", 0, 0, ovn_northd_status,
> _lock);
> > + );
> > +unixctl_command_register("status", "", 0, 0, ovn_northd_status,
> );
> >
> > daemonize_complete();
> >
> > @@ -11075,23 +11080,21 @@ main(int argc, char *argv[])
> > /* Ensure that only a single ovn-northd is active in
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
On Fri, Nov 22, 2019 at 8:22 PM Frode Nordahl
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
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 @@
>pause
>
> 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.
>
>
>resume
>
> 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.
>
>
>is-paused
> @@ -91,7 +93,8 @@
>status
>
> 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.
>
>
>
> 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(, 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, );
> -unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
> -unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, );
> +unixctl_command_register("pause", "", 0, 0, ovn_northd_pause, );
> +unixctl_command_register("resume", "", 0, 0, ovn_northd_resume, );
> unixctl_command_register("is-paused", "", 0, 0, ovn_northd_is_paused,
> - );
> -unixctl_command_register("status", "", 0, 0, ovn_northd_status,
> _lock);
> + );
> +unixctl_command_register("status", "", 0, 0, ovn_northd_status, );
>
> 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(_idl_loop),
> -.ovnsb_idl = ovnsb_idl_loop.idl,
> -.ovnsb_txn = ovsdb_idl_loop_run(_idl_loop),
> -.sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
> -.sbrec_mcast_group_by_name_dp =
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
Ok, this time I am at a loss of what's wrong with the submission, I would kindly request reviews of the submission as-is while I figure out my grievances with the bot. -- Frode Nordahl On Fri, Nov 22, 2019 at 4:03 PM 0-day Robot wrote: > > Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > git-am: > fatal: sha1 information is lacking or useless (tests/ovn-northd.at). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 northd: Improve handling of pause and resume > The copy of the patch that failed is found in: > > /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch > When you have resolved this problem, run "git am --resolved". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > > Please check this out. If you feel there has been an error, please email > acon...@redhat.com > > Thanks, > 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
Bleep bloop. Greetings Frode Nordahl, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: fatal: sha1 information is lacking or useless (tests/ovn-northd.at). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 northd: Improve handling of pause and resume The copy of the patch that failed is found in: /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/OVN/.git/rebase-apply/patch When you have resolved this problem, run "git am --resolved". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
