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 <[email protected]>
> >>>> ---
> >>>
> >>> 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?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
