[ovs-dev] [PATCH ovn v2] ovn-controller: Handle changes in requested-tnl-key

Numan Siddique numans at ovn.org
Mon Sep 14 18:12:34 UTC 2020


On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara at redhat.com> wrote:

> On 9/12/20 9:10 AM, Mark Gray wrote:
> > runtime_data_sb_datapath_binding_handler() only handles deletion of rows
> > in the Datapath_Binding table in OVN-SB. The user can update the
> > requested-tnl-key by the following command:
> >
> > ovn-nbctl set logical_switch sw0 other_config:requested-tnl-key=<key>
> >
> > This command modifies the tunnel_key column. This patch
> > ensures that an update of this column is handled by the
> > incremental processing engine by forcing a recompute
> > of the runtime_data node.
> >
> > As it is expected that changing the requested-tnl-key is not updated
> > frequently, we do not attempt to make this update incrementally.
> >
> > Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
> > ---
> >  controller/ovn-controller.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
>
> Hi Mark,
>
> Would you mind adding a test case for this in ovn.at?
>

Hi Mark,

I didn't review the code yet. I agree with Dumitru if you could add a test
case.
I think you can  enhance this test case in this file too -
https://github.com/ovn-org/ovn/blob/master/tests/ovn-performance.at
and make sure that lflow_run is triggered when a datapath_binding is
updated.

Thanks
Numan


> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 995805470..106f8eae1 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1362,8 +1362,8 @@ runtime_data_sb_port_binding_handler(struct
> engine_node *node, void *data)
> >  }
> >
> >  static bool
> > -runtime_data_sb_datapath_binding_handler(struct engine_node *node
> OVS_UNUSED,
> > -                                         void *data OVS_UNUSED)
> > +runtime_data_sb_datapath_binding_handler(struct engine_node *node,
> > +                                         void *data)
> >  {
> >      struct sbrec_datapath_binding_table *dp_table =
> >          (struct sbrec_datapath_binding_table *)EN_OVSDB_GET(
> > @@ -1378,6 +1378,18 @@ runtime_data_sb_datapath_binding_handler(struct
> engine_node *node OVS_UNUSED,
> >                  return false;
> >              }
> >          }
> > +
> > +        /* Force recompute when the tunnel_key is updated. However,
> > +           don't update if the external_id column is updated as this
> > +           can be done when a logical switch or logical router is
> > +           added which does not recquire a recompute.
> > +        */
> > +        if (sbrec_datapath_binding_is_updated(dp,
> > +                 SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY) &&
> > +            !sbrec_datapath_binding_is_updated(dp,
> > +                 SBREC_DATAPATH_BINDING_COL_EXTERNAL_IDS)) {
> > +            return false;
> > +        }
> >      }
> >
> >      return true;
> >
>
> I'm not sure whether this is completely correct.
>
> What if the sequence of operations is:
>
> ovn-nbctl set logical_switch sw other_config:requested-tnl-key=42
> # ovn-northd writes to datapath_binding.tunnel_key
>
> ovn-sbctl set datapath_binding sw external_ids:foo=bar
>
> # It can happen that ovn-controller gets both SB updates at the
> # same time and we end up returning true.
>
> That would mean we successfully "incrementally processed" the datapath
> binding update but we actually didn't update the flows, right?
>
> Or am I missing something?
>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list