[ovs-dev] [PATCH] connmgr: support changing openflow versions without restarting
Aaron Conole
aconole at redhat.com
Wed Aug 12 07:46:09 UTC 2020
Flavio Leitner <fbl at sysclose.org> writes:
> 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;
> }
I like this approach.
>
> 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
More information about the dev
mailing list