On 8/25/22 08:35, Han Zhou wrote:
> On Mon, Aug 22, 2022 at 9:41 AM Han Zhou <hz...@ovn.org> wrote:
>>
>>
>>
>> On Fri, Aug 19, 2022 at 8:40 AM Xavier Simonart <xsimo...@redhat.com>
> wrote:
>>>
>>> When VIF ports are claimed on a chassis, SBDB Port_Binding table is
> updated.
>>> If the SBDB IDL is still is read-only ("in transaction") when such a
> update
>>> is required, the update is not possible and recompute is triggered
> through
>>> I+P failure.
>>>
>>> This situation can happen:
>>> - after updating Port_Binding->chassis to SBDB for one port, in a
> following
>>> iteration, ovn-controller handles Interface:external_ids:ovn-installed
>>> (for the same port) while SBDB is still read-only.
>>> - after updating Port_Binding->chassis to SBDB for one port, in a
> following
>>> iteration, ovn-controller updates Port_Binding->chassis for another
> port,
>>> while SBDB is still read-only.
>>>
>>> This patch prevent the recompute, by having the if-status module
>>> updating the Port_Binding chassis (if needed) when possible.
>>> This does not delay Port_Binding chassis update compared to before this
> patch.
>>> - With the patch, Port_Binding chassis will be updated as soon as SBDB
> is
>>> again writable, without recompute.
>>> - Without the patch, Port_Binding chassis was updated as soon as SBDB
> was
>>> again writable, through a recompute.
>>>
>>> As part of this patch, ovn-installed will not be updated for additional
> chassis;
>>> it will only be updated when the migration is completed.
>>>
>>> Note that this patch also fixes an issue where port was not always
> properly
>>> reported up for virtual ports (causing flaky failures in "virtual ports"
>>> test case).
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
>>> Signed-off-by: Xavier Simonart <xsimo...@redhat.com>
>>>
>>> ---
>>> v2: - handled Dumitru's comments.
>>> - handled Han's comments, mainly ensure we moved out of CLAIMED
> state
>>> only after updating pb->chassis to guarentee physical flows are
> installed
>>> when ovn-installed is updated in OVS.
>>> - slighly reorganize the code to isolate 'notify_up = false' cases
> in
>>> claim_port (i.e. ports such as virtual ports), in the idea of
> making
>>> future patch preventing recomputes when virtual ports are
> claimed.
>>> - updated test case to cause more race conditions.
>>> - rebased on origin/main
>>> - note that "additional chassis" as now supported by
>>> "Support LSP:options:requested-chassis as a list" might still
> cause
>>> recomputes.
>>> - fixed missing flows when Port_Binding chassis was updated by
> mgr_update
>>> w/o any lflow recalculation.
>>> v3: - handled Dumitru's comments on v2, mainly have runtime_data
> handler
>>> handling pb_claims when sb becomes writable (instead of a lflow
> handler).
>>> - fixed test as it was not checking recomputes on all hv, as well
> as a flaky
>>> v4: - handled Han's comments, mainly update pb->chassis until it's
> confirmed.
>>> - updated doc to reflect that pb->chassis update can happen at any
> state, and
>>> the CLAIMED state is only for the initial attempt.
>>
>> Thanks Xavier. All looks good to me except that it seems you updated the
> diagram but forgot to update the description, I have the below small
> incremental change, and if you are ok, I can merge it without the need for
> v6:
>>
>> diff --git a/controller/if-status.c b/controller/if-status.c
>> index 2590ec27f..d1c14ac30 100644
>> --- a/controller/if-status.c
>> +++ b/controller/if-status.c
>> @@ -54,11 +54,12 @@ VLOG_DEFINE_THIS_MODULE(if_status);
>> */
>>
>> enum if_state {
>> - OIF_CLAIMED, /* Newly claimed interface. pb->chassis not yet
> updated.
>> - */
>> - OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis successfully
>> - * updated in SB and for which flows are still
> being
>> - * installed.
>> + OIF_CLAIMED, /* Newly claimed interface. pb->chassis update
> not yet
>> + initiated. */
>> + OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent
> to
>> + * SB (but update notification not confirmed, so
> the
>> + * update may be resent in any of the following
> states)
>> + * and for which flows are still being installed.
>> */
>> OIF_MARK_UP, /* Interface with flows successfully installed in
> OVS
>> * but not yet marked "up" in the binding module
> (in
>>
>> Thanks,
>> Han
>
> I heard no objection to the above and this is really minor, so applied it
> to main. Thanks Xavier!
Thanks Xavier and Han! I'm glad to see the if-status/ovn-installed code
getting in better shape.
> (Just realized after pushing, Dumitru had Acked one of the previous
> versions but I forgot to add his name. I apologize!)
No problem whatsoever. :)
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
