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

Xiao Liang shaw.leon at gmail.com
Tue May 24 07:30:24 UTC 2016


On Mon, May 23, 2016 at 9:31 PM, Flavio Leitner <fbl at sysclose.org> wrote:
> On Sat, May 14, 2016 at 10:11:32PM +0800, Xiao Liang wrote:
>> On Fri, May 13, 2016 at 1:16 AM, Flavio Leitner <fbl at sysclose.org> wrote:
>> > On Wed, May 11, 2016 at 10:13:48AM +0800, Xiao Liang wrote:
>> >> On Wed, May 11, 2016 at 4:31 AM, Flavio Leitner <fbl at sysclose.org> wrote:
>> >> > On Tue, May 10, 2016 at 10:31:19AM +0800, Xiao Liang wrote:
>> >> >> On Tue, May 10, 2016 at 4:28 AM, Flavio Leitner <fbl at sysclose.org> wrote:
>> >> >> > 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?
>> >> >> >
>> >> >> I didn't found whether port name should be unique in openflow spec,
>> >> >> and it didn't specify the behavior of "mod-port by name" either. If
>> >> >> there are duplicate names, current "ovs-ofctl mod-port" will modify
>> >> >> the first port it sees. Do you think this is acceptable or we should
>> >> >> force the name to be unique? (I think this behavior should be
>> >> >> documented in ovs-ofctl)
>> >> >
>> >> > I agree that the behavior should be documented in ovs-ofctl.  I also
>> >> > didn't find anything in the specs but it doesn't seem useful to have
>> >> > two or more ports sharing the same name.  At least not in the same
>> >> > bridge.
>> >> >
>> >> > Also that the code truncates the name, so if someone pass names like:
>> >> > interface_number_1 and interface_number_2, both would result in the
>> >> > same truncated name and that may cause issues as well.
>> >> >
>> >> > Do you know any use-case where duplicated names would be required?
>> >> >
>> >> > --
>> >> > fbl
>> >> >
>> >>
>> >> Consider swapping the names of two interfaces. We have to introduce a
>> >> renaming transaction if don't allow duplicated names.
>> >
>> > You can do something like this:
>> > ovs-vsctl -- \
>> >     -- set Interface eth0 ofname=$tempname \
>> >     -- set Interface eth1 ofname=$eth0_prev_name \
>> >     -- set Interface eth0 ofname=$eth1_prev_name \
>> >
>> >
>> >> There are some other issues: can ofname duplicate the devname of
>> >> another interface?
>> >
>> > It's the same issue of having duplicated ofnames.
>> >
>> >> And what if duplicate names are configured in
>> >> ovsdb?
>> >
>> > That shouldn't be possible and the vswitch could report an
>> > error?
>> >
>> > --
>> > fbl
>> >
>>
>> Hi Flavio,
>>
>> Let me clarify a bit.
>> There are mainly two things: (a) whether devnames and ofnames are in
>> the same namespace, and (b) whether there can be duplicate ofnames.
>> If the answer to (a) is yes, whenever adding an interface or changing
>> the ofname with ovs-vsctl, it must be checked against all existing
>> devname and ofnames. Say if there is an interface "eth0" with ofname
>> set to "eth1", then the physical interface eth1 could not be add. This
>> is probably not desired, because it loses flexibility to a simulate
>> different network environment.
>> For (b), I prefer to allow duplicated name and leave it to users how
>> ofnames are used. In my opinion openflow port name is more like a
>> property than an identifier. There's no problem as long as one doesn't
>> try to identify ports by name while there are duplicate names
>> configured.
>> Does it make sense?
>
> It does. My point is that although you want ofname to be a
> property and not an identifier, it can be used as an identifier
> as well.  When that happens, it exposes internal implementation
> details or show undefined behavior.
>
> Today it works reliable because it is actually the interface name
> which is unique, so I am quite sure there are users relying on that.
>
> However, the spec is clear about the uniqueness of ofport_no and
> the patch doesn't change the default ofname.  We would have to
> assume that anyone using ofname knows what his is doing.
>
> I would ack V4 that includes the other change requested.
>
> Thanks,
> --
> fbl
>

I believe users relying on the interface names won't often config
duplicates, so it shouldn't be a big problem.
I'm sending patch v4, please have a look.

Thanks,
Xiao



More information about the dev mailing list