Hi Dumitru,
On 12/07/23 4:12 pm, Dumitru Ceara wrote:
On 6/6/23 14:10, Xavier Simonart wrote:
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)
Hi Priyankar,
Thanks for the fix!
I guess we have two options:
(1) go ahead with your patch but also include Xavier's test.
(2) figure out a more generic solution which covers the second problem
reported by Xavier too and respin this patch as a v2 that fixes all cases.
I'd prefer we go for (2) mainly because we know there's a problem but
also because the "sleep 2" in the test case makes me uneasy.
Do you have some input on this?
I concur. We should go ahead with (2). Lately I was not able to spend
time on the patch. I'll try to spend some cycles working on the generic
fix for the issues forementioned.
P.S: Thanks a lot Xavier for providing an automated testcase. Appreciate
your support.
Thanks,
Priyankar
Regards,
Dumitru
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://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=BWfApgwqQ_BTXd_yEq_kwWLM-KTqkdPvolDMCJmUagg&m=5RE3xB3ecoAnUsl4KL4Rrl10TBJg061rQvodIeQHoE8XevtCE7KUyGPW6HESfx9q&s=rGwggMziqjM6RzqSmxtynVVCRbZysklqZChhzQHYPHw&e=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=BWfApgwqQ_BTXd_yEq_kwWLM-KTqkdPvolDMCJmUagg&m=5RE3xB3ecoAnUsl4KL4Rrl10TBJg061rQvodIeQHoE8XevtCE7KUyGPW6HESfx9q&s=rGwggMziqjM6RzqSmxtynVVCRbZysklqZChhzQHYPHw&e=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
