Hi Priyankar, Mark
Thanks for the patch. I agree with Mark - the description is really great !
Based on your description, I tried creating a unit-test reproducing the
issue, and checking that your patch fixes it.
I came up with [0]. It reproduces some issues (flows not deleted) on
origin/main, and the issues fixed using your patch. So, it looks good.
However, if, in [0] I remove the "sleep 2" (see below), then it seems that
there are still some issues.
It might not be exactly the same issue you saw, but is very similar - the
same flow does not get properly deleted.
I think that the (new) issue is the following:
When a port is claimed by two different chassis (as part of the migration),
ovn-controllers try to avoid a "war" between themselves, and postpone port
claiming if the port got claimed very recently.
This works fine. But, if, while a port claim is postponed, the interface is
deleted, it seems that some flows are not properly removed.
Checking that the port is postponed in is_binding_lport_this_chassis might
be enough, but this requires additional check.
(if we want to add this unit test to the patch, then we probably need to
move some of the functions to ovn-macros to avoid duplication, as I steal
them from the system tests)
Thanks
Xavier
[0]
OVN_FOR_EACH_NORTHD([
AT_SETUP([XXXX)
ovn_start
ovn-nbctl ls-add ls0
ovn-nbctl lsp-add ls0 lsp0
ovn-nbctl lsp-add ls0 lsp1
net_add n1
for i in 1 2; do
sim_add hv$i
as hv$i
ovs-vsctl add-br br-phys
ovn_attach n1 br-phys 192.168.0.$i
ovn-appctl vlog/set dbg
done
sleep_sb() {
echo SB going to sleep
AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)])
}
wake_up_sb() {
echo SB waking up
AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)])
}
sleep_controller() {
hv=$1
echo Controller $hv going to sleep
as $hv ovn-appctl debug/pause
OVS_WAIT_UNTIL([test x$(as $hv ovn-appctl -t ovn-controller debug/status)
= "xpaused"])
}
wake_up_controller() {
hv=$1
echo Controller $hv going to wake up
as $hv ovn-appctl debug/resume
OVS_WAIT_UNTIL([test x$(as $hv ovn-appctl -t ovn-controller debug/status)
= "xrunning"])
}
get_flows()
{
hv=$1
out=oflows${2}
as $hv ovs-ofctl dump-flows br-int |
sed -e 's/cookie=[[0-9.]]*s, //g' |
sed -e 's/duration=[[0-9.]]*s, //g' |
sed -e 's/idle_age=[[0-9]]*, //g' |
sed -e 's/n_packets=[[0-9]]*, //g' |
sed -e 's/n_bytes=[[0-9]]*, //g' | sort -k2 > $out
AT_CAPTURE_FILE([$out])
}
check ovn-nbctl --wait=hv sync
hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1)
hv2_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv2)
as hv1 ovs-vsctl -- add-port br-int vif1 -- set Interface vif1
external-ids:iface-id=lsp1
as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif
external-ids:iface-id=lsp0
check ovn-nbctl pg-add pg1 lsp0 lsp1
wait_for_ports_up
check ovn-nbctl --wait=hv acl-add ls0 to-lport 1000 'eth.type == 0x1234 &&
outport == @pg1' drop
# Delete vif => store flows w/ only vif1, and no vif
as hv1 ovs-vsctl -- del-port br-int vif
check ovn-nbctl --wait=hv sync
get_flows hv1 1
AS_BOX([sleeping hv1, binding hv1 and hv2])
sleep_controller hv1
as hv2 ovs-vsctl -- add-port br-int vif -- set Interface vif
external-ids:iface-id=lsp0
as hv1 ovs-vsctl -- add-port br-int vif -- set Interface vif
external-ids:iface-id=lsp0
OVS_WAIT_UNTIL([
chassis=$(ovn-sbctl --bare --columns chassis list port_binding lsp0)
test "$chassis" = $hv2_uuid
])
AS_BOX([sleeping sb, waking up hv1, Sleeping hv2])
sleep_sb
wake_up_controller hv1
sleep_controller hv2
*sleep 2*
AS_BOX([Unbinding hv1, waking up sb, waking up hv2])
as hv1 ovs-vsctl -- del-port br-int vif
wake_up_sb
wake_up_controller hv2
AS_BOX([Unbinding hv2])
as hv2 ovs-vsctl -- del-port br-int vif
check ovn-nbctl --wait=hv sync
get_flows hv1 2
check diff oflows1 oflows2
OVN_CLEANUP([hv1],[hv2])
AT_CLEANUP
])
~
On Thu, Jun 1, 2023 at 8:21 PM Priyankar Jain <priyankar.j...@nutanix.com>
wrote:
> Hi Mark,
>
> Thanks for the review.
>
> On 01/06/23 12:29 am, Mark Michelson wrote:
> > Hi Priyankar,
> >
> > The description makes the issue crystal clear, and you appear to be
> > solving the race condition that can happen between the OVS interface
> > table and the southbound port_binding table.
> >
> > Acked-by: Mark Michelson <mmich...@redhat.com>
> >
> > Just to let you know, the flapping problem you mention can be avoided
> > altogether by using options:requested-chassis on the northbound logical
> > switch port. When you migrate the port to a new chassis, place the new
> > chassis's name or hostname as this option, and ovn-controller will only
> > claim the logical switch port on that chassis. The old chassis will not
> > try to claim the port even if the tap is still present.
> >
>
> Thanks for the suggestion. I'll definitely try out this.
> Appreciate all the help!
>
> Regards,
> Priyankar
>
> > I wouldn't be surprised if there were other ways to trigger this race
> > condition as well. I suspect the port-flapping scenario is most likely
> > to trigger it, though.
> >
> > On 5/31/23 01:35, Priyankar Jain wrote:
> >> Currently during port migration, two chassis (source and destination)
> >> can try to claim the same logical switch port simultaneously for a
> >> short-period of time until the tap is deleted on source hypervisor.
> >> ovn-controllers on these 2 hosts constantly receives port-binding
> >> updates about other chassis claiming the port and as a result it tries
> >> to claim the port again (because its chassis has a tap interface
> >> referencing the LSP). This flapping ends once CMS cleans up tap
> >> interface from the source chassis.
> >>
> >> Now following steps occur during a single iteration inc-proc-eng during
> >> flapping:
> >>
> >> 1. PB update received on OVN controller about other chassis owning the
> >> port.
> >> 2. ovn-controller tries to claim the port.
> >> 3. It installs the OVS flows for the port and updates the runtime_data
> >> to include this port in locally relevant ports.
> >> 4. If some change to runtime data happens as part of 3, port-groups
> >> containing the affected ports are recomputed. It uses related_lports
> >> runtime data to compute the port-groups.
> >>
> >> Finally, ovn-controller sends a port-binding update to SB changing the
> >> chassis to itself.
> >> At a later point of time, SB sends the notification to ovn-controller
> >> about (4) being completed.
> >>
> >> Once CMS deletes the tap interface, ovn-controller receives the
> >> notification and updates the runtime data accordingly.
> >>
> >> Issue: ovs-flows are (sometimes)not cleaned up upon port migration.
> >>
> >> If the notification of OVS interface deletion is received before SB
> >> acks the PortBinding update, then ovn-controller does not cleanup
> >> related_lports leading to incorrect port-groups computation.
> >>
> >> i.e if the order of events is as follows:
> >>
> >> 1. PB update received on OVN controller about other chassis owning the
> >> port.
> >> 2. ovn-controller claims the port, installs OVS flows and sends the
> >> PortBinding update to SB.
> >> 3. OVS interface deletion notification received by ovn-controller.
> >> 4. SB ack received for step-2 PB update.
> >>
> >> This commit fixes this issue by removing the logical_port from related
> >> port even in case there is no binding available locally.
> >>
> >> Signed-off-by: Priyankar Jain <priyankar.j...@nutanix.com>
> >> ---
> >> controller/binding.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/controller/binding.c b/controller/binding.c
> >> index 9b0647b70..9889be5c7 100644
> >> --- a/controller/binding.c
> >> +++ b/controller/binding.c
> >> @@ -1568,6 +1568,7 @@ consider_vif_lport_(const struct
> >> sbrec_port_binding *pb,
> >> || is_additional_chassis(pb, b_ctx_in->chassis_rec)) {
> >> /* Release the lport if there is no lbinding. */
> >> if (!lbinding_set || !can_bind) {
> >> + remove_related_lport(pb, b_ctx_out);
> >> return release_lport(pb, b_ctx_in->chassis_rec,
> >> !b_ctx_in->ovnsb_idl_txn,
> >> b_ctx_out->tracked_dp_bindings,
> >
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
