| Message ID | 1461389177-14344-1-git-send-email-shaw.leon@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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.leon@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? > + > 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. > #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
On Tue, May 10, 2016 at 4:28 AM, Flavio Leitner <fbl@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.leon@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 >
On Tue, May 10, 2016 at 10:31:19AM +0800, Xiao Liang wrote: > On Tue, May 10, 2016 at 4:28 AM, Flavio Leitner <fbl@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.leon@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?
On Wed, May 11, 2016 at 4:31 AM, Flavio Leitner <fbl@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 <fbl@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.leon@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
On Wed, May 11, 2016 at 10:13:48AM +0800, Xiao Liang wrote: > On Wed, May 11, 2016 at 4:31 AM, Flavio Leitner <fbl@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 <fbl@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.leon@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?
On Fri, May 13, 2016 at 1:16 AM, Flavio Leitner <fbl@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 <fbl@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 <fbl@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.leon@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? Thanks, Xiao
On Sat, May 14, 2016 at 10:11:32PM +0800, Xiao Liang wrote: > On Fri, May 13, 2016 at 1:16 AM, Flavio Leitner <fbl@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 <fbl@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 <fbl@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.leon@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,
On Mon, May 23, 2016 at 9:31 PM, Flavio Leitner <fbl@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 <fbl@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 <fbl@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 <fbl@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.leon@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
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); + } 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); + 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); + #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
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. Signed-off-by: Xiao Liang <shaw.leon@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(-)