Thanks, I applied this to master and branch-2.1.
On Wed, Apr 23, 2014 at 05:27:02PM -0700, Ethan Jackson wrote:
> Thanks for taking care of this.
>
> Acked-by: Ethan Jackson <et...@nicira.com>
>
>
> On Wed, Apr 23, 2014 at 5:06 PM, Ben Pfaff <b...@nicira.com> wrote:
> > Before commit 2a73b1d73d4bdb (bridge: Reconfigure in single pass.), if a
> > port disappeared, for one reason or another, from a datapath, the next
> > bridge reconfiguration pass would notice and, if the port was still
> > configured in the database, add the port back to the datapath. That
> > commit, however, removed the logic from bridge_refresh_ofp_port() that
> > did that and failed to add the same logic to the replacement function
> > bridge_delete_or_reconfigure_ports(). This commit fixes the problem.
> >
> > To see this problem on a Linux kernel system:
> >
> > ovs-vsctl add-br br0 # 1
> > tunctl -t tap # 2
> > ovs-vsctl add-port br0 tap # 3
> > ovs-dpctl show # 4
> > tunctl -d tap # 5
> > ovs-dpctl show # 6
> > tunctl -t tap # 7
> > ovs-vsctl del-port tap -- add-port br0 tap # 8
> > ovs-dpctl show # 9
> >
> > Steps 1-4 create a bridge and a tap and add it to the bridge and
> > demonstrate that the tap is part of the datapath. Step 5 and 6 delete
> > the tap and demonstrate that it has therefore disappeared from the
> > datapath. Step 7 recreates a tap with the same name, and step 8
> > forces ovs-vswitchd to reconfigure. Step 9 shows the effect of the
> > fix: without the fix, the new tap is not added back to the datapath;
> > with this fix, it is.
> >
> > Special thanks to Gurucharan Shetty <gshe...@nicira.com> for finding a
> > simple reproduction case and then bisecting to find the commit that
> > introduced the problem.
> >
> > Bug #1238467.
> > Reported-by: Ronald Lee <ronald...@vmware.com>
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > vswitchd/bridge.c | 44 +++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 41 insertions(+), 3 deletions(-)
> >
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index f46d002..45a1491 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -252,6 +252,7 @@ static bool iface_is_internal(const struct
> > ovsrec_interface *iface,
> > static const char *iface_get_type(const struct ovsrec_interface *,
> > const struct ovsrec_bridge *);
> > static void iface_destroy(struct iface *);
> > +static void iface_destroy__(struct iface *);
> > static struct iface *iface_lookup(const struct bridge *, const char *name);
> > static struct iface *iface_find(const char *name);
> > static struct iface *iface_from_ofp_port(const struct bridge *,
> > @@ -667,6 +668,9 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
> > struct ofproto_port ofproto_port;
> > struct ofproto_port_dump dump;
> >
> > + struct sset ofproto_ports;
> > + struct port *port, *port_next;
> > +
> > /* List of "ofp_port"s to delete. We make a list instead of deleting
> > them
> > * right away because ofproto implementations aren't necessarily able
> > to
> > * iterate through a changing list of ports in an entirely robust way.
> > */
> > @@ -676,11 +680,21 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
> >
> > del = NULL;
> > n = allocated = 0;
> > + sset_init(&ofproto_ports);
> >
> > + /* Main task: Iterate over the ports in 'br->ofproto' and remove the
> > ports
> > + * that are not configured in the database. (This commonly happens
> > when
> > + * ports have been deleted, e.g. with "ovs-vsctl del-port".)
> > + *
> > + * Side tasks: Reconfigure the ports that are still in 'br'. Delete
> > ports
> > + * that have the wrong OpenFlow port number (and arrange to add them
> > back
> > + * with the correct OpenFlow port number). */
> > OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) {
> > ofp_port_t requested_ofp_port;
> > struct iface *iface;
> >
> > + sset_add(&ofproto_ports, ofproto_port.name);
> > +
> > iface = iface_lookup(br, ofproto_port.name);
> > if (!iface) {
> > /* No such iface is configured, so we should delete this
> > @@ -746,11 +760,37 @@ bridge_delete_or_reconfigure_ports(struct bridge *br)
> > iface_destroy(iface);
> > del = add_ofp_port(ofproto_port.ofp_port, del, &n, &allocated);
> > }
> > -
> > for (i = 0; i < n; i++) {
> > ofproto_port_del(br->ofproto, del[i]);
> > }
> > free(del);
> > +
> > + /* Iterate over this module's idea of interfaces in 'br'. Remove any
> > ports
> > + * that we didn't see when we iterated through the datapath, i.e. ports
> > + * that disappeared underneath use. This is an unusual situation, but
> > it
> > + * can happen in some cases:
> > + *
> > + * - An admin runs a command like "ovs-dpctl del-port" (which is a
> > bad
> > + * idea but could happen).
> > + *
> > + * - The port represented a device that disappeared, e.g. a tuntap
> > + * device destroyed via "tunctl -d", a physical Ethernet device
> > + * whose module was just unloaded via "rmmod", or a virtual NIC
> > for a
> > + * VM whose VM was just terminated. */
> > + HMAP_FOR_EACH_SAFE (port, port_next, hmap_node, &br->ports) {
> > + struct iface *iface, *iface_next;
> > +
> > + LIST_FOR_EACH_SAFE (iface, iface_next, port_elem, &port->ifaces) {
> > + if (!sset_contains(&ofproto_ports, iface->name)) {
> > + iface_destroy__(iface);
> > + }
> > + }
> > +
> > + if (list_is_empty(&port->ifaces)) {
> > + port_destroy(port);
> > + }
> > + }
> > + sset_destroy(&ofproto_ports);
> > }
> >
> > static void
> > @@ -3145,8 +3185,6 @@ bridge_configure_dp_desc(struct bridge *br)
> >
> > /* Port functions. */
> >
> > -static void iface_destroy__(struct iface *);
> > -
> > static struct port *
> > port_create(struct bridge *br, const struct ovsrec_port *cfg)
> > {
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
