| Message ID | 20200912071001.5576-1-mark.d.gray@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ovn-controller: Handle changes in requested-tnl-key | expand |
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@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? > > 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
On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 14/09/2020 19:12, Numan Siddique wrote: > On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara@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@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. >> # 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@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >
On Mon, Sep 21, 2020 at 2:49 PM Mark Gray <mark.d.gray@redhat.com> wrote: > On 14/09/2020 19:12, Numan Siddique wrote: > > On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara@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@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? > I think it should be acceptable given that this use case happens rarely. I think it's better to fall back to recompute in rare cases than handle it incrementally (and in the process make the code more complicated :) if it is not straightforward to address it) Thanks Numan > > 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. > > >> # 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@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 21/09/2020 11:17, Numan Siddique wrote: > On Mon, Sep 21, 2020 at 2:49 PM Mark Gray <mark.d.gray@redhat.com> wrote: > >> On 14/09/2020 19:12, Numan Siddique wrote: >>> On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara@redhat.com> >> wrote: >>> >>>> On 9/12/20 9:10 AM, Mark Gray wrote: .. >>>> >>>> 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? >> > > I think it should be acceptable given that this use case happens rarely. > I think it's better to fall back to recompute in rare cases than handle it > incrementally > (and in the process make the code more complicated :) if it is not > straightforward to address it) > > Thanks > Numan > > Yeah, I don't want to fall into the trap of optimizing too early. What I don't know is how often a lr-add or ls-add happens. However, from what you are suggesting here, it is not too often so I will make the change as you suggest.
On Mon, Sep 21, 2020, 7:56 PM Mark Gray <mark.d.gray@redhat.com> wrote: > On 21/09/2020 11:17, Numan Siddique wrote: > > On Mon, Sep 21, 2020 at 2:49 PM Mark Gray <mark.d.gray@redhat.com> > wrote: > > > >> On 14/09/2020 19:12, Numan Siddique wrote: > >>> On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara@redhat.com> > >> wrote: > >>> > >>>> On 9/12/20 9:10 AM, Mark Gray wrote: > .. > >>>> > >>>> 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? > >> > > > > I think it should be acceptable given that this use case happens rarely. > > I think it's better to fall back to recompute in rare cases than handle > it > > incrementally > > (and in the process make the code more complicated :) if it is not > > straightforward to address it) > > > > Thanks > > Numan > > > > > > Yeah, I don't want to fall into the trap of optimizing too early. What I > don't know is how often a lr-add or ls-add happens. However, from what > you are suggesting here, it is not too often so I will make the change > as you suggest. > Hi Mark, I think I misunderstood you. ls-add/lr-add would often happen. But setting the tunnel key would not be that often. So I thought your patch would trigger a recompute when a datapath in the south db would get updated (and not when it is added ). Thanks Numan > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, Sep 21, 2020 at 2:19 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > On 14/09/2020 19:12, Numan Siddique wrote: > > On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara@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@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@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 21/09/2020 20:15, Han Zhou wrote: > On Mon, Sep 21, 2020 at 2:19 AM Mark Gray <mark.d.gray@redhat.com> wrote: >> >> On 14/09/2020 19:12, Numan Siddique wrote: >>> On Mon, Sep 14, 2020 at 10:18 PM Dumitru Ceara <dceara@redhat.com> > wrote: >>> >>>> On 9/12/20 9:10 AM, Mark Gray wrote: >>>>> runtime_data_sb_datapath_binding_handler() only handles deletion of .. > > 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. > Hi Han, I originally tried to do this but it triggered failures in other unit tests. I assumed it was because I misunderstood when this table was getting updated for other use cases and,therefore, I needed to have a more specific check. However based on your feedback, I looked at this again and realised the _is_new() function was never returning 'true'. I spent some time root-causing and found that it was an issue in the OVSDB IDL code. I have submitted a patch to fix - it would be great if you could review because I think you are familiar with the code. It's at https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/375645.html. I have another patch ready for 'ovn-controller: Handle changes in requested-tnl-key' satisfying all the review comments but I will wait until 'ovsdb-idl: Fix *_is_new() IDL functions' has been committed before reposting. Mark
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;
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@redhat.com> --- controller/ovn-controller.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)