| Message ID | 20200812200755.13083-1-aconole@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] connmgr: support changing openflow versions without restarting | expand |
On Wed, Aug 12, 2020 at 04:07:55PM -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") > Cc: Numan Siddique <numans@ovn.org> > Cc: Flavio Leitner <fbl@sysclose.org> > 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> > --- Acked-by: Flavio Leitner <fbl@sysclose.org>
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 109 characters long (recommended limit is 79)
#49 FILE: ofproto/connmgr.c:607:
VLOG_INFO("%s: Changes to controller \"%s\" expects re-initialization: Re-initializing now.",
Lines checked: 115, 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 Thu, Aug 13, 2020 at 1:55 AM Flavio Leitner <fbl@sysclose.org> wrote: > > On Wed, Aug 12, 2020 at 04:07:55PM -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") > > Cc: Numan Siddique <numans@ovn.org> > > Cc: Flavio Leitner <fbl@sysclose.org> > > 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> > > --- > > Acked-by: Flavio Leitner <fbl@sysclose.org> Acked-by: Numan Siddique <numans@ovn.org> Tested-by: Numan Siddique <numans@ovn.org> Thanks Numan > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 8/12/20 10:07 PM, 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") > Cc: Numan Siddique <numans@ovn.org> > Cc: Flavio Leitner <fbl@sysclose.org> > 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> > --- > v2: Keep most of the original API. > > NOTE: The log on line 607 will flag the 0-day robot, but I thought > for string searching purposes it's better to keep it all one > line. Thanks, Aaron, Flavio and Numan! This is actually easy to split the long line near the '%s' without loosing any grep-ability. So, I did that and applied to master. backported down to 2.12. Best regards, Ilya Maximets.
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 51d656cba9..d37e9ff1ab 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -190,8 +190,8 @@ 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 int ofservice_reconfigure(struct ofservice *, + const struct ofproto_controller *) OVS_REQUIRES(ofproto_mutex); static void ofservice_create(struct connmgr *mgr, const char *target, const struct ofproto_controller *) @@ -602,7 +602,14 @@ connmgr_set_controllers(struct connmgr *mgr, struct shash *controllers) target); ofservice_destroy(ofservice); } else { - ofservice_reconfigure(ofservice, c); + if (ofservice_reconfigure(ofservice, c)) { + char *target_to_restore = xstrdup(target); + VLOG_INFO("%s: Changes to controller \"%s\" expects re-initialization: Re-initializing now.", + mgr->name, target); + ofservice_destroy(ofservice); + ofservice_create(mgr, target_to_restore, c); + free(target_to_restore); + } } } @@ -2011,16 +2018,15 @@ ofservice_wait(struct ofservice *ofservice) } } -static void +static int ofservice_reconfigure(struct ofservice *ofservice, const struct ofproto_controller *settings) OVS_REQUIRES(ofproto_mutex) { - /* If the allowed OpenFlow versions change, close all of the existing - * connections to allow them to reconnect and possibly negotiate a new - * version. */ + /* If the allowed OpenFlow versions change, a full cleanup is needed + * for the ofservice and connections. */ if (ofservice->s.allowed_versions != settings->allowed_versions) { - ofservice_close_all(ofservice); + return -EINVAL; } ofservice->s = *settings; @@ -2029,6 +2035,8 @@ ofservice_reconfigure(struct ofservice *ofservice, LIST_FOR_EACH (ofconn, ofservice_node, &ofservice->conns) { ofconn_reconfigure(ofconn, settings); } + + return 0; } /* 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") Cc: Numan Siddique <numans@ovn.org> Cc: Flavio Leitner <fbl@sysclose.org> 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> --- v2: Keep most of the original API. NOTE: The log on line 607 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 | 24 ++++++++++++++++-------- tests/bridge.at | 17 +++++++++++++++++ 2 files changed, 33 insertions(+), 8 deletions(-)