[ovs-dev,v1,ovn] ovn-nb/sbctl.c: Set no-leader-only as default for clustered dbs
diff mbox series

Message ID 20191002000528.68566-1-amginwal@gmail.com
State New
Headers show
Series
  • [ovs-dev,v1,ovn] ovn-nb/sbctl.c: Set no-leader-only as default for clustered dbs
Related show

Commit Message

aginwala aginwala Oct. 2, 2019, 12:05 a.m. UTC
From: Aliasgar Ginwala <aginwala@ebay.com>

When using ovn-nb/sbctl running in cluster, one can use local
socket to run different commands. It is very inconvenient to pass
no-leader-only in different tools using ovn-nb/sbctl instead of
allowing one to to connect to any nodes in the cluster including
itself.
e.g common usage ovn-nb/sbctl show.
Hence, this commit handles the same.

Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
---
 utilities/ovn-nbctl.8.xml | 17 +++++++++--------
 utilities/ovn-nbctl.c     |  3 ++-
 utilities/ovn-sbctl.8.in  | 18 +++++++++---------
 utilities/ovn-sbctl.c     |  2 +-
 4 files changed, 21 insertions(+), 19 deletions(-)

Comments

Han Zhou Oct. 2, 2019, 7:25 p.m. UTC | #1
On Tue, Oct 1, 2019 at 5:06 PM <amginwal@gmail.com> wrote:
>
> From: Aliasgar Ginwala <aginwala@ebay.com>
>
> When using ovn-nb/sbctl running in cluster, one can use local
> socket to run different commands. It is very inconvenient to pass
> no-leader-only in different tools using ovn-nb/sbctl instead of
> allowing one to to connect to any nodes in the cluster including
> itself.
> e.g common usage ovn-nb/sbctl show.
> Hence, this commit handles the same.
>
> Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> ---
>  utilities/ovn-nbctl.8.xml | 17 +++++++++--------
>  utilities/ovn-nbctl.c     |  3 ++-
>  utilities/ovn-sbctl.8.in  | 18 +++++++++---------
>  utilities/ovn-sbctl.c     |  2 +-
>  4 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index fd75c0e44..3dd05fa65 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1192,14 +1192,15 @@
>      <dt><code>--leader-only</code></dt>
>      <dt><code>--no-leader-only</code></dt>
>      <dd>
> -      By default, or with <code>--leader-only</code>, when the database
server
> -      is a clustered database, <code>ovn-nbctl</code> will avoid servers
other
> -      than the cluster leader.  This ensures that any data that
> -      <code>ovn-nbctl</code> reads and reports is up-to-date.  With
> -      <code>--no-leader-only</code>, <code>ovn-nbctl</code> will use any
server
> -      in the cluster, which means that for read-only transactions it can
report
> -      and act on stale data (transactions that modify the database are
always
> -      serialized even with <code>--no-leader-only</code>).  Refer to
> +      By default, or with <code>--no-leader-only</code>, when the
database
> +      server is a clustered database, <code>ovn-nbctl</code> may connect
to
> +      any server in the cluster, which means that for read-only
transactions
> +      it can report and act on stale data (transactions that modify the
> +      database are always serialized even with
<code>--no-leader-only</code>).
> +      To avoid reading stale data, one can specify
<code>--leader-only</code>,
> +      so that <code>ovn-nbctl</code> will avoid servers other than the
cluster
> +      leader. For daemon mode, since it is for long running connections,
> +      default is set to <code>--leader-only</code>. Refer to
>        <code>Understanding Cluster Consistency</code> in
<code>ovsdb</code>(7)
>        for more information.
>      </dd>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index a89a9cb4d..3804dd25a 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -81,7 +81,7 @@ static struct ovsdb_idl_txn *the_idl_txn;
>  OVS_NO_RETURN static void nbctl_exit(int status);
>
>  /* --leader-only, --no-leader-only: Only accept the leader in a cluster.
*/
> -static int leader_only = true;
> +static int leader_only = false;
>
>  /* --shuffle-remotes, --no-shuffle-remotes: Shuffle the order of remotes
that
>   * are specified in the connetion method string. */
> @@ -188,6 +188,7 @@ main(int argc, char *argv[])
>                        "(use --help for help)");
>          }
>          daemon_mode = true;
> +        leader_only = true;
>      }
>      /* Initialize IDL. */
>      idl = the_idl = ovsdb_idl_create_unconnected(&nbrec_idl_class, true);
> diff --git a/utilities/ovn-sbctl.8.in b/utilities/ovn-sbctl.8.in
> index 2aaa457e8..f52412812 100644
> --- a/utilities/ovn-sbctl.8.in
> +++ b/utilities/ovn-sbctl.8.in
> @@ -53,16 +53,16 @@ e.g. \fBssl:192.168.10.5:6640\fR, as described in
\fBovsdb\fR(7).
>  .
>  .IP "\fB\-\-leader\-only\fR"
>  .IQ "\fB\-\-no\-leader\-only\fR"
> -By default, or with \fB\-\-leader\-only\fR, when the database server
> -is a clustered database, \fBovn\-sbctl\fR will avoid servers other
> -than the cluster leader.  This ensures that any data that
> -\fBovn\-sbctl\fR reads and reports is up-to-date.  With
> -\fB\-\-no\-leader\-only\fR, \fBovn\-sbctl\fR will use any server in
> -the cluster, which means that for read-only transactions it can report
> +By default, or with \fB\-\-no\-leader\-only\fR, when the database server
> +is a clustered database, \fBovn\-sbctl\fR may connect to any server
> +in the cluster, which means that for read-only transactions it can report
>  and act on stale data (transactions that modify the database are
> -always serialized even with \fB\-\-no\-leader\-only\fR).  Refer to
> -\fBUnderstanding Cluster Consistency\fR in \fBovsdb\fR(7) for more
> -information.
> +always serialized even with \fB\-\-no\-leader\-only\fR). To avoid reading
> +stale data, one can specify \fB\-\-leader\-only\fR, so that
> +\fBovn\-sbctl\fR will avoid servers other than the cluster leader. For
daemon
> +mode, since it is for long running connections, default is set to
> +\fB\-\-leader\-only\fR. Refer to \fBUnderstanding Cluster Consistency\fR
in
> +\fBovsdb\fR(7) for more information.
>  .
>  .IP "\fB\-\-no\-syslog\fR"
>  By default, \fBovn\-sbctl\fR logs its arguments and the details of any
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index 9a9b6f0ec..f1cb8790f 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -82,7 +82,7 @@ static struct ovsdb_idl_txn *the_idl_txn;
>  OVS_NO_RETURN static void sbctl_exit(int status);
>
>  /* --leader-only, --no-leader-only: Only accept the leader in a cluster.
*/
> -static int leader_only = true;
> +static int leader_only = false;
>
>  static void sbctl_cmd_init(void);
>  OVS_NO_RETURN static void usage(void);
> --
> 2.20.1 (Apple Git-117)
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks Aliasgar.
Acked-by: Han Zhou <hzhou8@ebay.com>

