Flavio Leitner <f...@sysclose.org> writes: > On Tue, Aug 11, 2020 at 03:58:44PM -0400, Aaron Conole wrote: >> 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? > > 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 >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev