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

Mark Gray mark.d.gray at redhat.com
Mon Sep 21 09:19:16 UTC 2020


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.

>> # 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
>>
>>
> 



More information about the dev mailing list