[ovs-dev] [PATCH] connmgr: support changing openflow versions without restarting

Flavio Leitner fbl at sysclose.org
Tue Aug 11 19:42:25 UTC 2020


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 at ovn.org>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1782834
> Signed-off-by: Aaron Conole <aconole at 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.

-- 
fbl


More information about the dev mailing list