[ovs-dev] [PATCH ovn] Revert "lflow.c: Use warn level log for a lflow that has neither dp nor dpg."

Dumitru Ceara dceara at redhat.com
Thu Apr 29 10:12:02 UTC 2021


On 4/29/21 11:29 AM, Ilya Maximets wrote:
> On 4/29/21 10:07 AM, Dumitru Ceara wrote:
>> On 4/29/21 3:55 AM, Han Zhou wrote:
>>> This reverts commit 0ba2e1a2e2c413f057ac198e4c2bcf00ac40b19f.
>>
>> Oops, I should've thought about this when reviewing the initial patch,
>> good that the CI caught it.
>>
>>>
>>> System tests with "dp-groups=yes" are all failing due to the warning
>>> log. Need to investigate why the warning appears. But before that is
>>> clear, revert this log level change to make CI pass.
>>
>> I would remove this part of the commit log or change it to something like:
>>
>> "With conditional monitoring, if a logical flow refers to a datapath
>> group that doesn't contain any local datapaths, ovsdb-server will send
>> updates with lflow.logical_dp_group set to NULL.
> 
> This is not correct.  lflow does have a logical_dp_group in a message
> received from the ovsdb-server.  It's IDL that hides it from a user
> because it didn't receive the group itself.  And it's allowed to do
> that because of the 'min:0, max:1' definition of the column in the schema.

You're right, thanks for the correction.

> 
>> This is a valid case
>> and we shouldn't log a warning message for it."
> 
> +1
> It's a valid case and should not be logged as a warning.
> 
>>
>> It's actually expected to have lflows with both logical_dp_group and
>> logical_datapath NULL now since we unconditionally monitor lflows with
>> datapath group set.
> 
> Another point here is that we can't actually guarantee the order of updates
> from the server.  Even though currently ovsdb-server sends updates for
> monitored tables in a single 'update' message, it's not mandatory and could
> change in the future.  In this case it will be possible to receive lfow
> before receiving dp_group even with monotor-all.

+1

So even with the additional patch Han is preparing (to unconditionally
monitor all DPGs) we should still revert this change.

> 
>>
>> With conditional monitoring it might be that there are lflows that refer
>> to a DPG but none of the DPG datapaths are local to ovn-controller so
>> they won't be monitored.  In that case lflow.logical_dp_group will be
>> NULL.  It's not a bug however so the old VLOG_DBG was fine.
>>
>> For the revert patch:
>> Acked-by: Dumitru Ceara <dceara at redhat.com>
>>
>> Thanks,
>> Dumitru
>>
> 



More information about the dev mailing list