| Message ID | 1462208933-664-1-git-send-email-aconole@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Aaron Conole <aconole@redhat.com> writes: > The ovs-ctl script was changed recently to have per-service start/stop > control. However, when that change was made the add_managers() call was > overlooked. This results in calls to `ovs-ctl --no-ovs-vswitchd start` > telling the ovsdb-server to connect to the remote controllers. > > This commit disables the effect of the add_managers call if the > `--no-ovs-vswitchd` argument is given. > > Fixes: 7fc28c50c012 ("ovs-ctl: Allow selective start for db and switch") > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- Ping. > utilities/ovs-ctl.in | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in > index 4aa2999..f494312 100755 > --- a/utilities/ovs-ctl.in > +++ b/utilities/ovs-ctl.in > @@ -199,9 +199,11 @@ add_managers () { > # churn in the database at startup or restart. (For example, managers > # won't briefly see empty datapath-id or ofport columns for records that > # exist at startup.) > - action "Enabling remote OVSDB managers" \ > - ovs-appctl -t ovsdb-server ovsdb-server/add-remote \ > - db:Open_vSwitch,Open_vSwitch,manager_options > + if test X"$OVS_VSWITCHD" = Xyes; then > + action "Enabling remote OVSDB managers" \ > + ovs-appctl -t ovsdb-server ovsdb-server/add-remote \ > + db:Open_vSwitch,Open_vSwitch,manager_options > + fi > } > > do_start_forwarding () {
On Mon, May 02, 2016 at 01:08:53PM -0400, Aaron Conole wrote: > The ovs-ctl script was changed recently to have per-service start/stop > control. However, when that change was made the add_managers() call was > overlooked. This results in calls to `ovs-ctl --no-ovs-vswitchd start` > telling the ovsdb-server to connect to the remote controllers. > > This commit disables the effect of the add_managers call if the > `--no-ovs-vswitchd` argument is given. > > Fixes: 7fc28c50c012 ("ovs-ctl: Allow selective start for db and switch") > Signed-off-by: Aaron Conole <aconole@redhat.com> I think this means that if we first start ovs-vswitchd, then separately start ovsdb-server in a later call to ovs-ctl, nothing will add the remote managers. That's because, when we start ovs-vswitchd, ovsdb-server isn't running, so we can't enable anything, and then later when we start ovsdb-server, we don't add the remotes because we're not starting ovs-vswitchd. So, it might be better to condition this on $OVSDB_SERVER. Then we'll at least get the above case right, and the behavior in the common case where we start both at once will be unchanged. There will be a bit more churn from managers' perspective in the uncommon case where we start in an odd order, but at least we'll connect to them.
Ben Pfaff <blp@ovn.org> writes: > On Mon, May 02, 2016 at 01:08:53PM -0400, Aaron Conole wrote: >> The ovs-ctl script was changed recently to have per-service start/stop >> control. However, when that change was made the add_managers() call was >> overlooked. This results in calls to `ovs-ctl --no-ovs-vswitchd start` >> telling the ovsdb-server to connect to the remote controllers. >> >> This commit disables the effect of the add_managers call if the >> `--no-ovs-vswitchd` argument is given. >> >> Fixes: 7fc28c50c012 ("ovs-ctl: Allow selective start for db and switch") >> Signed-off-by: Aaron Conole <aconole@redhat.com> > > I think this means that if we first start ovs-vswitchd, then separately > start ovsdb-server in a later call to ovs-ctl, nothing will add the > remote managers. That's because, when we start ovs-vswitchd, > ovsdb-server isn't running, so we can't enable anything, and then later > when we start ovsdb-server, we don't add the remotes because we're not > starting ovs-vswitchd. Hrrm, I didn't consider that it was common to start vswitchd before the database. > So, it might be better to condition this on $OVSDB_SERVER. Then we'll > at least get the above case right, and the behavior in the common case > where we start both at once will be unchanged. There will be a bit more > churn from managers' perspective in the uncommon case where we start in > an odd order, but at least we'll connect to them. We really only want this to happen when both come alive; given there are tests for both daemons being up, I can try to make it a bit more intelligent and solve either order, with the connect signal only coming when the second daemon starts. I'll cook up a v2 sometime this week, and see if it will pass muster. Thanks for the perspective, Ben! -Aaron
On Wed, May 18, 2016 at 06:05:14AM -0400, Aaron Conole wrote: > Ben Pfaff <blp@ovn.org> writes: > > > On Mon, May 02, 2016 at 01:08:53PM -0400, Aaron Conole wrote: > >> The ovs-ctl script was changed recently to have per-service start/stop > >> control. However, when that change was made the add_managers() call was > >> overlooked. This results in calls to `ovs-ctl --no-ovs-vswitchd start` > >> telling the ovsdb-server to connect to the remote controllers. > >> > >> This commit disables the effect of the add_managers call if the > >> `--no-ovs-vswitchd` argument is given. > >> > >> Fixes: 7fc28c50c012 ("ovs-ctl: Allow selective start for db and switch") > >> Signed-off-by: Aaron Conole <aconole@redhat.com> > > > > I think this means that if we first start ovs-vswitchd, then separately > > start ovsdb-server in a later call to ovs-ctl, nothing will add the > > remote managers. That's because, when we start ovs-vswitchd, > > ovsdb-server isn't running, so we can't enable anything, and then later > > when we start ovsdb-server, we don't add the remotes because we're not > > starting ovs-vswitchd. > > Hrrm, I didn't consider that it was common to start vswitchd before the > database. It is not common, and I don't recommend it, because ovs-vswitchd can't do useful work before it can get its configuration from ovsdb-server, but I don't think it's a good idea to have mysterious problems if someone does happen to start in that order. > > So, it might be better to condition this on $OVSDB_SERVER. Then we'll > > at least get the above case right, and the behavior in the common case > > where we start both at once will be unchanged. There will be a bit more > > churn from managers' perspective in the uncommon case where we start in > > an odd order, but at least we'll connect to them. > > We really only want this to happen when both come alive; given there are > tests for both daemons being up, I can try to make it a bit more > intelligent and solve either order, with the connect signal only coming > when the second daemon starts. OK, that's reasonable too. > I'll cook up a v2 sometime this week, and see if it will pass muster. > > Thanks for the perspective, Ben! Thank you!
diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in index 4aa2999..f494312 100755 --- a/utilities/ovs-ctl.in +++ b/utilities/ovs-ctl.in @@ -199,9 +199,11 @@ add_managers () { # churn in the database at startup or restart. (For example, managers # won't briefly see empty datapath-id or ofport columns for records that # exist at startup.) - action "Enabling remote OVSDB managers" \ - ovs-appctl -t ovsdb-server ovsdb-server/add-remote \ - db:Open_vSwitch,Open_vSwitch,manager_options + if test X"$OVS_VSWITCHD" = Xyes; then + action "Enabling remote OVSDB managers" \ + ovs-appctl -t ovsdb-server ovsdb-server/add-remote \ + db:Open_vSwitch,Open_vSwitch,manager_options + fi } do_start_forwarding () {
The ovs-ctl script was changed recently to have per-service start/stop control. However, when that change was made the add_managers() call was overlooked. This results in calls to `ovs-ctl --no-ovs-vswitchd start` telling the ovsdb-server to connect to the remote controllers. This commit disables the effect of the add_managers call if the `--no-ovs-vswitchd` argument is given. Fixes: 7fc28c50c012 ("ovs-ctl: Allow selective start for db and switch") Signed-off-by: Aaron Conole <aconole@redhat.com> --- utilities/ovs-ctl.in | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)