[ovs-dev] [PATCH v3] Add configurable OpenFlow port name.

Flavio Leitner fbl at sysclose.org
Mon May 9 20:28:16 UTC 2016


On Sat, Apr 23, 2016 at 01:26:17PM +0800, Xiao Liang wrote:
> Add new column "ofname" in Interface table to configure port name reported
> to controllers with OpenFlow protocol, thus decouple OpenFlow port name from
> device name.
> 
> For example:
>     # ovs-vsctl set Interface eth0 ofname=wan
>     # ovs-vsctl set Interface eth1 ofname=lan0
> then controllers can recognize ports by their names.

This change is nice because now the same setup like a "compute node"
can use the same logical name to refer to a specific interface that
could have different netdev name on different HW.

Comments inline.

> Signed-off-by: Xiao Liang <shaw.leon at gmail.com>
> ---
> v2: Added test for ofname
>     Increased db schema version
>     Updated NEWS
> v3: Rebase
> ---
>  NEWS                       |  1 +
>  lib/db-ctl-base.h          |  2 +-
>  ofproto/ofproto-provider.h |  1 +
>  ofproto/ofproto.c          | 67 ++++++++++++++++++++++++++++++++++++++++++++--
>  ofproto/ofproto.h          |  9 ++++++-
>  tests/ofproto.at           | 60 +++++++++++++++++++++++++++++++++++++++++
>  utilities/ovs-vsctl.c      |  1 +
>  vswitchd/bridge.c          | 10 +++++--
>  vswitchd/vswitch.ovsschema |  6 +++--
>  vswitchd/vswitch.xml       | 14 ++++++++++
>  10 files changed, 163 insertions(+), 8 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index ea7f3a1..156781c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -15,6 +15,7 @@ Post-v2.5.0
>         now implemented.  Only flow mod and port mod messages are supported
>         in bundles.
>       * New OpenFlow extension NXM_NX_MPLS_TTL to provide access to MPLS TTL.
> +     * Port name can now be set with "ofname" column in the Interface table.
>     - ovs-ofctl:
>       * queue-get-config command now allows a queue ID to be specified.
>       * '--bundle' option can now be used with OpenFlow 1.3.
> diff --git a/lib/db-ctl-base.h b/lib/db-ctl-base.h
> index f8f576b..5bd62d5 100644
> --- a/lib/db-ctl-base.h
> +++ b/lib/db-ctl-base.h
> @@ -177,7 +177,7 @@ struct weak_ref_table {
>  struct cmd_show_table {
>      const struct ovsdb_idl_table_class *table;
>      const struct ovsdb_idl_column *name_column;
> -    const struct ovsdb_idl_column *columns[3]; /* Seems like a good number. */
> +    const struct ovsdb_idl_column *columns[4]; /* Seems like a good number. */
>      const struct weak_ref_table wref_table;
>  };
>  
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index daa0077..8795242 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -84,6 +84,7 @@ struct ofproto {
>      struct hmap ports;          /* Contains "struct ofport"s. */
>      struct shash port_by_name;
>      struct simap ofp_requests;  /* OpenFlow port number requests. */
> +    struct smap ofp_names;      /* OpenFlow port names. */
>      uint16_t alloc_port_no;     /* Last allocated OpenFlow port number. */
>      uint16_t max_ports;         /* Max possible OpenFlow port num, plus one. */
>      struct hmap ofport_usage;   /* Map ofport to last used time. */
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index ff6affd..a2799f4 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -550,6 +550,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
>      hmap_init(&ofproto->ofport_usage);
>      shash_init(&ofproto->port_by_name);
>      simap_init(&ofproto->ofp_requests);
> +    smap_init(&ofproto->ofp_names);
>      ofproto->max_ports = ofp_to_u16(OFPP_MAX);
>      ofproto->eviction_group_timer = LLONG_MIN;
>      ofproto->tables = NULL;
> @@ -1546,6 +1547,7 @@ ofproto_destroy__(struct ofproto *ofproto)
>      hmap_destroy(&ofproto->ofport_usage);
>      shash_destroy(&ofproto->port_by_name);
>      simap_destroy(&ofproto->ofp_requests);
> +    smap_destroy(&ofproto->ofp_names);
>  
>      OFPROTO_FOR_EACH_TABLE (table, ofproto) {
>          oftable_destroy(table);
> @@ -1945,7 +1947,7 @@ ofproto_port_open_type(const char *datapath_type, const char *port_type)
>   * 'ofp_portp' is non-null). */
>  int
>  ofproto_port_add(struct ofproto *ofproto, struct netdev *netdev,
> -                 ofp_port_t *ofp_portp)
> +                 ofp_port_t *ofp_portp, const char *ofp_name)
>  {
>      ofp_port_t ofp_port = ofp_portp ? *ofp_portp : OFPP_NONE;
>      int error;
> @@ -1956,6 +1958,9 @@ ofproto_port_add(struct ofproto *ofproto, struct netdev *netdev,
>  
>          simap_put(&ofproto->ofp_requests, netdev_name,
>                    ofp_to_u16(ofp_port));
> +        if (ofp_name) {
> +            smap_replace(&ofproto->ofp_names, netdev_name, ofp_name);

Should it be unique?  See below.

> +        }
>          error = update_port(ofproto, netdev_name);
>      }
>      if (ofp_portp) {
> @@ -2009,6 +2014,8 @@ ofproto_port_del(struct ofproto *ofproto, ofp_port_t ofp_port)
>          simap_delete(&ofproto->ofp_requests, ofp_request_node);
>      }
>  
> +    smap_remove(&ofproto->ofp_names, name);
> +
>      error = ofproto->ofproto_class->port_del(ofproto, ofp_port);
>      if (!error && ofport) {
>          /* 'name' is the netdev's name and update_port() is going to close the
> @@ -2285,6 +2292,7 @@ ofport_open(struct ofproto *ofproto,
>  {
>      enum netdev_flags flags;
>      struct netdev *netdev;
> +    const char *ofp_name;
>      int error;
>  
>      error = netdev_open(ofproto_port->name, ofproto_port->type, &netdev);
> @@ -2307,7 +2315,13 @@ ofport_open(struct ofproto *ofproto,
>      }
>      pp->port_no = ofproto_port->ofp_port;
>      netdev_get_etheraddr(netdev, &pp->hw_addr);
> -    ovs_strlcpy(pp->name, ofproto_port->name, sizeof pp->name);
> +
> +    ofp_name = smap_get(&ofproto->ofp_names, ofproto_port->name);
> +    if (!ofp_name) {
> +        ofp_name = ofproto_port->name;
> +    }
> +    ovs_strlcpy(pp->name, ofp_name, sizeof pp->name);

Before pp->name was the device's name which is unique on the system.
Now pp->name can change to some other random string which ovs-ofctl
mod-port will use it.  The new name also needs to be unique, right?


> +
>      netdev_get_flags(netdev, &flags);
>      pp->config = flags & NETDEV_UP ? 0 : OFPUTIL_PC_PORT_DOWN;
>      pp->state = netdev_get_carrier(netdev) ? 0 : OFPUTIL_PS_LINK_DOWN;
> @@ -8011,3 +8025,52 @@ ofproto_port_set_realdev(struct ofproto *ofproto, ofp_port_t vlandev_ofp_port,
>      }
>      return error;
>  }
> +
> +const char *
> +ofproto_port_get_ofpname(struct ofproto *ofproto, ofp_port_t ofp_port)
> +{
> +    struct ofport *ofport;
> +
> +    ofport = ofproto_get_port(ofproto, ofp_port);
> +    return (ofport ? ofport->pp.name : NULL);
> +}
> +
> +void
> +ofproto_port_set_ofpname(struct ofproto *ofproto, ofp_port_t ofp_port,
> +                         const char *ofp_name)
> +{
> +    struct ofport *ofport;
> +    const char *devname;
> +    size_t name_size;
> +
> +    ofport = ofproto_get_port(ofproto, ofp_port);
> +    if (!ofport) {
> +        return;
> +    }
> +
> +    devname = netdev_get_name(ofport->netdev);
> +    name_size = sizeof(ofport->pp.name);
> +
> +    if (!devname ||
> +        (ofp_name && !strncmp(ofp_name, ofport->pp.name, name_size - 1)) ||
> +        (!ofp_name && !strncmp(devname, ofport->pp.name, name_size - 1))) {
> +        /* No need to change port name. */
> +        return;
> +    }
> +
> +    /* Send a OFPPR_DELETE followed by OFPPR_ADD port_status message
> +     * to notify controller a port name change. */
> +    connmgr_send_port_status(ofproto->connmgr, NULL, &ofport->pp,
> +                             OFPPR_DELETE);
> +
> +    if (!ofp_name) {
> +        smap_remove(&ofproto->ofp_names, devname);
> +        ovs_strlcpy(ofport->pp.name, devname, name_size);
> +    } else {
> +        smap_replace(&ofproto->ofp_names, netdev_get_name(ofport->netdev),
> +                 ofp_name);
> +        ovs_strlcpy(ofport->pp.name, ofp_name, name_size);
> +    }
> +    connmgr_send_port_status(ofproto->connmgr, NULL, &ofport->pp,
> +                             OFPPR_ADD);
> +}
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 2d241c9..3382ef1 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -288,7 +288,8 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
>  
>  const char *ofproto_port_open_type(const char *datapath_type,
>                                     const char *port_type);
> -int ofproto_port_add(struct ofproto *, struct netdev *, ofp_port_t *ofp_portp);
> +int ofproto_port_add(struct ofproto *, struct netdev *, ofp_port_t *ofp_portp,
> +                     const char *);
>  int ofproto_port_del(struct ofproto *, ofp_port_t ofp_port);
>  int ofproto_port_get_stats(const struct ofport *, struct netdev_stats *stats);
>  
> @@ -521,6 +522,12 @@ int ofproto_port_set_realdev(struct ofproto *, ofp_port_t vlandev_ofp_port,
>  enum ofputil_table_miss ofproto_table_get_miss_config(const struct ofproto *,
>                                                        uint8_t table_id);
>  
> +const char *ofproto_port_get_ofpname(struct ofproto *ofproto,
> +                                     ofp_port_t ofp_port);
> +void ofproto_port_set_ofpname(struct ofproto *ofproto,
> +                              ofp_port_t ofp_port,
> +                              const char *ofp_name);
> +

Looks like those belong to the "Configuration of ports" section.


>  #ifdef  __cplusplus
>  }
>  #endif
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index c4260ab..5f38160 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -77,6 +77,66 @@ OFPT_GET_CONFIG_REPLY: frags=normal miss_send_len=0
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +AT_SETUP([ofproto - set OpenFlow port name])
> +OVS_VSWITCHD_START(
> +       [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 --\
> +        add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2 ofname=n2])
> +check_ofname () {
> +    AT_CHECK([ovs-ofctl -vwarn show br0], [0], [stdout])
> +    echo >expout "OFPT_FEATURES_REPLY: dpid:fedcba9876543210
> +n_tables:254, n_buffers:256
> +capabilities: FLOW_STATS TABLE_STATS PORT_STATS QUEUE_STATS ARP_MATCH_IP
> +actions: output enqueue set_vlan_vid set_vlan_pcp strip_vlan mod_dl_src mod_dl_dst mod_nw_src mod_nw_dst mod_nw_tos mod_tp_src mod_tp_dst
> + 1($1): addr:aa:55:aa:55:00:0x
> +     config:     PORT_DOWN
> +     state:      LINK_DOWN
> +     speed: 0 Mbps now, 0 Mbps max
> + 2(n2): addr:aa:55:aa:55:00:0x
> +     config:     PORT_DOWN
> +     state:      LINK_DOWN
> +     speed: 0 Mbps now, 0 Mbps max
> + LOCAL(br0): addr:aa:55:aa:55:00:0x
> +     config:     PORT_DOWN
> +     state:      LINK_DOWN
> +     speed: 0 Mbps now, 0 Mbps max
> +OFPT_GET_CONFIG_REPLY: frags=normal miss_send_len=0"
> +    AT_CHECK([[sed '
> +s/ (xid=0x[0-9a-fA-F]*)//
> +s/00:0.$/00:0x/' < stdout]],
> +      [0], [expout])
> +}
> +
> +AT_CHECK([ovs-vsctl set Interface p1 ofname=p2])
> +check_ofname p2
> +
> +AT_CHECK([ovs-ofctl -P standard monitor br0 --detach --no-chdir --pidfile])
> +# Use NXT_SET_ASYNC_CONFIG to enable only port status message
> +ovs-appctl -t ovs-ofctl ofctl/send 01040028000000020000232000000013000000000000000500000007000000020000000000000005
> +ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
> +AT_CHECK([ovs-vsctl set Interface p1 ofname=n2])
> +check_ofname n2
> +OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
> +AT_CHECK([[sed '
> +s/ (xid=0x[0-9a-fA-F]*)//
> +s/ *duration.*//
> +s/00:0.$/00:0x/' < monitor.log]],
> +      [0], [dnl
> +OFPT_PORT_STATUS: DEL: 1(p2): addr:aa:55:aa:55:00:0x
> +     config:     PORT_DOWN
> +     state:      LINK_DOWN
> +     speed: 0 Mbps now, 0 Mbps max
> +OFPT_PORT_STATUS: ADD: 1(n2): addr:aa:55:aa:55:00:0x
> +     config:     PORT_DOWN
> +     state:      LINK_DOWN
> +     speed: 0 Mbps now, 0 Mbps max
> +])
> +
> +AT_CHECK([ovs-vsctl clear Interface p1 ofname])
> +check_ofname p1
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  dnl This is really bare-bones.
>  dnl It at least checks request and reply serialization and deserialization.
>  AT_SETUP([ofproto - port stats - (OpenFlow 1.0)])
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index c9c0f6d..44b386a 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -1008,6 +1008,7 @@ static struct cmd_show_table cmd_show_tables[] = {
>      {&ovsrec_table_interface,
>       &ovsrec_interface_col_name,
>       {&ovsrec_interface_col_type,
> +      &ovsrec_interface_col_ofname,
>        &ovsrec_interface_col_options,
>        &ovsrec_interface_col_error},
>       {NULL, NULL, NULL}
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 700f65c..bb49fe0 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -667,6 +667,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>  
>              LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
>                  iface_set_ofport(iface->cfg, iface->ofp_port);
> +
> +                ofproto_port_set_ofpname(br->ofproto, iface->ofp_port,
> +                                         iface->cfg->ofname);
> +
>                  /* Clear eventual previous errors */
>                  ovsrec_interface_set_error(iface->cfg, NULL);
>                  iface_configure_cfm(iface);
> @@ -1777,7 +1781,8 @@ iface_do_create(const struct bridge *br,
>      }
>  
>      *ofp_portp = iface_pick_ofport(iface_cfg);
> -    error = ofproto_port_add(br->ofproto, netdev, ofp_portp);
> +    error = ofproto_port_add(br->ofproto, netdev, ofp_portp,
> +                             iface_cfg->ofname);
>      if (error) {
>          goto error;
>      }
> @@ -1860,7 +1865,8 @@ iface_create(struct bridge *br, const struct ovsrec_interface *iface_cfg,
>              error = netdev_open(port->name, "internal", &netdev);
>              if (!error) {
>                  ofp_port_t fake_ofp_port = OFPP_NONE;
> -                ofproto_port_add(br->ofproto, netdev, &fake_ofp_port);
> +                ofproto_port_add(br->ofproto, netdev, &fake_ofp_port,
> +                                 iface_cfg->ofname);
>                  netdev_close(netdev);
>              } else {
>                  VLOG_WARN("could not open network device %s (%s)",
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index e0937f4..d147d04 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
>  {"name": "Open_vSwitch",
> - "version": "7.13.0",
> - "cksum": "2202834738 22577",
> + "version": "7.13.1",
> + "cksum": "643309718 22653",
>   "tables": {
>     "Open_vSwitch": {
>       "columns": {
> @@ -204,6 +204,8 @@
>           "mutable": false},
>         "type": {
>           "type": "string"},
> +       "ofname": {
> +         "type": {"key": "string", "min": 0, "max": 1}},
>         "options": {
>           "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}},
>         "ingress_policing_rate": {
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 166f264..1869c81 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1724,6 +1724,20 @@
>          on a host.
>        </column>
>  
> +
> +      <column name="ofname">
> +        <p>
> +          OpenFlow port name for this interface. This name is truncated to
> +          <code>OFP_MAX_PORT_NAME_LEN-1</code>, and reported to controllers in
> +          port description. If not set, <ref column='name'/> is used.
> +        </p>
> +        <p>
> +          OpenFlow does not have a way to announce a port name change, so if
> +          <ref column="ofname"/> is changed, Open vSwitch represents it over
> +          OpenFlow as a port deletion followed immediately by a port addition.
> +        </p>
> +      </column>
> +
>        <column name="ifindex">
>          A positive interface index as defined for SNMP MIB-II in RFCs 1213 and
>          2863, if the interface has one, otherwise 0.  The ifindex is useful for
> -- 
> 2.8.0
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

-- 
fbl




More information about the dev mailing list