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)
>
>> +
>> netdev_get_flags(netdev, &flags);
>> pp->config = flags & NETDEV_UP ? 0 : OFPUTIL_PC_PORT_DOWN;
>> pp->state = netdev_get_carrier(netdev) ? 0 : OFPUTIL_PS_LINK_DOWN;
>> @@ -8011,3 +8025,52 @@ ofproto_port_set_realdev(struct ofproto *ofproto,
>> ofp_port_t vlandev_ofp_port,
>> }
>> return error;
>> }
>> +
>> +const char *
>> +ofproto_port_get_ofpname(struct ofproto *ofproto, ofp_port_t ofp_port)
>> +{
>> + struct ofport *ofport;
>> +
>> + ofport = ofproto_get_port(ofproto, ofp_port);
>> + return (ofport ? ofport->pp.name : NULL);
>> +}
>> +
>> +void
>> +ofproto_port_set_ofpname(struct ofproto *ofproto, ofp_port_t ofp_port,
>> + const char *ofp_name)
>> +{
>> + struct ofport *ofport;
>> + const char *devname;
>> + size_t name_size;
>> +
>> + ofport = ofproto_get_port(ofproto, ofp_port);
>> + if (!ofport) {
>> + return;
>> + }
>> +
>> + devname = netdev_get_name(ofport->netdev);
>> + name_size = sizeof(ofport->pp.name);
>> +
>> + if (!devname ||
>> + (ofp_name && !strncmp(ofp_name, ofport->pp.name, name_size - 1)) ||
>> + (!ofp_name && !strncmp(devname, ofport->pp.name, name_size - 1))) {
>> + /* No need to change port name. */
>> + return;
>> + }
>> +
>> + /* Send a OFPPR_DELETE followed by OFPPR_ADD port_status message
>> + * to notify controller a port name change. */
>> + connmgr_send_port_status(ofproto->connmgr, NULL, &ofport->pp,
>> + OFPPR_DELETE);
>> +
>> + if (!ofp_name) {
>> + smap_remove(&ofproto->ofp_names, devname);
>> + ovs_strlcpy(ofport->pp.name, devname, name_size);
>> + } else {
>> + smap_replace(&ofproto->ofp_names, netdev_get_name(ofport->netdev),
>> + ofp_name);
>> + ovs_strlcpy(ofport->pp.name, ofp_name, name_size);
>> + }
>> + connmgr_send_port_status(ofproto->connmgr, NULL, &ofport->pp,
>> + OFPPR_ADD);
>> +}
>> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
>> index 2d241c9..3382ef1 100644
>> --- a/ofproto/ofproto.h
>> +++ b/ofproto/ofproto.h
>> @@ -288,7 +288,8 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
>>
>> const char *ofproto_port_open_type(const char *datapath_type,
>> const char *port_type);
>> -int ofproto_port_add(struct ofproto *, struct netdev *, ofp_port_t
>> *ofp_portp);
>> +int ofproto_port_add(struct ofproto *, struct netdev *, ofp_port_t
>> *ofp_portp,
>> + const char *);
>> int ofproto_port_del(struct ofproto *, ofp_port_t ofp_port);
>> int ofproto_port_get_stats(const struct ofport *, struct netdev_stats
>> *stats);
>>
>> @@ -521,6 +522,12 @@ int ofproto_port_set_realdev(struct ofproto *,
>> ofp_port_t vlandev_ofp_port,
>> enum ofputil_table_miss ofproto_table_get_miss_config(const struct ofproto
>> *,
>> uint8_t table_id);
>>
>> +const char *ofproto_port_get_ofpname(struct ofproto *ofproto,
>> + ofp_port_t ofp_port);
>> +void ofproto_port_set_ofpname(struct ofproto *ofproto,
>> + ofp_port_t ofp_port,
>> + const char *ofp_name);
>> +
>
> Looks like those belong to the "Configuration of ports" section.
>
Sure, will move this up.
Thanks,
Xiao
>
>> #ifdef __cplusplus
>> }
>> #endif
>> diff --git a/tests/ofproto.at b/tests/ofproto.at
>> index c4260ab..5f38160 100644
>> --- a/tests/ofproto.at
>> +++ b/tests/ofproto.at
>> @@ -77,6 +77,66 @@ OFPT_GET_CONFIG_REPLY: frags=normal miss_send_len=0
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> +AT_SETUP([ofproto - set OpenFlow port name])
>> +OVS_VSWITCHD_START(
>> + [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 --\
>> + add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2
>> ofname=n2])
>> +check_ofname () {
>> + AT_CHECK([ovs-ofctl -vwarn show br0], [0], [stdout])
>> + echo >expout "OFPT_FEATURES_REPLY: dpid:fedcba9876543210
>> +n_tables:254, n_buffers:256
>> +capabilities: FLOW_STATS TABLE_STATS PORT_STATS QUEUE_STATS ARP_MATCH_IP
>> +actions: output enqueue set_vlan_vid set_vlan_pcp strip_vlan mod_dl_src
>> mod_dl_dst mod_nw_src mod_nw_dst mod_nw_tos mod_tp_src mod_tp_dst
>> + 1($1): addr:aa:55:aa:55:00:0x
>> + config: PORT_DOWN
>> + state: LINK_DOWN
>> + speed: 0 Mbps now, 0 Mbps max
>> + 2(n2): addr:aa:55:aa:55:00:0x
>> + config: PORT_DOWN
>> + state: LINK_DOWN
>> + speed: 0 Mbps now, 0 Mbps max
>> + LOCAL(br0): addr:aa:55:aa:55:00:0x
>> + config: PORT_DOWN
>> + state: LINK_DOWN
>> + speed: 0 Mbps now, 0 Mbps max
>> +OFPT_GET_CONFIG_REPLY: frags=normal miss_send_len=0"
>> + AT_CHECK([[sed '
>> +s/ (xid=0x[0-9a-fA-F]*)//
>> +s/00:0.$/00:0x/' < stdout]],
>> + [0], [expout])
>> +}
>> +
>> +AT_CHECK([ovs-vsctl set Interface p1 ofname=p2])
>> +check_ofname p2
>> +
>> +AT_CHECK([ovs-ofctl -P standard monitor br0 --detach --no-chdir --pidfile])
>> +# Use NXT_SET_ASYNC_CONFIG to enable only port status message
>> +ovs-appctl -t ovs-ofctl ofctl/send
>> 01040028000000020000232000000013000000000000000500000007000000020000000000000005
>> +ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
>> +AT_CHECK([ovs-vsctl set Interface p1 ofname=n2])
>> +check_ofname n2
>> +OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
>> +AT_CHECK([[sed '
>> +s/ (xid=0x[0-9a-fA-F]*)//
>> +s/ *duration.*//
>> +s/00:0.$/00:0x/' < monitor.log]],
>> + [0], [dnl
>> +OFPT_PORT_STATUS: DEL: 1(p2): addr:aa:55:aa:55:00:0x
>> + config: PORT_DOWN
>> + state: LINK_DOWN
>> + speed: 0 Mbps now, 0 Mbps max
>> +OFPT_PORT_STATUS: ADD: 1(n2): addr:aa:55:aa:55:00:0x
>> + config: PORT_DOWN
>> + state: LINK_DOWN
>> + speed: 0 Mbps now, 0 Mbps max
>> +])
>> +
>> +AT_CHECK([ovs-vsctl clear Interface p1 ofname])
>> +check_ofname p1
>> +
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>> dnl This is really bare-bones.
>> dnl It at least checks request and reply serialization and deserialization.
>> AT_SETUP([ofproto - port stats - (OpenFlow 1.0)])
>> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
>> index c9c0f6d..44b386a 100644
>> --- a/utilities/ovs-vsctl.c
>> +++ b/utilities/ovs-vsctl.c
>> @@ -1008,6 +1008,7 @@ static struct cmd_show_table cmd_show_tables[] = {
>> {&ovsrec_table_interface,
>> &ovsrec_interface_col_name,
>> {&ovsrec_interface_col_type,
>> + &ovsrec_interface_col_ofname,
>> &ovsrec_interface_col_options,
>> &ovsrec_interface_col_error},
>> {NULL, NULL, NULL}
>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>> index 700f65c..bb49fe0 100644
>> --- a/vswitchd/bridge.c
>> +++ b/vswitchd/bridge.c
>> @@ -667,6 +667,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
>> *ovs_cfg)
>>
>> LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
>> iface_set_ofport(iface->cfg, iface->ofp_port);
>> +
>> + ofproto_port_set_ofpname(br->ofproto, iface->ofp_port,
>> + iface->cfg->ofname);
>> +
>> /* Clear eventual previous errors */
>> ovsrec_interface_set_error(iface->cfg, NULL);
>> iface_configure_cfm(iface);
>> @@ -1777,7 +1781,8 @@ iface_do_create(const struct bridge *br,
>> }
>>
>> *ofp_portp = iface_pick_ofport(iface_cfg);
>> - error = ofproto_port_add(br->ofproto, netdev, ofp_portp);
>> + error = ofproto_port_add(br->ofproto, netdev, ofp_portp,
>> + iface_cfg->ofname);
>> if (error) {
>> goto error;
>> }
>> @@ -1860,7 +1865,8 @@ iface_create(struct bridge *br, const struct
>> ovsrec_interface *iface_cfg,
>> error = netdev_open(port->name, "internal", &netdev);
>> if (!error) {
>> ofp_port_t fake_ofp_port = OFPP_NONE;
>> - ofproto_port_add(br->ofproto, netdev, &fake_ofp_port);
>> + ofproto_port_add(br->ofproto, netdev, &fake_ofp_port,
>> + iface_cfg->ofname);
>> netdev_close(netdev);
>> } else {
>> VLOG_WARN("could not open network device %s (%s)",
>> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
>> index e0937f4..d147d04 100644
>> --- a/vswitchd/vswitch.ovsschema
>> +++ b/vswitchd/vswitch.ovsschema
>> @@ -1,6 +1,6 @@
>> {"name": "Open_vSwitch",
>> - "version": "7.13.0",
>> - "cksum": "2202834738 22577",
>> + "version": "7.13.1",
>> + "cksum": "643309718 22653",
>> "tables": {
>> "Open_vSwitch": {
>> "columns": {
>> @@ -204,6 +204,8 @@
>> "mutable": false},
>> "type": {
>> "type": "string"},
>> + "ofname": {
>> + "type": {"key": "string", "min": 0, "max": 1}},
>> "options": {
>> "type": {"key": "string", "value": "string", "min": 0, "max":
>> "unlimited"}},
>> "ingress_policing_rate": {
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index 166f264..1869c81 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -1724,6 +1724,20 @@
>> on a host.
>> </column>
>>
>> +
>> + <column name="ofname">
>> + <p>
>> + OpenFlow port name for this interface. This name is truncated to
>> + <code>OFP_MAX_PORT_NAME_LEN-1</code>, and reported to controllers
>> in
>> + port description. If not set, <ref column='name'/> is used.
>> + </p>
>> + <p>
>> + OpenFlow does not have a way to announce a port name change, so if
>> + <ref column="ofname"/> is changed, Open vSwitch represents it over
>> + OpenFlow as a port deletion followed immediately by a port
>> addition.
>> + </p>
>> + </column>
>> +
>> <column name="ifindex">
>> A positive interface index as defined for SNMP MIB-II in RFCs 1213
>> and
>> 2863, if the interface has one, otherwise 0. The ifindex is useful
>> for
>> --
>> 2.8.0
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
> --
> fbl
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
