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(&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[])
> > /*
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(&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_b
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
[ovs-dev] [PATCH ovn v2 2/2] northd: Improve handling of pause and resume
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
---
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(&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_l
