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

Han Zhou zhouhan at gmail.com
Mon Sep 21 19:15:20 UTC 2020


On Mon, Sep 21, 2020 at 2:19 AM Mark Gray <mark.d.gray at redhat.com> wrote:
>
> On 14/09/2020 19:12, Numan Siddique wrote:
> > 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.
> >
>
> I did consider that at the time, I can add it.
>
> > 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.
>
> You are right, it is not 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
> >>
>
> I hadn't considered that case.
>
> If I only check for the case when the tunnel key gets updated, then this
> will force recompute on too many changes. For example, when I tried
> this, some tests in ovn_performance.at fail (when adding a logical
> router or logical switch). I don't think this is acceptable?
>
> Ideally, what I would like to do is query what the previous tnl-key was
> in order to help to determine why it was changed. Unfortunately, OVSDB
> does not provide an easy way to do this. Therefore, the other option
> would be to query the flow-table to see what value it is set to before
> deciding whether a full recompute is required.
>

Hi Mark, what do you mean by "query the flow-table"?
Here I think we can simply check if the record is updated, i.e.:

!sbrec_datapath_binding_is_new() && !sbrec_datapath_binding_is_deleted()

then return false (trigger recompute) to ensure the correctness.

Thanks,
Han

> >> # 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
> >>
> >>
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list