On Wed, Dec 11, 2013 at 02:17:56PM -0800, Jarno Rajahalme wrote:
>
> 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?
My intent here was actually to avoid churn in some theoretical ofproto
implementation which limits (or entirely fixes) the port numbers that
certain ports can have, by only trying a given ofport_request once until
it changes. But that may not be a real problem (I don't know of any
ofprotos that actually work that way), whereas the problem you describe
is one that I would like to avoid in reality.
I folded this in:
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 37f5812..f3b03af 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -75,7 +75,6 @@ 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. */
@@ -684,7 +683,7 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
* 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 &&
+ if (iface->ofp_port != OFPP_LOCAL &&
requested_ofp_port != OFPP_NONE &&
requested_ofp_port != iface->ofp_port) {
ofp_port_t victim_request;
@@ -1494,7 +1493,6 @@ 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;
> > 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?
Only by the style guide:
Do parenthesize a subexpression that must be split across more than
one line, e.g.:
*idxp = ((l1_idx << PORT_ARRAY_L1_SHIFT)
| (l2_idx << PORT_ARRAY_L2_SHIFT)
| (l3_idx << PORT_ARRAY_L3_SHIFT));
In turn that was inspired by this advice from the GNU coding standards,
but I rewrote it to be editor agnostic:
Insert extra parentheses so that Emacs will indent the code properly.
For example, the following indentation looks nice if you do it by hand,
v = rup->ru_utime.tv_sec*1000 + rup->ru_utime.tv_usec/1000
+ rup->ru_stime.tv_sec*1000 + rup->ru_stime.tv_usec/1000;
but Emacs would alter it. Adding a set of parentheses produces
something that looks equally nice, and which Emacs will preserve:
v = (rup->ru_utime.tv_sec*1000 + rup->ru_utime.tv_usec/1000
+ rup->ru_stime.tv_sec*1000 + rup->ru_stime.tv_usec/1000);
> It would be nice to have test cases that validate the above behaviors.
I've now written one, and I'll fold it in:
AT_SETUP([ofproto - ofport_request])
OVS_VSWITCHD_START
ADD_OF_PORTS([br0], [1], [2], [3])
set_and_check_specific_ofports () {
ovs-vsctl set Interface p1 ofport_request="$1" -- \
set Interface p2 ofport_request="$2" -- \
set Interface p3 ofport_request="$3"
ofports=`ovs-vsctl get Interface p1 ofport -- \
get Interface p2 ofport -- \
get Interface p3 ofport`
AT_CHECK_UNQUOTED([echo $ofports], [0], [$1 $2 $3
])
}
for pre in '1 2 3' '1 3 2' '2 1 3' '2 3 1' '3 1 2' '3 2 1'; do
for post in '1 2 3' '1 3 2' '2 1 3' '2 3 1' '3 1 2' '3 2 1'; do
echo -----------------------------------------------------------
echo "Check changing port numbers from $pre to $post"
set_and_check_ofports $pre
set_and_check_ofports $post
done
done
ovs-vsctl del-port p3
set_and_check_poorly_specified_ofports () {
ovs-vsctl set Interface p1 ofport_request="$1" -- \
set Interface p2 ofport_request="$2"
p1=`ovs-vsctl get Interface p1 ofport`
p2=`ovs-vsctl get Interface p2 ofport`
echo $p1 $p2
AT_CHECK([test "$p1" != "$p2"])
if test "$1" = "$2" && test "$1" != '[[]]'; then
# One port number must be the requested one.
AT_CHECK([test "$p1" = "$1" || test "$p2" = "$1"])
# The other port number must be different (already tested above).
else
AT_CHECK([test "$1" = '[[]]' || test "$p1" == "$1"])
AT_CHECK([test "$2" = '[[]]' || test "$p2" == "$2"])
fi
}
for pre in '1 2' '[[]] 2' '1 [[]]' '[[]] [[]]' '2 1' '[[]] 1' '2 [[]]' \
'1 1' '2 2'; do
for post in '1 2' '[[]] 2' '1 [[]]' '[[]] [[]]' '2 1' '[[]] 1' '2 [[]]' \
'1 1' '2 2'; do
echo -----------------------------------------------------------
echo "Check changing port numbers from $pre to $post"
set_and_check_poorly_specified_ofports $pre
set_and_check_poorly_specified_ofports $post
done
done
OVS_VSWITCHD_STOP
AT_CLEANUP
I have a feeling that we will uncover some more bugs here, because this
code is some of the thorniest in OVS. I'm going to push this in a
minute anyway. We can fix the bugs as they are reported, and I hope to
add tests for them this time.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
