[ovs-dev] [PATCH 3/6] connmgr: Improve interface for setting controllers.

Ben Pfaff blp at ovn.org
Wed Oct 31 23:21:29 UTC 2018


Thanks, I applied this to master.

On Wed, Oct 31, 2018 at 02:49:28PM -0700, Yifeng Sun wrote:
> Thanks for the improvement.
> 
> Tested-by: Yifeng Sun <pkusunyifeng at gmail.com>
> Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>
> 
> On Mon, Oct 29, 2018 at 3:59 PM Ben Pfaff <blp at ovn.org> wrote:
> 
> > Using an shash instead of an array simplifies the code for both the caller
> > and the callee.  Putting the set of allowed OpenFlow versions into the
> > ofproto_controller data structure also simplifies the overall function
> > interface slightly.
> >
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > ---
> >  ofproto/connmgr.c |  51 +++++++++++----------------
> >  ofproto/connmgr.h |   5 ++-
> >  ofproto/ofproto.c |   7 ++--
> >  ofproto/ofproto.h |   7 ++--
> >  vswitchd/bridge.c | 102
> > ++++++++++++++++++++++--------------------------------
> >  5 files changed, 69 insertions(+), 103 deletions(-)
> >
> > diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> > index c7532cf01217..f5576d50524d 100644
> > --- a/ofproto/connmgr.c
> > +++ b/ofproto/connmgr.c
> > @@ -580,9 +580,7 @@ connmgr_free_controller_info(struct shash *info)
> >  /* Changes 'mgr''s set of controllers to the 'n_controllers' controllers
> > in
> >   * 'controllers'. */
> >  void
> > -connmgr_set_controllers(struct connmgr *mgr,
> > -                        const struct ofproto_controller *controllers,
> > -                        size_t n_controllers, uint32_t allowed_versions)
> > +connmgr_set_controllers(struct connmgr *mgr, struct shash *controllers)
> >      OVS_EXCLUDED(ofproto_mutex)
> >  {
> >      bool had_controllers = connmgr_has_controllers(mgr);
> > @@ -591,52 +589,49 @@ connmgr_set_controllers(struct connmgr *mgr,
> >       * cover a smaller amount of code, if that yielded some benefit. */
> >      ovs_mutex_lock(&ofproto_mutex);
> >
> > -    /* Create newly configured controllers and services.
> > -     * Create a name to ofproto_controller mapping in 'new_controllers'.
> > */
> > -    struct shash new_controllers = SHASH_INITIALIZER(&new_controllers);
> > -    for (size_t i = 0; i < n_controllers; i++) {
> > -        const struct ofproto_controller *c = &controllers[i];
> > +    /* Create newly configured controllers and services. */
> > +    struct shash_node *node;
> > +    SHASH_FOR_EACH (node, controllers) {
> > +        const char *target = node->name;
> > +        const struct ofproto_controller *c = node->data;
> >
> > -        if (!vconn_verify_name(c->target)) {
> > +        if (!vconn_verify_name(target)) {
> >              bool add = false;
> > -            struct ofconn *ofconn = find_controller_by_target(mgr,
> > c->target);
> > +            struct ofconn *ofconn = find_controller_by_target(mgr,
> > target);
> >              if (!ofconn) {
> >                  VLOG_INFO("%s: added primary controller \"%s\"",
> > -                          mgr->name, c->target);
> > +                          mgr->name, target);
> >                  add = true;
> >              } else if (rconn_get_allowed_versions(ofconn->rconn) !=
> > -                       allowed_versions) {
> > +                       c->allowed_versions) {
> >                  VLOG_INFO("%s: re-added primary controller \"%s\"",
> > -                          mgr->name, c->target);
> > +                          mgr->name, target);
> >                  add = true;
> >                  ofconn_destroy(ofconn);
> >              }
> >              if (add) {
> > -                add_controller(mgr, c->target, c->dscp, allowed_versions);
> > +                add_controller(mgr, target, c->dscp, c->allowed_versions);
> >              }
> > -        } else if (!pvconn_verify_name(c->target)) {
> > +        } else if (!pvconn_verify_name(target)) {
> >              bool add = false;
> > -            struct ofservice *ofservice = ofservice_lookup(mgr,
> > c->target);
> > +            struct ofservice *ofservice = ofservice_lookup(mgr, target);
> >              if (!ofservice) {
> >                  VLOG_INFO("%s: added service controller \"%s\"",
> > -                          mgr->name, c->target);
> > +                          mgr->name, target);
> >                  add = true;
> > -            } else if (ofservice->allowed_versions != allowed_versions) {
> > +            } else if (ofservice->allowed_versions !=
> > c->allowed_versions) {
> >                  VLOG_INFO("%s: re-added service controller \"%s\"",
> > -                          mgr->name, c->target);
> > +                          mgr->name, target);
> >                  ofservice_destroy(mgr, ofservice);
> >                  add = true;
> >              }
> >              if (add) {
> > -                ofservice_create(mgr, c->target, allowed_versions,
> > c->dscp);
> > +                ofservice_create(mgr, target, c->allowed_versions,
> > c->dscp);
> >              }
> >          } else {
> >              VLOG_WARN_RL(&rl, "%s: unsupported controller \"%s\"",
> > -                         mgr->name, c->target);
> > -            continue;
> > +                         mgr->name, target);
> >          }
> > -
> > -        shash_add_once(&new_controllers, c->target, &controllers[i]);
> >      }
> >
> >      /* Delete controllers that are no longer configured.
> > @@ -644,8 +639,7 @@ connmgr_set_controllers(struct connmgr *mgr,
> >      struct ofconn *ofconn, *next_ofconn;
> >      HMAP_FOR_EACH_SAFE (ofconn, next_ofconn, hmap_node,
> > &mgr->controllers) {
> >          const char *target = ofconn_get_target(ofconn);
> > -        struct ofproto_controller *c = shash_find_data(&new_controllers,
> > -                                                       target);
> > +        struct ofproto_controller *c = shash_find_data(controllers,
> > target);
> >          if (!c) {
> >              VLOG_INFO("%s: removed primary controller \"%s\"",
> >                        mgr->name, target);
> > @@ -660,8 +654,7 @@ connmgr_set_controllers(struct connmgr *mgr,
> >      struct ofservice *ofservice, *next_ofservice;
> >      HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, node, &mgr->services) {
> >          const char *target = pvconn_get_name(ofservice->pvconn);
> > -        struct ofproto_controller *c = shash_find_data(&new_controllers,
> > -                                                       target);
> > +        struct ofproto_controller *c = shash_find_data(controllers,
> > target);
> >          if (!c) {
> >              VLOG_INFO("%s: removed service controller \"%s\"",
> >                        mgr->name, target);
> > @@ -671,8 +664,6 @@ connmgr_set_controllers(struct connmgr *mgr,
> >          }
> >      }
> >
> > -    shash_destroy(&new_controllers);
> > -
> >      ovs_mutex_unlock(&ofproto_mutex);
> >
> >      update_in_band_remotes(mgr);
> > diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
> > index 4a22f1c26611..11c8f9a85121 100644
> > --- a/ofproto/connmgr.h
> > +++ b/ofproto/connmgr.h
> > @@ -56,6 +56,7 @@ enum ofconn_type {
> >      OFCONN_PRIMARY,             /* An ordinary OpenFlow controller. */
> >      OFCONN_SERVICE              /* A service connection, e.g.
> > "ovs-ofctl". */
> >  };
> > +const char *ofconn_type_to_string(enum ofconn_type);
> >
> >  /* An asynchronous message that might need to be queued between threads.
> > */
> >  struct ofproto_async_msg {
> > @@ -94,9 +95,7 @@ void connmgr_retry(struct connmgr *);
> >  bool connmgr_has_controllers(const struct connmgr *);
> >  void connmgr_get_controller_info(struct connmgr *, struct shash *);
> >  void connmgr_free_controller_info(struct shash *);
> > -void connmgr_set_controllers(struct connmgr *,
> > -                             const struct ofproto_controller[], size_t n,
> > -                             uint32_t allowed_versions);
> > +void connmgr_set_controllers(struct connmgr *, struct shash *);
> >  void connmgr_reconnect(const struct connmgr *);
> >
> >  int connmgr_set_snoops(struct connmgr *, const struct sset *snoops);
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 8b2e3ca97a2b..222c749940ec 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -639,12 +639,9 @@ ofproto_set_datapath_id(struct ofproto *p, uint64_t
> > datapath_id)
> >  }
> >
> >  void
> > -ofproto_set_controllers(struct ofproto *p,
> > -                        const struct ofproto_controller *controllers,
> > -                        size_t n_controllers, uint32_t allowed_versions)
> > +ofproto_set_controllers(struct ofproto *p, struct shash *controllers)
> >  {
> > -    connmgr_set_controllers(p->connmgr, controllers, n_controllers,
> > -                            allowed_versions);
> > +    connmgr_set_controllers(p->connmgr, controllers);
> >  }
> >
> >  void
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 3ca88baf018f..595729dd61f1 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -208,11 +208,12 @@ enum ofproto_band {
> >  };
> >
> >  struct ofproto_controller {
> > -    char *target;               /* e.g. "tcp:127.0.0.1" */
> >      int max_backoff;            /* Maximum reconnection backoff, in
> > seconds. */
> >      int probe_interval;         /* Max idle time before probing, in
> > seconds. */
> >      enum ofproto_band band;     /* In-band or out-of-band? */
> >      bool enable_async_msgs;     /* Initially enable asynchronous
> > messages? */
> > +    uint32_t allowed_versions;  /* OpenFlow protocol versions that may
> > +                                 * be negotiated for a session. */
> >
> >      /* OpenFlow packet-in rate-limiting. */
> >      int rate_limit;             /* Max packet-in rate in packets per
> > second. */
> > @@ -304,9 +305,7 @@ int ofproto_port_query_by_name(const struct ofproto *,
> > const char *devname,
> >  /* Top-level configuration. */
> >  uint64_t ofproto_get_datapath_id(const struct ofproto *);
> >  void ofproto_set_datapath_id(struct ofproto *, uint64_t datapath_id);
> > -void ofproto_set_controllers(struct ofproto *,
> > -                             const struct ofproto_controller *, size_t n,
> > -                             uint32_t allowed_versions);
> > +void ofproto_set_controllers(struct ofproto *, struct shash *controllers);
> >  void ofproto_set_fail_mode(struct ofproto *, enum ofproto_fail_mode
> > fail_mode);
> >  void ofproto_reconnect_controllers(struct ofproto *);
> >  void ofproto_set_extra_in_band_remotes(struct ofproto *,
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 9d230b20641c..83708ee51c7a 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -3462,48 +3462,6 @@ bridge_del_ports(struct bridge *br, const struct
> > shash *wanted_ports)
> >      }
> >  }
> >
> > -/* Initializes 'oc' appropriately as a management service controller for
> > - * 'br'.
> > - *
> > - * The caller must free oc->target when it is no longer needed. */
> > -static void
> > -bridge_ofproto_controller_for_mgmt(const struct bridge *br,
> > -                                   struct ofproto_controller *oc)
> > -{
> > -    oc->target = xasprintf("punix:%s/%s.mgmt", ovs_rundir(), br->name);
> > -    oc->max_backoff = 0;
> > -    oc->probe_interval = 60;
> > -    oc->band = OFPROTO_OUT_OF_BAND;
> > -    oc->rate_limit = 0;
> > -    oc->burst_limit = 0;
> > -    oc->enable_async_msgs = true;
> > -    oc->dscp = 0;
> > -}
> > -
> > -/* Converts ovsrec_controller 'c' into an ofproto_controller in 'oc'.  */
> > -static void
> > -bridge_ofproto_controller_from_ovsrec(const struct ovsrec_controller *c,
> > -                                      struct ofproto_controller *oc)
> > -{
> > -    int dscp;
> > -
> > -    oc->target = c->target;
> > -    oc->max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8;
> > -    oc->probe_interval = c->inactivity_probe ? *c->inactivity_probe /
> > 1000 : 5;
> > -    oc->band = (!c->connection_mode || !strcmp(c->connection_mode,
> > "in-band")
> > -                ? OFPROTO_IN_BAND : OFPROTO_OUT_OF_BAND);
> > -    oc->rate_limit = c->controller_rate_limit ? *c->controller_rate_limit
> > : 0;
> > -    oc->burst_limit = (c->controller_burst_limit
> > -                       ? *c->controller_burst_limit : 0);
> > -    oc->enable_async_msgs = (!c->enable_async_messages
> > -                             || *c->enable_async_messages);
> > -    dscp = smap_get_int(&c->other_config, "dscp", DSCP_DEFAULT);
> > -    if (dscp < 0 || dscp > 63) {
> > -        dscp = DSCP_DEFAULT;
> > -    }
> > -    oc->dscp = dscp;
> > -}
> > -
> >  /* Configures the IP stack for 'br''s local interface properly according
> > to the
> >   * configuration in 'c'.  */
> >  static void
> > @@ -3589,10 +3547,6 @@ bridge_configure_remotes(struct bridge *br,
> >
> >      enum ofproto_fail_mode fail_mode;
> >
> > -    struct ofproto_controller *ocs;
> > -    size_t n_ocs;
> > -    size_t i;
> > -
> >      /* Check if we should disable in-band control on this bridge. */
> >      disable_in_band = smap_get_bool(&br->cfg->other_config,
> > "disable-in-band",
> >                                      false);
> > @@ -3611,13 +3565,22 @@ bridge_configure_remotes(struct bridge *br,
> >      n_controllers = (ofproto_get_flow_restore_wait() ? 0
> >                       : bridge_get_controllers(br, &controllers));
> >
> > -    ocs = xmalloc((n_controllers + 1) * sizeof *ocs);
> > -    n_ocs = 0;
> > -
> > -    bridge_ofproto_controller_for_mgmt(br, &ocs[n_ocs++]);
> > -    for (i = 0; i < n_controllers; i++) {
> > +    /* The set of controllers to pass down to ofproto. */
> > +    struct shash ocs = SHASH_INITIALIZER(&ocs);
> > +
> > +    /* Add managment controller. */
> > +    struct ofproto_controller *oc = xmalloc(sizeof *oc);
> > +    *oc = (struct ofproto_controller) {
> > +        .probe_interval = 60,
> > +        .band = OFPROTO_OUT_OF_BAND,
> > +        .enable_async_msgs = true,
> > +        .allowed_versions = bridge_get_allowed_versions(br),
> > +    };
> > +    shash_add_nocopy(
> > +        &ocs, xasprintf("punix:%s/%s.mgmt", ovs_rundir(), br->name), oc);
> > +
> > +    for (size_t i = 0; i < n_controllers; i++) {
> >          struct ovsrec_controller *c = controllers[i];
> > -
> >          if (daemon_should_self_confine()
> >              && (!strncmp(c->target, "punix:", 6)
> >              || !strncmp(c->target, "unix:", 5))) {
> > @@ -3668,17 +3631,34 @@ bridge_configure_remotes(struct bridge *br,
> >          }
> >
> >          bridge_configure_local_iface_netdev(br, c);
> > -        bridge_ofproto_controller_from_ovsrec(c, &ocs[n_ocs]);
> > -        if (disable_in_band) {
> > -            ocs[n_ocs].band = OFPROTO_OUT_OF_BAND;
> > +
> > +        int dscp = smap_get_int(&c->other_config, "dscp", DSCP_DEFAULT);
> > +        if (dscp < 0 || dscp > 63) {
> > +            dscp = DSCP_DEFAULT;
> >          }
> > -        n_ocs++;
> > -    }
> >
> > -    ofproto_set_controllers(br->ofproto, ocs, n_ocs,
> > -                            bridge_get_allowed_versions(br));
> > -    free(ocs[0].target); /* From bridge_ofproto_controller_for_mgmt(). */
> > -    free(ocs);
> > +        oc = xmalloc(sizeof *oc);
> > +        *oc = (struct ofproto_controller) {
> > +            .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8,
> > +            .probe_interval = (c->inactivity_probe
> > +                               ? *c->inactivity_probe / 1000 : 5),
> > +            .band = ((!c->connection_mode
> > +                      || !strcmp(c->connection_mode, "in-band"))
> > +                     && !disable_in_band
> > +                     ? OFPROTO_IN_BAND : OFPROTO_OUT_OF_BAND),
> > +            .enable_async_msgs = (!c->enable_async_messages
> > +                                  || *c->enable_async_messages),
> > +            .allowed_versions = bridge_get_allowed_versions(br),
> > +            .rate_limit = (c->controller_rate_limit
> > +                           ? *c->controller_rate_limit : 0),
> > +            .burst_limit = (c->controller_burst_limit
> > +                            ? *c->controller_burst_limit : 0),
> > +            .dscp = dscp,
> > +        };
> > +        shash_add(&ocs, c->target, oc);
> > +    }
> > +    ofproto_set_controllers(br->ofproto, &ocs);
> > +    shash_destroy_free_data(&ocs);
> >
> >      /* Set the fail-mode. */
> >      fail_mode = !br->cfg->fail_mode
> > --
> > 2.16.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list