On Mon, Sep 21, 2020 at 2:19 AM Mark Gray <[email protected]> wrote: > > On 14/09/2020 19:12, Numan Siddique wrote: > > On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <[email protected]> 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 <[email protected]> > >>> --- > >>> 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 > >> [email protected] > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