I'd leave to Ben and others to take a look and confirm. I support this
change because based on the day-to-day usage/test it seems more convenient
this way. We'd better change this before there are too many users of
clustered mode.
Ben Pfaff Oct. 3, 2019, 1:03 p.m. UTC | #2
On Tue, Oct 01, 2019 at 05:05:28PM -0700, amginwal@gmail.com wrote:
> From: Aliasgar Ginwala <aginwala@ebay.com>
> 
> When using ovn-nb/sbctl running in cluster, one can use local
> socket to run different commands. It is very inconvenient to pass
> no-leader-only in different tools using ovn-nb/sbctl instead of
> allowing one to to connect to any nodes in the cluster including
> itself.
> e.g common usage ovn-nb/sbctl show.
> Hence, this commit handles the same.
> 
> Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>

This change makes more of a difference than its size implies, because it
means that scripts that previously were guaranteed to get up-to-date
data can now get inconsistent results.  It loses read-after-write
consistency, for example.  I'd really prefer to avoid surprising users
with that kind of thing (especially as a change).

If it's common to want that kind of behavior, though, perhaps there
could be a nice way to set it as the default for a session.  In daemon
mode, of course, it's already possible to control it for the daemon's
users.  One option for outside daemon mode might be to introduce an
environment variable.  The environment variable could be specific to
this feature, e.g. OVN_LEADER_ONLY=0 or OVN_LEADER_ONLY=1, or it could
be a general-purpose options variable,
e.g. OVN_OPTIONS=--no-leader-only.  I don't know whether it should be
specific to one of ovn-sbctl and ovn-nbctl or apply to both.

Have you thought about these possibilities?

Thanks,

Ben.
Han Zhou Oct. 3, 2019, 5:19 p.m. UTC | #3
On Thu, Oct 3, 2019 at 9:09 AM Ben Pfaff <blp@ovn.org> wrote:
>
> On Tue, Oct 01, 2019 at 05:05:28PM -0700, amginwal@gmail.com wrote:
> > From: Aliasgar Ginwala <aginwala@ebay.com>
> >
> > When using ovn-nb/sbctl running in cluster, one can use local
> > socket to run different commands. It is very inconvenient to pass
> > no-leader-only in different tools using ovn-nb/sbctl instead of
> > allowing one to to connect to any nodes in the cluster including
> > itself.
> > e.g common usage ovn-nb/sbctl show.
> > Hence, this commit handles the same.
> >
> > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
>
> This change makes more of a difference than its size implies, because it
> means that scripts that previously were guaranteed to get up-to-date
> data can now get inconsistent results.  It loses read-after-write
> consistency, for example.  I'd really prefer to avoid surprising users
> with that kind of thing (especially as a change).
>
> If it's common to want that kind of behavior, though, perhaps there
> could be a nice way to set it as the default for a session.  In daemon
> mode, of course, it's already possible to control it for the daemon's
> users.  One option for outside daemon mode might be to introduce an
> environment variable.  The environment variable could be specific to
> this feature, e.g. OVN_LEADER_ONLY=0 or OVN_LEADER_ONLY=1, or it could
> be a general-purpose options variable,
> e.g. OVN_OPTIONS=--no-leader-only.  I don't know whether it should be
> specific to one of ovn-sbctl and ovn-nbctl or apply to both.
>
> Have you thought about these possibilities?
>
Thanks Ben for the comments. Let's compare the pros and cons of each
solution:

