| Message ID | 20200807213203.796199-1-aconole@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] connmgr: support changing openflow versions without restarting | expand |
Bleep bloop. Greetings Aaron Conole, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 83 characters long (recommended limit is 79)
#47 FILE: ofproto/connmgr.c:608:
VLOG_INFO("%s: restarting controller \"%s\" due to version change",
Lines checked: 123, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Sat, Aug 8, 2020 at 3:02 AM Aaron Conole <aconole@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 <blp@ovn.org> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834 > Signed-off-by: Aaron Conole <aconole@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 <numans@ovn.org> Tested-by: Numan Siddique <numans@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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@ovn.org> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834 > Signed-off-by: Aaron Conole <aconole@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. 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); } 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. > > 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. > 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.
Flavio Leitner <fbl@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@ovn.org> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834 >> Signed-off-by: Aaron Conole <aconole@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.
On Tue, Aug 11, 2020 at 03:58:44PM -0400, Aaron Conole wrote: > Flavio Leitner <fbl@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@ovn.org> > >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834 > >> Signed-off-by: Aaron Conole <aconole@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Flavio Leitner <fbl@sysclose.org> writes: > On Tue, Aug 11, 2020 at 03:58:44PM -0400, Aaron Conole wrote: >> Flavio Leitner <fbl@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@ovn.org> >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834 >> >> Signed-off-by: Aaron Conole <aconole@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@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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
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@ovn.org> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834 Signed-off-by: Aaron Conole <aconole@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(-)