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?
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 <[email protected]>
> 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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>>> ---
>>>> 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
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