1) keep current implementation - default to leader-only

pros: Consistency is preserved with default parameter

cons:
- Existing scripts with ovn-nb/sbctl using default unix socket will fail.
E.g. when restarting a central node service using: ovn-ctl start_northd.
The command will fail when executing the step ovn-nbctl init, because the
current node is not the leader. There can be other scripts built by user
that is supposed to work with the default option now might fail as well.
All these can be fixed by updating all the scripts that doesn't require
strong consistency with --no-leader-only.
- Inconvenience of manual command execution for regular operations (mostly
not care about occasionally reading staled data).

2) apply this patch - default to no-leader-only, except daemon mode

pros: Existing scripts will not *fail*.

cons: There are *small* chances that inconsistent (stale) data can be read
by the existing tools (except daemon mode, because this patch enforces
leader-only as default for daemon mode). To avoid that, the scripts that
has strong consistency requirement should be updated to add --leader-only.

3) default to leader-only, but let user to change the default behavior by
setting environment variable

pros:
- Consistency is preserved with default parameter, if user doesn't set
environment variable
- User has the freedom to change default behavior by setting environment
variable

cons:
- Existing scripts with ovn-nb/sbctl using default unix socket will still
fail, unless they are updated to set the environment variable, which is
even less straightforward change compared with adding --no-leader-only
directly.
- If user set the environment variable to use --no-leader-only by default
(e.g. in bashrc of central node), then I think it is the same problem that
by default user could get inconsistent data.
- One more place to control the default behavior, which sometimes leads to
more confusion.

I just listed the pros and cons for discussion. I don't have a strong
opinion which way is the best. (The number of pros/cons doesn't necessarily
mean it is better/worse)
Ben Pfaff Oct. 3, 2019, 6:35 p.m. UTC | #4
On Thu, Oct 03, 2019 at 10:19:35AM -0700, Han Zhou wrote:
> On Thu, Oct 3, 2019 at 9:09 AM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Tue, Oct 01, 2019 at 05:05:28PM -0700, amginwal@gmail.com wrote:
> > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > >
> > > When using ovn-nb/sbctl running in cluster, one can use local
> > > socket to run different commands. It is very inconvenient to pass
> > > no-leader-only in different tools using ovn-nb/sbctl instead of
> > > allowing one to to connect to any nodes in the cluster including
> > > itself.
> > > e.g common usage ovn-nb/sbctl show.
> > > Hence, this commit handles the same.
> > >
> > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> >
> > This change makes more of a difference than its size implies, because it
> > means that scripts that previously were guaranteed to get up-to-date
> > data can now get inconsistent results.  It loses read-after-write
> > consistency, for example.  I'd really prefer to avoid surprising users
> > with that kind of thing (especially as a change).
> >
> > If it's common to want that kind of behavior, though, perhaps there
> > could be a nice way to set it as the default for a session.  In daemon
> > mode, of course, it's already possible to control it for the daemon's
> > users.  One option for outside daemon mode might be to introduce an
> > environment variable.  The environment variable could be specific to
> > this feature, e.g. OVN_LEADER_ONLY=0 or OVN_LEADER_ONLY=1, or it could
> > be a general-purpose options variable,
> > e.g. OVN_OPTIONS=--no-leader-only.  I don't know whether it should be
> > specific to one of ovn-sbctl and ovn-nbctl or apply to both.
> >
> > Have you thought about these possibilities?
> >
> Thanks Ben for the comments. Let's compare the pros and cons of each
> solution:

Thank you for listing the pros and cons.  I agree with them.

One factor that is not completely captured by your list is the effect on
transitioning deployments from standalone to clustered.  Currently, this
should not have surprising effects: one would just replace the single
database URL by a comma-separated list and it will continue to work,
just more reliably.  If the default changes to --no-leader-only, then
each use of ovn-nbctl and ovn-sbctl in the deployment requires some
thought.

I have one comment that's really about the patch rather than the pros
and cons, but I didn't mention it earlier and I should have.  It's
related to this.

> cons: There are *small* chances that inconsistent (stale) data can be read
> by the existing tools (except daemon mode, because this patch enforces
> leader-only as default for daemon mode). [...]

I believe the patch does something like "daemon_mode = true; leader_only
= true;" for --daemon-mode.  I think this is a bad idea because it means
that "--no-leader-only --daemon-mode" will not have the intended
effect.  Instead, we should start out with "leader_only = -1;" and then
after parsing options do something like "if (leader_only < 0) {
leader_only = daemon_mode; }".

> I just listed the pros and cons for discussion. I don't have a strong
> opinion which way is the best. (The number of pros/cons doesn't necessarily
> mean it is better/worse)

