Flavio Leitner <f...@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 <b...@ovn.org>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834
>> Signed-off-by: Aaron Conole <acon...@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?
> 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.
No strong opinion on the approach, though.
> } 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?
>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
