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. 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