I don't have a strong opinion either, but I want to make sure that the
implications receive some discussion.
Han Zhou Oct. 3, 2019, 7:38 p.m. UTC | #5
On Thu, Oct 3, 2019 at 11:35 AM Ben Pfaff <blp@ovn.org> wrote:
>
> On Thu, Oct 03, 2019 at 10:19:35AM -0700, Han Zhou wrote:
> > On Thu, Oct 3, 2019 at 9:09 AM Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > On Tue, Oct 01, 2019 at 05:05:28PM -0700, amginwal@gmail.com wrote:
> > > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > > >
> > > > When using ovn-nb/sbctl running in cluster, one can use local
> > > > socket to run different commands. It is very inconvenient to pass
> > > > no-leader-only in different tools using ovn-nb/sbctl instead of
> > > > allowing one to to connect to any nodes in the cluster including
> > > > itself.
> > > > e.g common usage ovn-nb/sbctl show.
> > > > Hence, this commit handles the same.
> > > >
> > > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> > >
> > > This change makes more of a difference than its size implies, because
it
> > > means that scripts that previously were guaranteed to get up-to-date
> > > data can now get inconsistent results.  It loses read-after-write
> > > consistency, for example.  I'd really prefer to avoid surprising users
> > > with that kind of thing (especially as a change).
> > >
> > > If it's common to want that kind of behavior, though, perhaps there
> > > could be a nice way to set it as the default for a session.  In daemon
> > > mode, of course, it's already possible to control it for the daemon's
> > > users.  One option for outside daemon mode might be to introduce an
> > > environment variable.  The environment variable could be specific to
> > > this feature, e.g. OVN_LEADER_ONLY=0 or OVN_LEADER_ONLY=1, or it could
> > > be a general-purpose options variable,
> > > e.g. OVN_OPTIONS=--no-leader-only.  I don't know whether it should be
> > > specific to one of ovn-sbctl and ovn-nbctl or apply to both.
> > >
> > > Have you thought about these possibilities?
> > >
> > Thanks Ben for the comments. Let's compare the pros and cons of each
> > solution:
>
> Thank you for listing the pros and cons.  I agree with them.
>
> One factor that is not completely captured by your list is the effect on
> transitioning deployments from standalone to clustered.  Currently, this
> should not have surprising effects: one would just replace the single
> database URL by a comma-separated list and it will continue to work,
> just more reliably.  If the default changes to --no-leader-only, then
> each use of ovn-nbctl and ovn-sbctl in the deployment requires some
> thought.
>
> I have one comment that's really about the patch rather than the pros
> and cons, but I didn't mention it earlier and I should have.  It's
> related to this.
>
> > cons: There are *small* chances that inconsistent (stale) data can be
read
> > by the existing tools (except daemon mode, because this patch enforces
> > leader-only as default for daemon mode). [...]
>
> I believe the patch does something like "daemon_mode = true; leader_only
> = true;" for --daemon-mode.  I think this is a bad idea because it means
> that "--no-leader-only --daemon-mode" will not have the intended
> effect.  Instead, we should start out with "leader_only = -1;" and then
> after parsing options do something like "if (leader_only < 0) {
> leader_only = daemon_mode; }".
>
Thanks for the finding!

> > I just listed the pros and cons for discussion. I don't have a strong
> > opinion which way is the best. (The number of pros/cons doesn't
necessarily
> > mean it is better/worse)
>
> I don't have a strong opinion either, but I want to make sure that the
> implications receive some discussion.

Overall, the --no-leader-only is for convenience. In most cases when
executing ovn-nb/sbctl it is read-only using default OVN_NB/SB_DB which is
the unix socket. Even when using TCP/SSL, when it is read-only we'd prefer
--no-leader-only because it doesn't need to retry until finding the leader.
The --leader-only is for correctness/safety. We don't want any surprise
when user should use --leader-only but forget to.

So, we should make the decision based on whether we want the default
behavior to be the most convenient one or the safest one. I guess if we
have to choose, the safety should be more important. So I tend to agree
with the 3rd option as suggested by Ben, which by default ensures safety,
while providing a way for user to change the default behavior when he/she
decides to. It is a compromise between the two other options. I would
propose different environment variable names OVN_NBCTL_OPTIONS and
OVN_SBCTL_OPTIONS, to make the them more specific to the command but
general enough for adjusting default behavior for any options. What do you
think?

Thanks,
Han
aginwala Oct. 3, 2019, 9:17 p.m. UTC | #6
Thanks Han and ben for the suggestions. However, one more reason for using
this approach is because ovn-controller uses no-leader-only by default so
that all the chassis can be randomly distributed to talk to any node in the
cluster to avoid overloading leader node in a large scale env
"ovsdb_idl_set_leader_only(ovnsb_idl_loop.idl, false)"

On Thu, Oct 3, 2019 at 12:38 PM Han Zhou <zhouhan@gmail.com> wrote:

