Hi Han
Sorry for not replying earlier - I wanted to give a last chance for Dumitru
or others to comment
Thanks for applying the patch and all you feedback
Thanks
Xavier
On Thu, Aug 25, 2022 at 10:42 AM Dumitru Ceara <dce...@redhat.com> wrote:
> 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
