On Wed, May 11, 2016 at 4:31 AM, Flavio Leitner <f...@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 <f...@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.l...@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.
There are some other issues: can ofname duplicate the devname of
another interface? And what if duplicate names are configured in
ovsdb?
Thanks,
Xiao
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