> On Thu, Oct 3, 2019 at 11:35 AM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Thu, Oct 03, 2019 at 10:19:35AM -0700, Han Zhou wrote:
> > > On Thu, Oct 3, 2019 at 9:09 AM Ben Pfaff <blp@ovn.org> wrote:
> > > >
> > > > On Tue, Oct 01, 2019 at 05:05:28PM -0700, amginwal@gmail.com wrote:
> > > > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > > > >
> > > > > When using ovn-nb/sbctl running in cluster, one can use local
> > > > > socket to run different commands. It is very inconvenient to pass
> > > > > no-leader-only in different tools using ovn-nb/sbctl instead of
> > > > > allowing one to to connect to any nodes in the cluster including
> > > > > itself.
> > > > > e.g common usage ovn-nb/sbctl show.
> > > > > Hence, this commit handles the same.
> > > > >
> > > > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> > > >
> > > > This change makes more of a difference than its size implies, because
> it
> > > > means that scripts that previously were guaranteed to get up-to-date
> > > > data can now get inconsistent results.  It loses read-after-write
> > > > consistency, for example.  I'd really prefer to avoid surprising
> users
> > > > with that kind of thing (especially as a change).
> > > >
> > > > If it's common to want that kind of behavior, though, perhaps there
> > > > could be a nice way to set it as the default for a session.  In
> daemon
> > > > mode, of course, it's already possible to control it for the daemon's
> > > > users.  One option for outside daemon mode might be to introduce an
> > > > environment variable.  The environment variable could be specific to
> > > > this feature, e.g. OVN_LEADER_ONLY=0 or OVN_LEADER_ONLY=1, or it
> could
> > > > be a general-purpose options variable,
> > > > e.g. OVN_OPTIONS=--no-leader-only.  I don't know whether it should be
> > > > specific to one of ovn-sbctl and ovn-nbctl or apply to both.
> > > >
> > > > Have you thought about these possibilities?
> > > >
> > > Thanks Ben for the comments. Let's compare the pros and cons of each
> > > solution:
> >
> > Thank you for listing the pros and cons.  I agree with them.
> >
> > One factor that is not completely captured by your list is the effect on
> > transitioning deployments from standalone to clustered.  Currently, this
> > should not have surprising effects: one would just replace the single
> > database URL by a comma-separated list and it will continue to work,
> > just more reliably.  If the default changes to --no-leader-only, then
> > each use of ovn-nbctl and ovn-sbctl in the deployment requires some
> > thought.
> >
> > I have one comment that's really about the patch rather than the pros
> > and cons, but I didn't mention it earlier and I should have.  It's
> > related to this.
> >
> > > cons: There are *small* chances that inconsistent (stale) data can be
> read
> > > by the existing tools (except daemon mode, because this patch enforces
> > > leader-only as default for daemon mode). [...]
> >
> > I believe the patch does something like "daemon_mode = true; leader_only
> > = true;" for --daemon-mode.  I think this is a bad idea because it means
> > that "--no-leader-only --daemon-mode" will not have the intended
> > effect.  Instead, we should start out with "leader_only = -1;" and then
> > after parsing options do something like "if (leader_only < 0) {
> > leader_only = daemon_mode; }".
> >
> Thanks for the finding!
>
> > > I just listed the pros and cons for discussion. I don't have a strong
> > > opinion which way is the best. (The number of pros/cons doesn't
> necessarily
> > > mean it is better/worse)
> >
> > I don't have a strong opinion either, but I want to make sure that the
> > implications receive some discussion.
>
> Overall, the --no-leader-only is for convenience. In most cases when
> executing ovn-nb/sbctl it is read-only using default OVN_NB/SB_DB which is
> the unix socket. Even when using TCP/SSL, when it is read-only we'd prefer
> --no-leader-only because it doesn't need to retry until finding the leader.
> The --leader-only is for correctness/safety. We don't want any surprise
> when user should use --leader-only but forget to.
>
> So, we should make the decision based on whether we want the default
> behavior to be the most convenient one or the safest one. I guess if we
> have to choose, the safety should be more important. So I tend to agree
> with the 3rd option as suggested by Ben, which by default ensures safety,
> while providing a way for user to change the default behavior when he/she
> decides to. It is a compromise between the two other options. I would
> propose different environment variable names OVN_NBCTL_OPTIONS and
> OVN_SBCTL_OPTIONS, to make the them more specific to the command but
> general enough for adjusting default behavior for any options. What do you
> think?
>
I am ok with the env variable change if that's the final
conclusion.However, if we are changing for both nb/sb-ctl , I think just
OVN_OPTIONS is ok too instead of setting two env vars. Let me know and I
can send v2 accordingly.

>
> Thanks,
> Han
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff Oct. 4, 2019, 5:38 p.m. UTC | #7
OK.

I've made the points that I consider relevant.  I'll let the rest of the
OVN community come to consensus here.

