[ovs-dev] [add-remove 7/7] vswitchd: Better tolerate changes in datapath ports.
Ben Pfaff
blp at nicira.com
Fri Oct 1 23:46:10 UTC 2010
On Tue, Sep 28, 2010 at 05:23:36PM -0700, Jesse Gross wrote:
> On Tue, Sep 28, 2010 at 4:11 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Tue, Sep 28, 2010 at 03:54:20PM -0700, Jesse Gross wrote:
> >> On Tue, Sep 28, 2010 at 11:58 AM, Ben Pfaff <blp at nicira.com> wrote:
> >> > This commit fixes the problem by adding the ifindex to the callbacks. In
> >> > step 3, bridge_port_changed_ofhook_cb sees that the port that was deleted
> >> > was not the same as the one that it has in its interface and ignores it.
> >> >
> >> > Thanks to Jesse Gross for identifying the problem.
> >>
> >> I've only skimmed through this set very quickly, so this isn't a full
> >> review yet and it is possible that I missed something. However,
> >> ifindex is only available for devices that exist in the system but not
> >> virtual ports that we create. Therefore, won't this reject all
> >> virtual ports because they always return -EOPNOTSUPP when their
> >> ifindex is requested?
> >
> > It doesn't have a way to uniquely identify instantiations of a vport But
> > it accepts EOPNOTSUPP as an indication that it can't tell them apart
> > (see the change to make_ofport()). So it will always consider any two
> > non-netdev based vports to be the same.
>
> Oops, you're right. I misread the != -EOPNOTSUPP.
>
> However, this problem was originally reported for GRE ports and that
> seems like the place where it is most likely to happen given that the
> ports can be created out of thin air and therefore are more likely to
> change. I think we need to cover vports as well.
>
> >
> > Is there a way to uniquely identify an instantiation of a vport? Should
> > we invent one?
>
> No, there's nothing analogous that comes to mind (really, the OpenFlow
> port number is the closest equivalent but that is obviously
> insufficient).
>
> What would a vport identifier look like? Just a monotonically
> increasing number? In that case, would it possible to make the
> OpenFlow port number have similar properties?
>
> >
> > I suppose that two vports with the same name, port number, and
> > configuration could be considered the same vport. But currently there
> > is no way, as far as I know, to retrieve the configuration of a vport
> > from the kernel. (I added one in the netlink series.) If we could do
> > that, then we could at least determine whether vports were equivalent.
>
> We do a similar thing inside netdev.c to check whether to reconfigure
> based on changes in the arguments. I suppose we could do something
> along those lines. I think there would still be a corner case if you
> delete and add a port with the same configuration at the same time.
> It's illogical but not specifically disallowed or checked for.
Wait a second. We probably still need to solve this question of a
vport's identity and configuration, but I have a better solution: just
stop caring about ports getting deleted. Appended.
> However, at another level it seems a bit weird to have to do that.
> After all, userspace is the one making these changes and receiving the
> notification, it's not as if any of this is truly "unexpected" as the
> log message indicates.
It's unexpected in that this message is only supposed to show up if an
admin runs "ovs-dpctl del-if" on a port in a ovs-vswitchd managed
bridge. The admin isn't supposed to do that.
> Ideally, I would like to see this handled by closer sharing of
> information between userspace components. Passing information between
> the different pieces through the kernel has always struck me as a bit
> odd. Maybe making that change isn't worth it to fix this bug but the
> loose coupling seems inherently racy.
You're right. I looked again and the best solution, to me, seems to be
getting rid of port_changed_cb.
I'll repost the whole updated series in a few minutes.
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <blp at nicira.com>
Date: Tue, 28 Sep 2010 11:57:40 -0700
Subject: [PATCH] vswitchd: Better tolerate changes in datapath ports.
Until now, a command that removed and added ports in a single change to
the database, e.g.:
ovs-vsctl del-port br0 vif1.0 -- add-port br0 vif2.0
typically failed, because of this sequence of events:
1. Bridge code removes vif1.0 from br0.
2. Bridge code adds vif2.0 to br0.
3. ofproto_run() receives kernel notification that vif1.0 was deleted, so
it notifies the bridge by calling back to bridge_port_changed_ofhook_cb,
which sees that it has an interface with the specified port number, and
deletes it. Oops--this is where the problem occurs. For completeness:
4. ofproto_run() receives kernel notification that vif2.0 was added, so
it notifies the bridge by calling back to ,
which sees that it has no interface with the specified port number, and
does nothing.
This commit fixes the problem by making bridge_port_changed_ofhook_cb() not
care about ports being dropped. This is a corner case that we shouldn't
work too hard to care about, since it can only happen if an administrator
is meddling with datapaths using ovs-dpctl, and the consequences are simply
that packets directed to that device will take longer to be rerouted to
another device (it will take a while for the MAC learning table to time out
the entry). Basically, the admin gets what he deserves.
Thanks to Jesse Gross for identifying the problem.
Bug #3671.
---
vswitchd/bridge.c | 25 ++++++++-----------------
1 files changed, 8 insertions(+), 17 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 3012f3c..b0b2f72 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2563,29 +2563,20 @@ bridge_port_changed_ofhook_cb(enum ofp_port_reason reason,
struct iface *iface;
struct port *port;
+ if (reason == OFPPR_DELETE || !br->has_bonded_ports) {
+ return;
+ }
+
iface = iface_from_dp_ifidx(br, ofp_port_to_odp_port(opp->port_no));
if (!iface) {
return;
}
port = iface->port;
- if (reason == OFPPR_DELETE) {
- VLOG_WARN("bridge %s: interface %s deleted unexpectedly",
- br->name, iface->name);
- iface_destroy(iface);
- if (!port->n_ifaces) {
- VLOG_WARN("bridge %s: port %s has no interfaces, dropping",
- br->name, port->name);
- port_destroy(port);
- }
-
- bridge_flush(br);
- } else {
- if (port->n_ifaces > 1) {
- bool up = !(opp->state & OFPPS_LINK_DOWN);
- bond_link_status_update(iface, up);
- port_update_bond_compat(port);
- }
+ if (port->n_ifaces > 1) {
+ bool up = !(opp->state & OFPPS_LINK_DOWN);
+ bond_link_status_update(iface, up);
+ port_update_bond_compat(port);
}
}
--
1.7.1
More information about the dev
mailing list