On Wed, May 11, 2016 at 10:13:48AM +0800, Xiao Liang wrote:
> 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.
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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
