On Mon, May 23, 2016 at 9:31 PM, Flavio Leitner <f...@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 <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
>
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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
