diff mbox series

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

Message ID 20200912071001.5576-1-mark.d.gray@redhat.com
State New
Headers show
Series [ovs-dev,v2] ovn-controller: Handle changes in requested-tnl-key | expand

Commit Message

Mark Gray Sept. 12, 2020, 7:10 a.m. UTC
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(-)

Comments

Dumitru Ceara Sept. 14, 2020, 4:47 p.m. UTC | #1
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
Numan Siddique Sept. 14, 2020, 6:12 p.m. UTC | #2
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
>
>
Mark Gray Sept. 21, 2020, 9:19 a.m. UTC | #3
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
>>
>>
>
Numan Siddique Sept. 21, 2020, 10:17 a.m. UTC | #4
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
>
>
Mark Gray Sept. 21, 2020, 2:26 p.m. UTC | #5
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.
Numan Siddique Sept. 21, 2020, 4:09 p.m. UTC | #6
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
>
>
Han Zhou Sept. 21, 2020, 7:15 p.m. UTC | #7
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
diff mbox series

Patch

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;