[ovs-dev] [ovs-dev, RFC] vswitchd: warn about misconfigured interface type on netdev bridge

Ilya Maximets i.maximets at samsung.com
Thu Mar 21 13:47:09 UTC 2019


On 20.03.2019 21:10, Ben Pfaff wrote:
> On Wed, Mar 20, 2019 at 06:46:22PM +0300, Ilya Maximets wrote:
>> On 20.03.2019 4:20, Ben Pfaff wrote:
>>> On Tue, Mar 19, 2019 at 07:59:55PM +0300, Ilya Maximets wrote:
>>>> On 19.03.2019 18:51, Ilya Maximets wrote:
>>>>> On 19.03.2019 12:23, Eelco Chaudron wrote:
>>>>>> When debugging an issue we noticed that by accident someone has changes the
>>>>>> bridge datapath_type to netdev, where it's clearly a kernel (system bridge)
>>>>>> with physical and tap devices. Unfortunately, this is not something you
>>>>>> will easily spot, as the bridge datapath type value is not shown by default.
>>>>>>
>>>>>> In addition, OVS is not warning you about this potential mismatch in
>>>>>> interface and bridge datapath.
>>>>>>
>>>>>> I'm sending out this patch as an RFC as I could not find a clear
>>>>>> demarcation between bridge datapath types and interface datapath types.
>>>>>> The patch below will at least warn for netdev bridges with system
>>>>>> interfaces. But no warning will be given for some unsupported virtual
>>>>>> interfaces. For system bridges, the dpdk types will no be recognized as
>>>>>> system/virtual interfaces (unless the name exists) which will result in
>>>>>> an error.
>>>>>>
>>>>>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>>>>>> ---
>>>>>
>>>>> Hi.
>>>>>
>>>>> Hmm. What do you mean under misconfiguration?
>>>>> In practice, userspace datapath is able to open "system" type netdevs.
>>>>> It's supported by netdev-linux to open system network devices via socket.
>>>>> And these devices has "system" type.
>>>>> On 'datapath_type' changes, bridge/ofproto will be destroyed and re-created.
>>>>> All the ports from kernel datapath will be destroyed and new ones created
>>>>> in userspace. All the ports should still work as usual.
>>>>>
>>>>> The only possible issue will be that this bridge will no longer communicate
>>>>> with other bridges, because they're in different datapaths now.
>>>>
>>>> For this issue, probably, following warning will give a clue to a user:
>>>>
>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>> index 21dd54bab..df51aaa3a 100644
>>>> --- a/ofproto/ofproto-dpif.c
>>>> +++ b/ofproto/ofproto-dpif.c
>>>> @@ -3557,6 +3557,10 @@ ofport_update_peer(struct ofport_dpif *ofport)
>>>>  
>>>>          break;
>>>>      }
>>>> +    if (!ofport->peer) {
>>>> +        VLOG_WARN_RL(&rl, "No usable peer '%s' exist for patch port '%s'.",
>>>> +                     peer_name, netdev_get_name(ofport->up.netdev));
>>>> +    }
>>>>      free(peer_name);
>>>>  }
>>>>  
>>>> ---
>>>>
>>>> I could send this as a proper patch.
>>>
>>> In the log message, s/exist/exists/ for English grammar reasons.
>>
>> Sure. Thanks for spotting.
>>
>>>
>>> Even with the log message, the error might not be obvious to the reader.
>>> It would be nice, if a port with the given name existed on a different
>>> backer, to somehow point that out.  It might be hard for this code to
>>> figure it out though.  Maybe it would be helpful to simply note the
>>> datapath type; that might give the reader the hint that the datapath
>>> type could be relevant.
>>
>> I thought about this a little bit more and now I think that message
>> like that will be more confusing than helpful. It's because it will
>> be printed each time while one side of the patch already created but
>> the peer is not added yet. Logs could be confusing unless we reporting
>> successful pairing.
> 
> OK.
> 
>> Maybe it's more appropriate to propagate issue like this to the 'error'
>> column of the interface. But I didn't figure out yet how to do that
>> in a good way.
> 
> You're right, that would be better.  (I don't remember how that
> propagation works.)
> 
>> BTW, maybe the following small change will help for debugging issues
>> like that:
>>
>> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c                                                                           
>> index a36905186..4948137ef 100644                                                                                                    
>> --- a/utilities/ovs-vsctl.c                                                                                                          
>> +++ b/utilities/ovs-vsctl.c                                                                                                          
>> @@ -1001,6 +1001,7 @@ static struct cmd_show_table cmd_show_tables[] = {                                                             
>>       &ovsrec_bridge_col_name,                                                                                                       
>>       {&ovsrec_bridge_col_controller,                                                                                                
>>        &ovsrec_bridge_col_fail_mode,                                                                                                 
>> +      &ovsrec_bridge_col_datapath_type,                                                                                             
>>        &ovsrec_bridge_col_ports},                                                                                                    
>>       {NULL, NULL, NULL}                                                                                                             
>>      },
>> ---
>>
>> With this change 'ovs-vsctl show' will print non-default datapath types and
>> it'll be easy to spot.
> 
> I agree.  Would you mind sending an official patch?

Sure. Sent here:
  https://patchwork.ozlabs.org/patch/1059987/

Best regards, Ilya Maximets.


More information about the dev mailing list