On Thu, Oct 03, 2019 at 02:17:48PM -0700, aginwala wrote:
> Thanks Han and ben for the suggestions. However, one more reason for using
> this approach is because ovn-controller uses no-leader-only by default so
> that all the chassis can be randomly distributed to talk to any node in the
> cluster to avoid overloading leader node in a large scale env
> "ovsdb_idl_set_leader_only(ovnsb_idl_loop.idl, false)"
> 
> On Thu, Oct 3, 2019 at 12:38 PM Han Zhou <zhouhan@gmail.com> wrote:
> 
> > On Thu, Oct 3, 2019 at 11:35 AM Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > On Thu, Oct 03, 2019 at 10:19:35AM -0700, Han Zhou wrote:
> > > > On Thu, Oct 3, 2019 at 9:09 AM Ben Pfaff <blp@ovn.org> wrote:
> > > > >
> > > > > On Tue, Oct 01, 2019 at 05:05:28PM -0700, amginwal@gmail.com wrote:
> > > > > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > > > > >
> > > > > > When using ovn-nb/sbctl running in cluster, one can use local
> > > > > > socket to run different commands. It is very inconvenient to pass
> > > > > > no-leader-only in different tools using ovn-nb/sbctl instead of
> > > > > > allowing one to to connect to any nodes in the cluster including
> > > > > > itself.
> > > > > > e.g common usage ovn-nb/sbctl show.
> > > > > > Hence, this commit handles the same.
> > > > > >
> > > > > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> > > > >
> > > > > This change makes more of a difference than its size implies, because
> > it
> > > > > means that scripts that previously were guaranteed to get up-to-date
> > > > > data can now get inconsistent results.  It loses read-after-write
> > > > > consistency, for example.  I'd really prefer to avoid surprising
> > users
> > > > > with that kind of thing (especially as a change).
> > > > >
> > > > > If it's common to want that kind of behavior, though, perhaps there
> > > > > could be a nice way to set it as the default for a session.  In
> > daemon
> > > > > mode, of course, it's already possible to control it for the daemon's
> > > > > users.  One option for outside daemon mode might be to introduce an
> > > > > environment variable.  The environment variable could be specific to
> > > > > this feature, e.g. OVN_LEADER_ONLY=0 or OVN_LEADER_ONLY=1, or it
> > could
> > > > > be a general-purpose options variable,
> > > > > e.g. OVN_OPTIONS=--no-leader-only.  I don't know whether it should be
> > > > > specific to one of ovn-sbctl and ovn-nbctl or apply to both.
> > > > >
> > > > > Have you thought about these possibilities?
> > > > >
> > > > Thanks Ben for the comments. Let's compare the pros and cons of each
> > > > solution:
> > >
> > > Thank you for listing the pros and cons.  I agree with them.
> > >
> > > One factor that is not completely captured by your list is the effect on
> > > transitioning deployments from standalone to clustered.  Currently, this
> > > should not have surprising effects: one would just replace the single
> > > database URL by a comma-separated list and it will continue to work,
> > > just more reliably.  If the default changes to --no-leader-only, then
> > > each use of ovn-nbctl and ovn-sbctl in the deployment requires some
> > > thought.
> > >
> > > I have one comment that's really about the patch rather than the pros
> > > and cons, but I didn't mention it earlier and I should have.  It's
> > > related to this.
> > >
> > > > cons: There are *small* chances that inconsistent (stale) data can be
> > read
> > > > by the existing tools (except daemon mode, because this patch enforces
> > > > leader-only as default for daemon mode). [...]
> > >
> > > I believe the patch does something like "daemon_mode = true; leader_only
> > > = true;" for --daemon-mode.  I think this is a bad idea because it means
> > > that "--no-leader-only --daemon-mode" will not have the intended
> > > effect.  Instead, we should start out with "leader_only = -1;" and then
> > > after parsing options do something like "if (leader_only < 0) {
> > > leader_only = daemon_mode; }".
> > >
> > Thanks for the finding!
> >
> > > > I just listed the pros and cons for discussion. I don't have a strong
> > > > opinion which way is the best. (The number of pros/cons doesn't
> > necessarily
> > > > mean it is better/worse)
> > >
> > > I don't have a strong opinion either, but I want to make sure that the
> > > implications receive some discussion.
> >
> > Overall, the --no-leader-only is for convenience. In most cases when
> > executing ovn-nb/sbctl it is read-only using default OVN_NB/SB_DB which is
> > the unix socket. Even when using TCP/SSL, when it is read-only we'd prefer
> > --no-leader-only because it doesn't need to retry until finding the leader.
> > The --leader-only is for correctness/safety. We don't want any surprise
> > when user should use --leader-only but forget to.
> >
> > So, we should make the decision based on whether we want the default
> > behavior to be the most convenient one or the safest one. I guess if we
> > have to choose, the safety should be more important. So I tend to agree
> > with the 3rd option as suggested by Ben, which by default ensures safety,
> > while providing a way for user to change the default behavior when he/she
> > decides to. It is a compromise between the two other options. I would
> > propose different environment variable names OVN_NBCTL_OPTIONS and
> > OVN_SBCTL_OPTIONS, to make the them more specific to the command but
> > general enough for adjusting default behavior for any options. What do you
> > think?
> >
> I am ok with the env variable change if that's the final
> conclusion.However, if we are changing for both nb/sb-ctl , I think just
> OVN_OPTIONS is ok too instead of setting two env vars. Let me know and I
> can send v2 accordingly.
> 
> >
> > Thanks,
> > Han
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
aginwala Oct. 9, 2019, 11:58 p.m. UTC | #8
Thanks Ben and Han. As suggested, I used env vars OVN_NBCTL_OPTIONS and
OVN_SBCTL_OPTIONS and sent the new patch @
https://patchwork.ozlabs.org/patch/1174211/. PTAL.

On Fri, Oct 4, 2019 at 10:39 AM Ben Pfaff <blp@ovn.org> wrote:

