On Sat, May 14, 2016 at 10:11:32PM +0800, Xiao Liang wrote:
> On Fri, May 13, 2016 at 1:16 AM, Flavio Leitner <f...@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 <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
> >
>
> 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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
