[ovs-dev] [PATCH] connmgr: support changing openflow versions without restarting
Flavio Leitner
fbl at sysclose.org
Tue Aug 11 21:11:30 UTC 2020
On Tue, Aug 11, 2020 at 03:58:44PM -0400, Aaron Conole wrote:
> Flavio Leitner <fbl at sysclose.org> writes:
>
> > Hi Aaron,
> >
> > Thanks for the patch, see my comments below.
> >
> > On Fri, Aug 07, 2020 at 05:32:03PM -0400, Aaron Conole wrote:
> >> When commit a0baa7dfa4fe ("connmgr: Make treatment of active and passive
> >> connections more uniform") was applied, it did not take into account
> >> that a reconfiguration of the allowed_versions setting would require a
> >> reload of the ofservice object (only accomplished via a restart of OvS).
> >>
> >> For now, during the reconfigure cycle, we delete the ofservice object and
> >> then recreate it immediately. A new test is added to ensure we do not
> >> break this behavior again.
> >>
> >> Fixes: a0baa7dfa4fe ("connmgr: Make treatment of active and passive connections more uniform")
> >> Suggested-by: Ben Pfaff <blp at ovn.org>
> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834
> >> Signed-off-by: Aaron Conole <aconole at redhat.com>
> >> ---
> >> NOTE: The log on line 608 will flag the 0-day robot, but I thought
> >> for string searching purposes it's better to keep it all one
> >> line.
> >>
> >> ofproto/connmgr.c | 25 +++++++++++++++++++------
> >> tests/bridge.at | 17 +++++++++++++++++
> >> 2 files changed, 36 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> >> index 51d656cba9..b57a381097 100644
> >> --- a/ofproto/connmgr.c
> >> +++ b/ofproto/connmgr.c
> >> @@ -190,8 +190,9 @@ struct ofservice {
> >>
> >> static void ofservice_run(struct ofservice *);
> >> static void ofservice_wait(struct ofservice *);
> >> -static void ofservice_reconfigure(struct ofservice *,
> >> - const struct ofproto_controller *)
> >> +static bool ofservice_reconfigure(struct ofservice *,
> >> + const struct ofproto_controller *,
> >> + bool)
> >> OVS_REQUIRES(ofproto_mutex);
> >> static void ofservice_create(struct connmgr *mgr, const char *target,
> >> const struct ofproto_controller *)
> >> @@ -602,7 +603,14 @@ connmgr_set_controllers(struct connmgr *mgr, struct shash *controllers)
> >> target);
> >> ofservice_destroy(ofservice);
> >> } else {
> >> - ofservice_reconfigure(ofservice, c);
> >> + if (ofservice_reconfigure(ofservice, c, true) == false) {
> >> + char *target_to_restore = xstrdup(target);
> >> + VLOG_INFO("%s: restarting controller \"%s\" due to version change",
> >> + mgr->name, target);
> >> + ofservice_destroy(ofservice);
> >> + ofservice_create(mgr, target_to_restore, c);
> >> + free(target_to_restore);
> >
> > This seems more complicated than it needs to be because the only
> > situation where we care to re-create is when we set the controller
> > and the protocol version has changed, so changing the API of
> > reconfigure makes little sense and becomes confusing to follow up.
> > E.g. ofservice_reconfigure(.., true) == false.
>
> I thought maybe there could be some other kind of reconfigure situation
> in the future that might require a rebuild of the service anyway. This
> is why I wrote the API to return a boolean. Maybe it would have been
> clearer if I set it up as two commits?
It makes the code complex for a possible use case in the future that
might never happen. We can easily move the code around and extend the
API when the need arises.
> > Also that ofservice_create() calls ofservice_reconfigure() again.
> > What do you think of this instead?
> >
> > /* Changing version requires to re-create ofservice. */
> > if (ofservice->s.allowed_versions == c->allowed_versions) {
> > ofservice_reconfigure(ofservice, c);
>
> If we do this, then we should remove the version check in
> the ofservice_reconfigure function also, I think - it would not make
> sense any more.
I agree and perhaps add an assert there in case it's different.
> No strong opinion on the approach, though.
I think the new API would be OK if ofservice_reconfigure() was capable
of doing everything to reconfigure an ofservice. For example, in the
original case, all that was needed was to call ofservice_close_all().
Now the caller has to know whether the version mismatch is allowed
or not. Saying it's not, the function does partial work, which is
not good.
Another approach is to change ofservice_reconfigure() to fail if the
versions don't match and then handle outside like below:
ofservice_reconfigure()
/* Fail as that requires to re-create ofservice. */
if (ofservice->s.allowed_versions != settings->allowed_versions) {
return -EINVAL;
}
connmgr_set_controllers()
[...]
/* Re-create ofservice if can't be reconfigured. */
if (ofservice_reconfigure(ofservice, c)) {
char *target_to_restore = xstrdup(target);
VLOG_INFO("%s: restarting controller \"%s\" due to version change",
mgr->name, target);
ofservice_destroy(ofservice);
ofservice_create(mgr, target_to_restore, c);
free(target_to_restore);
}
> > } else {
> > char *target_to_restore = xstrdup(target);
> > VLOG_INFO("%s: restarting controller \"%s\" due to version change",
> > mgr->name, target);
> > ofservice_destroy(ofservice);
> > ofservice_create(mgr, target_to_restore, c);
> > free(target_to_restore);
> > }
> >
> >> + }
> >> }
> >> }
> >>
> >> @@ -1935,7 +1943,7 @@ ofservice_create(struct connmgr *mgr, const char *target,
> >> ofservice->rconn = rconn;
> >> ofservice->pvconn = pvconn;
> >> ofservice->s = *c;
> >> - ofservice_reconfigure(ofservice, c);
> >> + (void)ofservice_reconfigure(ofservice, c, false);
> >
> > OVS doesn't use that construct as far as I can tell.
>
> Yes - I wanted to forcibly ignore the return. I think it isn't needed.
>
> >>
> >> VLOG_INFO("%s: added %s controller \"%s\"",
> >> mgr->name, ofconn_type_to_string(ofservice->type), target);
> >> @@ -2011,9 +2019,10 @@ ofservice_wait(struct ofservice *ofservice)
> >> }
> >> }
> >>
> >> -static void
> >> +static bool
> >> ofservice_reconfigure(struct ofservice *ofservice,
> >> - const struct ofproto_controller *settings)
> >> + const struct ofproto_controller *settings,
> >> + bool reject_version)
> >
> > It would be nice to have a documentation about the purpose of
> > reject_version.
>
> Maybe a different name, actually? maybe invert it and call it
> 'allow_version_mismatch'? The condition then becomes:
>
> if (!allow_version_mismatch) {
> return false;
> }
>
> And I would change the calls?
>
> WDYT?
works for me.
>
> >> OVS_REQUIRES(ofproto_mutex)
> >> {
> >> /* If the allowed OpenFlow versions change, close all of the existing
> >> @@ -2021,6 +2030,9 @@ ofservice_reconfigure(struct ofservice *ofservice,
> >> * version. */
> >> if (ofservice->s.allowed_versions != settings->allowed_versions) {
> >> ofservice_close_all(ofservice);
> >> + if (reject_version) {
> >> + return false;
> >> + }
> >> }
> >>
> >> ofservice->s = *settings;
> >> @@ -2029,6 +2041,7 @@ ofservice_reconfigure(struct ofservice *ofservice,
> >> LIST_FOR_EACH (ofconn, ofservice_node, &ofservice->conns) {
> >> ofconn_reconfigure(ofconn, settings);
> >> }
> >> + return true;
> >
> > The file has mixed styles with and without empty line above
> > return. I personally prefer with it to make a clear separation
> > but yeah, no strong opinion here.
> >
> >> }
> >>
> >> /* Finds and returns the ofservice within 'mgr' that has the given
> >> diff --git a/tests/bridge.at b/tests/bridge.at
> >> index d48463e263..904f1381c7 100644
> >> --- a/tests/bridge.at
> >> +++ b/tests/bridge.at
> >> @@ -103,3 +103,20 @@ AT_CHECK([ovs-appctl -t ovs-vswitchd version], [0], [ignore])
> >> OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> >> OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >> AT_CLEANUP
> >> +
> >> +AT_SETUP([bridge - change ofproto versions])
> >> +dnl Start vswitch and add a version test bridge
> >> +OVS_VSWITCHD_START(
> >> + [add-br vr_test0 -- \
> >> + set bridge vr_test0 datapath-type=dummy \
> >> + protocols=OpenFlow10])
> >> +
> >> +dnl set the version to include, say, OpenFlow14
> >> +AT_CHECK([ovs-vsctl set bridge vr_test0 protocols=OpenFlow10,OpenFlow14])
> >> +
> >> +dnl now try to use bundle action on a flow
> >> +AT_CHECK([ovs-ofctl add-flow vr_test0 --bundle actions=normal])
> >> +
> >> +OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> >> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >> +AT_CLEANUP
> >> --
> >
> > Nice, thanks for adding the test.
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
fbl
More information about the dev
mailing list