> OK.
>
> I've made the points that I consider relevant.  I'll let the rest of the
> OVN community come to consensus here.
>
> On Thu, Oct 03, 2019 at 02:17:48PM -0700, aginwala wrote:
> > Thanks Han and ben for the suggestions. However, one more reason for
> using
> > this approach is because ovn-controller uses no-leader-only by default so
> > that all the chassis can be randomly distributed to talk to any node in
> the
> > cluster to avoid overloading leader node in a large scale env
> > "ovsdb_idl_set_leader_only(ovnsb_idl_loop.idl, false)"
> >
> > On Thu, Oct 3, 2019 at 12:38 PM Han Zhou <zhouhan@gmail.com> wrote:
> >
> > > On Thu, Oct 3, 2019 at 11:35 AM Ben Pfaff <blp@ovn.org> wrote:
> > > >
> > > > On Thu, Oct 03, 2019 at 10:19:35AM -0700, Han Zhou wrote:
> > > > > On Thu, Oct 3, 2019 at 9:09 AM Ben Pfaff <blp@ovn.org> wrote:
> > > > > >
> > > > > > On Tue, Oct 01, 2019 at 05:05:28PM -0700, amginwal@gmail.com
> wrote:
> > > > > > > From: Aliasgar Ginwala <aginwala@ebay.com>
> > > > > > >
> > > > > > > When using ovn-nb/sbctl running in cluster, one can use local
> > > > > > > socket to run different commands. It is very inconvenient to
> pass
> > > > > > > no-leader-only in different tools using ovn-nb/sbctl instead of
> > > > > > > allowing one to to connect to any nodes in the cluster
> including
> > > > > > > itself.
> > > > > > > e.g common usage ovn-nb/sbctl show.
> > > > > > > Hence, this commit handles the same.
> > > > > > >
> > > > > > > Signed-off-by: Aliasgar Ginwala <aginwala@ebay.com>
> > > > > >
> > > > > > This change makes more of a difference than its size implies,
> because
> > > it
> > > > > > means that scripts that previously were guaranteed to get
> up-to-date
> > > > > > data can now get inconsistent results.  It loses read-after-write
> > > > > > consistency, for example.  I'd really prefer to avoid surprising
> > > users
> > > > > > with that kind of thing (especially as a change).
> > > > > >
> > > > > > If it's common to want that kind of behavior, though, perhaps
> there
> > > > > > could be a nice way to set it as the default for a session.  In
> > > daemon
> > > > > > mode, of course, it's already possible to control it for the
> daemon's
> > > > > > users.  One option for outside daemon mode might be to introduce
> an
> > > > > > environment variable.  The environment variable could be
> specific to
> > > > > > this feature, e.g. OVN_LEADER_ONLY=0 or OVN_LEADER_ONLY=1, or it
> > > could
> > > > > > be a general-purpose options variable,
> > > > > > e.g. OVN_OPTIONS=--no-leader-only.  I don't know whether it
> should be
> > > > > > specific to one of ovn-sbctl and ovn-nbctl or apply to both.
> > > > > >
> > > > > > Have you thought about these possibilities?
> > > > > >
> > > > > Thanks Ben for the comments. Let's compare the pros and cons of
> each
> > > > > solution:
> > > >
> > > > Thank you for listing the pros and cons.  I agree with them.
> > > >
> > > > One factor that is not completely captured by your list is the
> effect on
> > > > transitioning deployments from standalone to clustered.  Currently,
> this
> > > > should not have surprising effects: one would just replace the single
> > > > database URL by a comma-separated list and it will continue to work,
> > > > just more reliably.  If the default changes to --no-leader-only, then
> > > > each use of ovn-nbctl and ovn-sbctl in the deployment requires some
> > > > thought.
> > > >
> > > > I have one comment that's really about the patch rather than the pros
> > > > and cons, but I didn't mention it earlier and I should have.  It's
> > > > related to this.
> > > >
> > > > > cons: There are *small* chances that inconsistent (stale) data can
> be
> > > read
> > > > > by the existing tools (except daemon mode, because this patch
> enforces
> > > > > leader-only as default for daemon mode). [...]
> > > >
> > > > I believe the patch does something like "daemon_mode = true;
> leader_only
> > > > = true;" for --daemon-mode.  I think this is a bad idea because it
> means
> > > > that "--no-leader-only --daemon-mode" will not have the intended
> > > > effect.  Instead, we should start out with "leader_only = -1;" and
> then
> > > > after parsing options do something like "if (leader_only < 0) {
> > > > leader_only = daemon_mode; }".
> > > >
> > > Thanks for the finding!
> > >
> > > > > I just listed the pros and cons for discussion. I don't have a
> strong
> > > > > opinion which way is the best. (The number of pros/cons doesn't
> > > necessarily
> > > > > mean it is better/worse)
> > > >
> > > > I don't have a strong opinion either, but I want to make sure that
> the
> > > > implications receive some discussion.
> > >
> > > Overall, the --no-leader-only is for convenience. In most cases when
> > > executing ovn-nb/sbctl it is read-only using default OVN_NB/SB_DB
> which is
> > > the unix socket. Even when using TCP/SSL, when it is read-only we'd
> prefer
> > > --no-leader-only because it doesn't need to retry until finding the
> leader.
> > > The --leader-only is for correctness/safety. We don't want any surprise
> > > when user should use --leader-only but forget to.
> > >
> > > So, we should make the decision based on whether we want the default
> > > behavior to be the most convenient one or the safest one. I guess if we
> > > have to choose, the safety should be more important. So I tend to agree
> > > with the 3rd option as suggested by Ben, which by default ensures
> safety,
> > > while providing a way for user to change the default behavior when
> he/she
> > > decides to. It is a compromise between the two other options. I would
> > > propose different environment variable names OVN_NBCTL_OPTIONS and
> > > OVN_SBCTL_OPTIONS, to make the them more specific to the command but
> > > general enough for adjusting default behavior for any options. What do
> you
> > > think?
> > >
> > I am ok with the env variable change if that's the final
> > conclusion.However, if we are changing for both nb/sb-ctl , I think just
> > OVN_OPTIONS is ok too instead of setting two env vars. Let me know and I
> > can send v2 accordingly.
> >
> > >
> > > Thanks,
> > > Han
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
>

