On Dec 10, 2013, at 11:20 PM, Ben Pfaff <b...@nicira.com> wrote:
> Feature #21293.
> Requested-by: Anuprem Chalvadi <achalv...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> NEWS | 3 ++
> vswitchd/bridge.c | 93 ++++++++++++++++++++++++++++++++++++++++++++------
> vswitchd/vswitch.xml | 21 +++++++-----
> 3 files changed, 99 insertions(+), 18 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 8556083..ce968f2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -14,6 +14,9 @@ Post-v2.0.0
> * OVS limits the OpenFlow port numbers it assigns to port 32767 and
> below, leaving port numbers above that range free for assignment
> by the controller.
> + * ovs-vswitchd now honors changes to the "ofport_request" column
> + in the Interface table by changing the port's OpenFlow port
> + number.
> - ovs-vswitchd.conf.db.5 man page will contain graphviz/dot
> diagram only if graphviz package was installed at the build time.
> - Support for Linux kernels up to 3.11
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 581f87c..131b800 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -75,6 +75,7 @@ struct iface {
> char *name; /* Host network device name. */
> struct netdev *netdev; /* Network device. */
> ofp_port_t ofp_port; /* OpenFlow port number. */
> + ofp_port_t requested_ofp_port; /* Port number requested previously. */
>
> /* These members are valid only within bridge_reconfigure(). */
> const char *type; /* Usually same as cfg->type. */
> @@ -247,6 +248,8 @@ static void iface_refresh_cfm_stats(struct iface *);
> static void iface_refresh_stats(struct iface *);
> static void iface_refresh_status(struct iface *);
> static bool iface_is_synthetic(const struct iface *);
> +static ofp_port_t iface_get_requested_ofp_port(
> + const struct ovsrec_interface *);
> static ofp_port_t iface_pick_ofport(const struct ovsrec_interface *);
>
> /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
> @@ -636,6 +639,7 @@ bridge_delete_ports(struct bridge *br)
> n = allocated = 0;
>
> OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
> + ofp_port_t requested_ofp_port;
> struct iface *iface;
>
> iface = iface_lookup(br, ofproto_port.name);
> @@ -657,6 +661,43 @@ bridge_delete_ports(struct bridge *br)
> goto delete;
> }
>
> + /* If the requested OpenFlow port for 'iface' changed, and it's not
> + * already the correct port, then we might want to temporarily delete
> + * this interface, so we can add it back again with the new OpenFlow
> + * port number. */
> + requested_ofp_port = iface_get_requested_ofp_port(iface->cfg);
> + if (requested_ofp_port != iface->requested_ofp_port &&
> + requested_ofp_port != OFPP_NONE &&
> + requested_ofp_port != iface->ofp_port) {
It would seem that this test should fail if the face requested a port number
that it did not get (for any reason), and is still requesting the same port
number, but the offending other port was deleted and the (originally) requested
port number would now be available. However, when testing this, this seems to
work:
- create ports p1, p2
- request port number 10 for p1 -> p1 changes to 10
- request port number 10 for p2 -> p2 does not change
- request port number 11 for p1 -> p1 changes to 11 AND p2 changes to 10
Is it so that the requested port number is not stored unless it is also gotten?
Maybe this is intended behavior to avoid surprisingly changing port numbers,
even if the change would be towards the wanted configuration?
> + ofp_port_t victim_request;
> + struct iface *victim;
> +
> + /* Check for an existing OpenFlow port currently occupying
> + * 'iface''s requested port number. If there isn't one, then
> + * delete this port. Otherwise we need to consider further. */
> + victim = iface_from_ofp_port(br, requested_ofp_port);
> + if (!victim) {
> + goto delete;
> + }
> +
> + /* 'victim' is a port currently using 'iface''s requested port
> + * number. Unless 'victim' specifically requested that port
> + * number, too, then we can delete both 'iface' and 'victim'
> + * temporarily. (We'll add both of them back again later with
> new
> + * OpenFlow port numbers.)
> + *
> + * If 'victim' did request port number 'requested_ofp_port', just
> + * like 'iface', then that's a configuration inconsistency that
> we
> + * can't resolve. We might as well let it keep its current port
> + * number. */
> + victim_request = iface_get_requested_ofp_port(victim->cfg);
> + if (victim_request != requested_ofp_port) {
> + del = add_ofp_port(victim->ofp_port, del, &n, &allocated);
> + iface_destroy(victim);
> + goto delete;
> + }
> + }
> +
> continue;
>
> delete:
> @@ -671,7 +712,8 @@ bridge_delete_ports(struct bridge *br)
> }
>
> static void
> -bridge_add_ports(struct bridge *br, const struct shash *wanted_ports)
> +bridge_add_ports__(struct bridge *br, const struct shash *wanted_ports,
> + bool with_requested_port)
> {
> struct shash_node *port_node;
>
> @@ -681,16 +723,33 @@ bridge_add_ports(struct bridge *br, const struct shash
> *wanted_ports)
>
> for (i = 0; i < port_cfg->n_interfaces; i++) {
> const struct ovsrec_interface *iface_cfg =
> port_cfg->interfaces[i];
> - struct iface *iface = iface_lookup(br, iface_cfg->name);
> + ofp_port_t requested_ofp_port;
> +
> + requested_ofp_port = iface_get_requested_ofp_port(iface_cfg);
> + if ((requested_ofp_port != OFPP_NONE) == with_requested_port) {
> + struct iface *iface = iface_lookup(br, iface_cfg->name);
>
> - if (!iface) {
> - iface_create(br, iface_cfg, port_cfg);
> + if (!iface) {
> + iface_create(br, iface_cfg, port_cfg);
> + }
> }
> }
> }
> }
>
> static void
> +bridge_add_ports(struct bridge *br, const struct shash *wanted_ports)
> +{
> + /* First add interfaces that request a particular port number. */
> + bridge_add_ports__(br, wanted_ports, true);
> +
> + /* Then add interfaces that want automatic port number assignment.
> + * We add these afterward to avoid accidentally taking a specifically
> + * requested port number. */
> + bridge_add_ports__(br, wanted_ports, false);
> +}
> +
> +static void
> port_configure(struct port *port)
> {
> const struct ovsrec_port *cfg = port->cfg;
> @@ -1416,6 +1475,7 @@ iface_create(struct bridge *br, const struct
> ovsrec_interface *iface_cfg,
> iface->port = port;
> iface->name = xstrdup(iface_cfg->name);
> iface->ofp_port = ofp_port;
> + iface->requested_ofp_port = iface_get_requested_ofp_port(iface_cfg);
> iface->netdev = netdev;
> iface->type = iface_get_type(iface_cfg, br->cfg);
> iface->cfg = iface_cfg;
> @@ -1439,7 +1499,7 @@ 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 = iface_pick_ofport(iface_cfg);
> + ofp_port_t fake_ofp_port = OFPP_NONE;
> ofproto_port_add(br->ofproto, netdev, &fake_ofp_port);
> netdev_close(netdev);
> } else {
> @@ -3566,14 +3626,27 @@ iface_is_synthetic(const struct iface *iface)
> }
>
> static ofp_port_t
> -iface_pick_ofport(const struct ovsrec_interface *cfg)
> +iface_validate_ofport__(size_t n, int64_t *ofport)
> +{
> + return (n && *ofport >= 1 && *ofport < OFPP_MAX
> + ? u16_to_ofp(*ofport)
> + : OFPP_NONE);
Are the outer parenthesis required?
> +}
> +
> +static ofp_port_t
> +iface_get_requested_ofp_port(const struct ovsrec_interface *cfg)
> {
> - ofp_port_t ofport = cfg->n_ofport ? u16_to_ofp(*cfg->ofport)
> - : OFPP_NONE;
> - return cfg->n_ofport_request ? u16_to_ofp(*cfg->ofport_request)
> - : ofport;
> + return iface_validate_ofport__(cfg->n_ofport_request,
> cfg->ofport_request);
> }
>
> +static ofp_port_t
> +iface_pick_ofport(const struct ovsrec_interface *cfg)
> +{
> + ofp_port_t requested_ofport = iface_get_requested_ofp_port(cfg);
> + return (requested_ofport != OFPP_NONE
> + ? requested_ofport
> + : iface_validate_ofport__(cfg->n_ofport, cfg->ofport));
> +}
>
> /* Port mirroring. */
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index f6a8db1..f21bc8a 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1306,14 +1306,19 @@
> </p>
>
> <p>
> - Open vSwitch currently assigns the OpenFlow port number for an
> - interface once, when the client first adds the interface. It does
> - not change the port number later if the client sets or changes or
> - clears <ref column="ofport_request"/>. Therefore, to ensure that
> - <ref column="ofport_request"/> takes effect, the client should set
> - it in the same database transaction that creates the interface.
> - (Future versions of Open vSwitch might honor changes to <ref
> - column="ofport_request"/>.)
> + A client should ideally set this column's value in the same
> + database transaction that it uses to create the interface. Open
> + vSwitch version 2.1 and later will honor a later request for a
> + specific port number, althuogh it might confuse some controllers:
> + OpenFlow does not have a way to announce a port number change, so
> + Open vSwitch represents it over OpenFlow as a port deletion
> + followed immediately by a port addition.
> + </p>
> +
> + <p>
> + If <ref column="ofport_request"/> is set or changed to some other
> + port's automatically assigned port number, Open vSwitch chooses a
> + new port number for the latter port.
It would be nice to have test cases that validate the above behaviors.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
