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

[Ben Pfaff]

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.

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.