Patch
diff mbox series

diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index fd75c0e44..3dd05fa65 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -1192,14 +1192,15 @@ 
     <dt><code>--leader-only</code></dt>
     <dt><code>--no-leader-only</code></dt>
     <dd>
-      By default, or with <code>--leader-only</code>, when the database server
-      is a clustered database, <code>ovn-nbctl</code> will avoid servers other
-      than the cluster leader.  This ensures that any data that
-      <code>ovn-nbctl</code> reads and reports is up-to-date.  With
-      <code>--no-leader-only</code>, <code>ovn-nbctl</code> will use any server
-      in the cluster, which means that for read-only transactions it can report
-      and act on stale data (transactions that modify the database are always
-      serialized even with <code>--no-leader-only</code>).  Refer to
+      By default, or with <code>--no-leader-only</code>, when the database
+      server is a clustered database, <code>ovn-nbctl</code> may connect to
+      any server in the cluster, which means that for read-only transactions
+      it can report and act on stale data (transactions that modify the
+      database are always serialized even with <code>--no-leader-only</code>).
+      To avoid reading stale data, one can specify <code>--leader-only</code>,
+      so that <code>ovn-nbctl</code> will avoid servers other than the cluster
+      leader. For daemon mode, since it is for long running connections,
+      default is set to <code>--leader-only</code>. Refer to
       <code>Understanding Cluster Consistency</code> in <code>ovsdb</code>(7)
       for more information.
     </dd>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index a89a9cb4d..3804dd25a 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -81,7 +81,7 @@  static struct ovsdb_idl_txn *the_idl_txn;
 OVS_NO_RETURN static void nbctl_exit(int status);
 
 /* --leader-only, --no-leader-only: Only accept the leader in a cluster. */
-static int leader_only = true;
+static int leader_only = false;
 
 /* --shuffle-remotes, --no-shuffle-remotes: Shuffle the order of remotes that
  * are specified in the connetion method string. */
@@ -188,6 +188,7 @@  main(int argc, char *argv[])
                       "(use --help for help)");
         }
         daemon_mode = true;
+        leader_only = true;
     }
     /* Initialize IDL. */
     idl = the_idl = ovsdb_idl_create_unconnected(&nbrec_idl_class, true);
diff --git a/utilities/ovn-sbctl.8.in b/utilities/ovn-sbctl.8.in
index 2aaa457e8..f52412812 100644
--- a/utilities/ovn-sbctl.8.in
+++ b/utilities/ovn-sbctl.8.in
@@ -53,16 +53,16 @@  e.g. \fBssl:192.168.10.5:6640\fR, as described in \fBovsdb\fR(7).
 .
 .IP "\fB\-\-leader\-only\fR"
 .IQ "\fB\-\-no\-leader\-only\fR"
-By default, or with \fB\-\-leader\-only\fR, when the database server
-is a clustered database, \fBovn\-sbctl\fR will avoid servers other
-than the cluster leader.  This ensures that any data that
-\fBovn\-sbctl\fR reads and reports is up-to-date.  With
-\fB\-\-no\-leader\-only\fR, \fBovn\-sbctl\fR will use any server in
-the cluster, which means that for read-only transactions it can report
+By default, or with \fB\-\-no\-leader\-only\fR, when the database server
+is a clustered database, \fBovn\-sbctl\fR may connect to any server
+in the cluster, which means that for read-only transactions it can report
 and act on stale data (transactions that modify the database are
-always serialized even with \fB\-\-no\-leader\-only\fR).  Refer to
-\fBUnderstanding Cluster Consistency\fR in \fBovsdb\fR(7) for more
-information.
+always serialized even with \fB\-\-no\-leader\-only\fR). To avoid reading
+stale data, one can specify \fB\-\-leader\-only\fR, so that
+\fBovn\-sbctl\fR will avoid servers other than the cluster leader. For daemon
+mode, since it is for long running connections, default is set to
+\fB\-\-leader\-only\fR. Refer to \fBUnderstanding Cluster Consistency\fR in
+\fBovsdb\fR(7) for more information.
 .
 .IP "\fB\-\-no\-syslog\fR"
 By default, \fBovn\-sbctl\fR logs its arguments and the details of any
diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
index 9a9b6f0ec..f1cb8790f 100644
--- a/utilities/ovn-sbctl.c
+++ b/utilities/ovn-sbctl.c
@@ -82,7 +82,7 @@  static struct ovsdb_idl_txn *the_idl_txn;
 OVS_NO_RETURN static void sbctl_exit(int status);
 
 /* --leader-only, --no-leader-only: Only accept the leader in a cluster. */
-static int leader_only = true;
+static int leader_only = false;
 
 static void sbctl_cmd_init(void);
 OVS_NO_RETURN static void usage(void);