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

[Ilya Maximets]

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

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.

$ ovs-vsctl show
c2513e49-0e08-4cd7-b633-461c88d810ff
    Bridge "br0"
        datapath_type: netdev
        Port "br0"
            Interface "br0"
                type: internal


Best regards, Ilya Maximets.