On Sat, Aug 8, 2020 at 3:02 AM Aaron Conole <acon...@redhat.com> 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>
Thanks Aaron. Tested this patch with OVN and ovn-controller will now reconnect successfully to the openflow connection if the supported version is added to the bridge protocol. Acked-by: Numan Siddique <num...@ovn.org> Tested-by: Numan Siddique <num...@ovn.org> Thanks Numan > --- > 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); > + } > } > } > > @@ -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); > > 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) > 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; > } > > /* 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 > -- > 2.25.4 > > _______________________________________________ > 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