| Message ID | 20191002000528.68566-1-amginwal@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v1,ovn] ovn-nb/sbctl.c: Set no-leader-only as default for clustered dbs | expand |
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.
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.
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)
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.
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
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 >
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 > >
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 > > > >
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);