[ovs-dev] [PATCH] ovn-controller: process lport bindings only when transaction is possible

Ben Pfaff blp at ovn.org
Sat Jul 2 19:23:35 UTC 2016


On Sat, Jul 02, 2016 at 01:56:21PM -0400, Lance Richardson wrote:
> As currently implemented, binding_run() normally updates the set of
> locally owned logical ports on each call.  When changes to the
> membership of this set are detected (i.e. when locally bound
> logical ports are added or deleted), additional processing to
> update the sb database with lport binding is performed.
> 
> However, the sb database can only be updated when a transaction to
> the sb database is possible (that is, when ctx->ovnsb_idl_txn is
> non-NULL).  If a new logical port is detected  while ctx->ovnsb_idl_txn
> happens to be NULL, its binding information will not be updated in
> the the sb database until another change to the set of locally-owned
> logical ports changes. If no such change ever occurs, the sb database
> is never updated with the appropriate binding information.
> 
> Eliminate this issue by only updating the set of locally owned logical
> ports when an sb database transaction is possible. This addresses
> a cause of occasional failures in the "3 HVs, 3 LS, 3 lports/LS, 1 LR"
> test case.
> 
> Signed-off-by: Lance Richardson <lrichard at redhat.com>
> ---
>  ovn/controller/binding.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index 8b439a6..369f8f2 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -266,7 +266,7 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
>      }
>  
>      if (br_int) {
> -        if (get_local_iface_ids(br_int, &lports)) {
> +        if (ctx->ovnsb_idl_txn && get_local_iface_ids(br_int, &lports)) {
>              process_full_binding = true;
>          }
>      } else {

At a glance, get_local_iface_ids() doesn't seem to directly modify the
southbound database, but it does modify data structures that later code
depends on.  Based on that, it's not obvious for me to see that this is
a correct change.  Can you point out anything relevant?

Thanks,

Ben.



More information about the dev mailing list